<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/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 class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a 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>
  </body>
</html>