[Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

[Martin Kosek]

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