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

[Petr Viktorin]

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 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:
>>
>
> 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 at 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³