[Freeipa-devel] [PATCH] 0005 User life cycle: del/mod/find/show stageuser commands

[Petr Vobornik]

On 06/10/2015 03:49 PM, David Kupka wrote:
> Dne 20.5.2015 v 11:26 Jan Cholasta napsal(a):
>> Dne 18.5.2015 v 10:33 thierry bordaz napsal(a):
>>> On 05/15/2015 04:44 PM, David Kupka wrote:
>>>> Hello Thierry,
>>>> thanks for the patch set. Overall functionality of ULC feature looks
>>>> good to
>>>> me and is definitely "alpha ready".
>>>>
>>>> I found following issues but don't insist on fixing it right now:
>>>>
>>>> 1) When stageuser-activate fails due to already existent
>>>> active/deleted user.
>>>> DN is show instead of user name that's used in other commands
>>>> (user-add,
>>>> stageuser-add).
>>>> $ ipa user-add tuser --first Test --last User
>>>> $ ipa stageuser-add tuser --first Test --last User
>>>> $ ipa stageuser-activate tuser
>>>> ipa: ERROR: Active user
>>>> uid=tuser,cn=users,cn=accounts,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
>>>>
>>>>
>>>>
>>>> already exists
>>>
>>> Hi David, Jan,
>>>
>>> Thanks you so much for all those tests and feedback. I agree, some minor
>>> bugs can be fixed separatly from this main patches.
>>>
>>> You are right, It should return the user ID not the DN.
>>>
>>>>
>>>> 2) According to the design there should be '--only-delete' and
>>>> '--also-delete'
>>>> options for user-find command instead there is '--preserved' option.
>>>> Honza proposed adding virtual boolean attribute 'deleted' to user
>>>> entry and
>>>> filter on it.
>>>> The 'deleted' attribute would be useful also in user-show where is no
>>>> way to
>>>> tell if the displayed user is active or deleted. (Except running with
>>>> --all
>>>> and looking on the dn).
>>>
>>> Yes a bit late to resynch the design.
>>> The final option is 'preserved' for user-find and 'preserve' for
>>> user-del. '--only-delete' or 'also-delete' are old name that I need to
>>> replace in the design.
>>>
>>> About the 'deleted' attribute, do you think adding a DS cos virtual
>>> attribute ?
>>
>> See the attached patch.
>>
>>>
>>>>
>>>> 3) uidNumber and gidNumber can't be set back to '-1' once set to other
>>>> value.
>>>> This would be useful when admin changes its mind and want IPA to
>>>> assign them.
>>>> IIUC, there should be no validation in cn=staged user container. All
>>>> validation should be done during stageuser-activate.
>>>
>>> Yes that comes from user plugin that enforce the number to be >0.
>>> That is a good point giving the ability to reset uidNumber/gidNumber.
>>> I will check if it is possible, how (give a value or an option to
>>> reset), and also if it would not create other issue.
>>>>
>>>> 4) Support for deleted -> stage workflow is still missing. But I'm
>>>> unsure if we
>>>> agreed to finish it now or later.
>>>
>>> Yes thanks
>>>>
>>>> 5) Twice deleting user with '--preserve' deletes him permanently.
>>>> $ ipa user-add tuser --first Test --last User
>>>> $ ipa user-del tuser --preserve
>>>> $ ipa user-del tuser --preserve
>>>> $ ipa user-find --preserved
>>>> ------------------------
>>>> 0 (delete) users matched
>>>> ------------------------
>>>> ----------------------------
>>>> Number of entries returned 0
>>>> ----------------------------
>>>
>>> Deleting a deleted (preserved) entry, should permanently remove the
>>> entry.
>>> Now if the second time the preserve option is present, it makes sense to
>>> not delete it.
>>
>> BTW: I might be stating the obvious here, but it would be better to use
>> one boolean parameter rather than two mutually exclusive flags in
>> user-del.
>>
>>>
>>>
>>> thanks
>>> theirry
>>
>
> Overall, LGTM,
>
> Just 2 nitpicks:
> 1) preserved attribute label: 'Preserved deleted user' -> 'Preserved user'
> 2) 'preserved' attribute should be shown in user-{find,show} when
> '--all' is specified
>
> Updated patch attached.
>

+1, Patch looks good. ACK

rebased and pushed to master: 69607250b9762a6c9b657dd31653b03d54a7b411
-- 
Petr Vobornik