[Freeipa-devel] [PATCH 0067] Add --use-posix option that forces trusted range type

[Ana Krivokapic]

On 06/06/2013 11:10 AM, Tomas Babej wrote:
> Hi,
>
> Adds --use-posix option to ipa trust-add command. It takes two
> allowed values:
> 'yes' : the 'ipa-ad-trust-posix' range type is enforced
> 'no' : the 'ipa-ad-trust' range type is enforced
>
> When --use-posix option is not specified, the range type should be
> determined by ID range discovery.
>
> https://fedorahosted.org/freeipa/ticket/3650
>
> Tomas
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

The patch works nicely, but I have a few comments:

1) You added a new option to the API, but you forgot to bump the
IPA_API_VERSION_MINOR in the VERSION file.

2) Typo in commit message: "shold" instead of "should".

3) This construct:

+            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:


4) Some unit tests to cover the behavior of the newly added option would be nice.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130617/51b20e1b/attachment.htm>