On 11/14/2013 03:30 PM, Ana Krivokapic wrote: > On 11/14/2013 03:11 PM, Martin Kosek wrote: >> On 11/13/2013 04:56 PM, Ana Krivokapic wrote: >>> On 11/13/2013 03:08 PM, Martin Kosek wrote: >>>> On 10/29/2013 12:30 PM, Ana Krivokapic wrote: >>>>> On 10/15/2013 06:09 PM, Ana Krivokapic wrote: >>>>>> On 09/30/2013 10:02 AM, Petr Viktorin wrote: >>>>>>> On 09/27/2013 03:12 PM, Martin Kosek wrote: >>>>>>>> On 09/27/2013 03:00 PM, Jan Cholasta wrote: >>>>>>>>> On 23.9.2013 19:41, Ana Krivokapic wrote: >>>>>>>>>> On 09/19/2013 03:29 PM, Ana Krivokapic wrote: >>>>>>>> ... >>>>>>>>> Patch 69: >>>>>>>>> >>>>>>>>> I think the changes in the update file should be also done in the >>>>>>>>> right LDIF >>>>>>>>> files in install/share, though I don't know what is the recent >>>>>>>>> consensus on this. >>>>>>>>> >>>>>>>>> >>>>>>>>> Honza >>>>>>>>> >>>>>>>> Last time I checked, we used to do the change both in LDIF and update >>>>>>>> file. Just to avoid the LDIF become obsolete. >>>>>>>> >>>>>>>> Martin >>>>>>> Rob recently said his preference is to move everything from LDIF to >>>>>>> updates, >>>>>>> and out of the the LDIF files: >>>>>>> http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html >>>>>>> >>>>>>> I would agree, having two places with the same information is redundant >>>>>>> and >>>>>>> error-prone. >>>>>>> >>>>>> Thanks Honza for the review. >>>>>> >>>>>> I incorporated your suggestions in this updated patchset. I attached all >>>>>> the >>>>>> patches for more convenient reviewing, but only patches 68 and 70 have >>>>>> changed. >>>>>> >>>>>> I haven't done any changes in the LDIF files since the consensus seems >>>>>> to be not >>>>>> to do that. >>>>> Patch 70 needed a rebase, attaching the whole patchset again. >>>> This works pretty fine, I have few comments though: >>>> >>>> 1) 0068: the task should be run only for users/hosts base DN - this is >>>> where we >>>> confine our automember and I think admin may be surprised that the rebuild >>>> call >>>> is does not respect it. >>> Fixed. >>> >>>> 2) 0068: I am missing some examples for automember-rebuild in the help. At >>>> least for running rebuild for all users/hosts and for running it for >>>> specified >>>> user/host. >>> I added some examples, as well as a general description of the new command. >>> >>>> 3) 0068: I think that the labels/doc for the new command/options should be >>>> improved. It is not obvious, that automember-rebuild can run for all >>>> users/hosts, at least from following doc: >>>> >>>> # ipa help automember >>>> ... >>>> automember-rebuild Rebuild auto membership for specified >>>> entries. >>>> ... >>>> >>>> Maybe we should remove the "for specified entries" part? >>>> >>>> As for the options, we now have this: >>>> >>>> # ipa help automember-rebuild >>>> Usage: ipa [global-options] automember-rebuild [options] >>>> >>>> Rebuild auto membership for specified entries. >>>> Options: >>>> -h, --help show this help message and exit >>>> --type=['group', 'hostgroup'] >>>> Grouping to which the rule applies <--completely >>>> stray >>>> --users=STR Users for which the rebuild task will be run >>>> --hosts=STR Hosts for which the rebuild task will be run >>>> >>>> >>>> We should probably also do not mention specified entries here. >>>> >>>> As for option help, maybe the following would better show that it can be >>>> run >>>> for all entries? >>>> >>>> --type=['group', 'hostgroup'] >>>> Rebuild membership for all members of a grouping >>>> --users=STR Rebuild membership for specified users >>>> --hosts=STR Rebuild membership for specified hosts >>> Agreed, labels fixed as per your suggestions. >>> >>>> This makes me thinking we may want to forbid entering both --type and >>>> --users/--hosts - i.e. either rebuild all or just selected ones - to make >>>> the >>>> selection even more clear. But I am open to discussion on this one. >>> Validation prevents any invalid combination of options (e.g. --type=group >>> and >>> --hosts used together, or --type=hostgroup and --users used together). If, >>> for >>> example, --users is specified, then --type=group is allowed but not >>> required. I >>> think it's clear enough. >>> >>>> 4) 0069: Add Automember Export Updates Task is currently redundant. I >>>> think we >>>> should either have permissions for all 3 possible tasks or for just the >>>> one we use. >>> I removed the unused permission. >>> >>>> 5) 0069: permissions should be of SYSTEM type as the ACI is out of SUFFIX, >>>> so >>>> that user does not try to modify them (will be able to in future versions). >>>> Adding Petr3 to CC for heads up on this one. >>> Fixed. >>> >>>> Martin >>> Thanks for the review, the updated patchset is attached. >> Looks good. Last thing I would like to get fixed is this part in 0068: >> >> + types = { >> + 'group': ('user', 'users', 'users'), >> + 'hostgroup': ('host', 'hosts', 'computers'), >> + } >> + >> + obj_name, opt_name, dn_part = types[gtype] >> + basedn = DN(('cn', dn_part),('cn', 'accounts'), api.env.basedn) >> + obj = self.api.Object[obj_name] >> >> I know it works now, but if we sometime decide to maybe support different >> user >> containers and not have it in cn=users,cn=accounts, it will help not user >> hardcoded DNs like that, but rather standard >> >> DN(api.env.container_users, api.env.basedn) >> or >> DN(api.env.container_hosts, api.env.basedn) >> >> Martin > > Fixed, updated patch attached. >
Thanks, ACK! Pushed to master. Martin _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
