<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 06/20/2013 12:56 PM, Tomas Babej
wrote:<br>
</div>
<blockquote cite="mid:51C2DFF5.8040608@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 06/17/2013 02:34 PM, Ana
Krivokapic wrote:<br>
</div>
<blockquote cite="mid:51BF0247.4040604@redhat.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 06/06/2013 11:10 AM, Tomas Babej
wrote:<br>
</div>
<blockquote cite="mid:51B05207.4010100@redhat.com" type="cite">Hi,
<br>
<br>
Adds --use-posix option to ipa trust-add command. It takes two
<br>
allowed values: <br>
'yes' : the 'ipa-ad-trust-posix' range type is enforced <br>
'no' : the 'ipa-ad-trust' range type is enforced <br>
<br>
When --use-posix option is not specified, the range type
should be <br>
determined by ID range discovery. <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://fedorahosted.org/freeipa/ticket/3650">https://fedorahosted.org/freeipa/ticket/3650</a>
<br>
<br>
Tomas <br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Freeipa-devel mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
The patch works nicely, but I have a few comments:<br>
<br>
1) You added a new option to the API, but you forgot to bump the
IPA_API_VERSION_MINOR in the VERSION file.<br>
<br>
2) Typo in commit message: "shold" instead of "should".<br>
<br>
3) This construct:<br>
<br>
<pre wrap="">+ if range_type is not None:
+ if range_type != old_range_type:
can be replaced with a more readable variant which also avoids nested ifs:
+ if range_type and range_type != old_range_type:
</pre>
<br>
</blockquote>
<br>
All fixed.<br>
<br>
<blockquote cite="mid:51BF0247.4040604@redhat.com" type="cite">4)
Some unit tests to cover the behavior of the newly added option
would be nice.<br>
</blockquote>
<br>
This is not doable at the moment, we have no unit test framework
to test the trust-add command.<br>
<br>
<blockquote cite="mid:51BF0247.4040604@redhat.com" type="cite">
<pre class="moz-signature" cols="80">--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Freeipa-devel mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
Tomas<br>
</blockquote>
<br>
ACK<br>
<br>
<pre class="moz-signature" cols="80">--
Regards,
Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.</pre>
</body>
</html>