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

Wed Nov 13 15:56:58 UTC 2013

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.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0069-03-Add-a-privilege-and-a-permission-needed-for-automemb.patch
Type: text/x-patch
Size: 1879 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131113/5f4025b7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0068-04-Add-automember-rebuild-command.patch
Type: text/x-patch
Size: 8164 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131113/5f4025b7/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0071-Fix-error-message-when-adding-duplicate-automember-r.patch
Type: text/x-patch
Size: 3819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131113/5f4025b7/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0070-04-Add-unit-tests-for-automember-rebuild-command.patch
Type: text/x-patch
Size: 21152 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131113/5f4025b7/attachment-0003.bin>