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

[Tomas Babej]

On Monday 24 of June 2013 16:22:01 Petr Viktorin wrote:
> 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.

I replaced --use-posix with more versatile --range-type. It supports only
trust-related values for now, and can be extended.

Rebased patch attached.

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

Design page for the umbrella ticket is here:

http://www.freeipa.org/page/V3/Use_posix_attributes_defined_in_AD

> 
> -- 
> Petr³
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130710/a4bf39ab/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0067-3-Add-range-type-option-that-forces-range-type-of-the-.patch
Type: text/x-patch
Size: 7023 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130710/a4bf39ab/attachment.bin>