Hello,<div><br></div><div> I was wondering if I could get a review of this patch. </div><div><br></div><div>Thanks,</div><div><br></div><div>Gabe<br><br>On Thursday, January 29, 2015, Gabe Alford <<a href="mailto:redhatrises@gmail.com">redhatrises@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>Hello,<br><br></div><div> Here is a patch for <a href="https://fedorahosted.org/freeipa/ticket/4029" target="_blank">https://fedorahosted.org/freeipa/ticket/4029</a> I added test cases for valid and invalid advice.<br></div><div><br></div>Thanks,<br></div><br>Gabe<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 14, 2015 at 10:23 AM, Tomas Babej <span dir="ltr"><<a href="javascript:_e(%7B%7D,'cvml','tbabej@redhat.com');" target="_blank">tbabej@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><div><div>
<br>
<div>On 01/14/2015 06:13 PM, Gabe Alford
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">On Wed, Jan 14, 2015 at 10:05 AM, Tomas
Babej <span dir="ltr"><<a href="javascript:_e(%7B%7D,'cvml','tbabej@redhat.com');" target="_blank">tbabej@redhat.com</a>></span>
wrote:<br>
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"> <br>
<div>On 01/14/2015 06:00 PM, Tomas Babej wrote:<br>
</div>
<blockquote type="cite">
<div>
<div> <br>
<div>On 01/14/2015 05:37 PM, Tomas Babej wrote:<br>
</div>
<blockquote type="cite"> <br>
<div>On 01/14/2015 02:55 PM, Gabe Alford wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>
<div>
<div>Hello,<br>
<br>
</div>
In looking into <a href="https://fedorahosted.org/freeipa/ticket/4029" target="_blank">https://fedorahosted.org/freeipa/ticket/4029</a>
I am wondering if there should be
separate ipa-advise test, Yes/No? Could
be handy in the future to test more
ipa-advise output? Or should this test
be added to the test_legacy_clients.py?<br>
<br>
</div>
Thanks,<br>
<br>
</div>
Gabe </div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Tue, Dec 2, 2014
at 9:21 PM, Gabe Alford <span dir="ltr"><<a href="javascript:_e(%7B%7D,'cvml','redhatrises@gmail.com');" target="_blank">redhatrises@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div>
<div>Hello,<br>
<br>
I was going to try my hand at
attempting a patch for ipa-tests.
However in wanting to test my
patch, I am not sure how to run
ipa-tests to check if it works or
not. Documentation is not really
clear on what needs to be done to
start a test and run a test. This
is for <a href="https://fedorahosted.org/freeipa/ticket/4029" target="_blank">https://fedorahosted.org/freeipa/ticket/4029</a>
<br>
<br>
</div>
I have attached the patch that I
have yet to really test with
ipa-test. Any help on how to test
the patch running ipa-tests would be
great. Of course, if one of the
reviewers looks at the patch and
looks good, then I would be happy
with that as well.<br>
<br>
</div>
Thanks,<br>
<br>
Gabe<br>
</div>
</blockquote>
</div>
<br>
</div>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
Freeipa-devel mailing list
<a href="javascript:_e(%7B%7D,'cvml','Freeipa-devel@redhat.com');" target="_blank">Freeipa-devel@redhat.com</a>
<a href="https://www.redhat.com/mailman/listinfo/freeipa-devel" target="_blank">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
Hello,<br>
<br>
TL;DR: feel free to create a separate ipa-advise
test file. Test requested in this ticket really
does not belong to the legacy clients feature
test.<br>
<br>
As for the any new tests that might come: I
think tests for ipa-advise that are specific to
that particular feature should be tested with
that feature, more so, if they contain parts
that are supposed to work copy-pasted. If a
tests, however, tests a general behaviour of
ipa-advise, it should live in the ipa-advise
namespace, hence separate test file.<br>
<br>
HTH,<br>
<br>
<pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</blockquote>
<br>
</div>
</div>
The attached patch looks fine, although, please also
test for a non-zero return code number.<br>
<br>
</blockquote>
<br>
Upon hitting send I noticed you did not include
raiseonerr=False into the run_command call. You need to
do that, otherwise a exception will be raised, since
ipa-advise exited with non-zero return code.<span><br>
</span></div>
</blockquote>
<div>Thanks Tomas.<br>
<br>
</div>
<div>Which do you prefer: a test_advise.py or an update to
the existing patch? <br>
</div>
</div>
</div>
</div>
</blockquote>
<br></div></div>
A new test file, as I pointed out in the second email :) sorry for
splitting.<br>
<br>
However, it would be the best if you could spin up a positive test
as well (maybe listing out available advices), not just this
negative one, to justify the overhead reinstalling IPA for testing
this feature.<span><br>
<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><span>
<blockquote type="cite"> <br>
<pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</blockquote>
<br>
<pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</span></div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</span><span><pre cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | <a href="http://freeipa.org" target="_blank">freeipa.org</a> </pre>
</span></div>
</blockquote></div><br></div>
</blockquote></div>