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

Thu Nov 14 14:30:22 UTC 2013

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.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0068-05-Add-automember-rebuild-command.patch
Type: text/x-patch
Size: 8288 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131114/f523dc0e/attachment.bin>