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

[Martin Kosek]

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.

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.

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

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.

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.

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.

Martin