On 06/20/2013 12:56 PM, Tomas Babej wrote:
On 06/17/2013 02:34 PM, Ana Krivokapic wrote:
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@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:


All fixed.

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

This is not doable at the moment, we have no unit test framework to test
the trust-add command.

--
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Tomas

I don't know much about AD trusts, but a command-line/API option that takes a 'yes' or 'no' string raised a tiny warning flag for me.

It looks like it's possible that there can be other range types in the future than posix and algorithmic? If that's the case, there should be a --range-type option instead. (If not, I'd still go for --range-type but that would just be bikeshedding.)

In any case I think an explicit 'auto' option would be nice.

But that's just an outsider's view, maybe --use-posix makes more sense.


AFAIK, for CLI changes there should be a a design page; is this covered anywhere?


--
PetrĀ³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to