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

[Petr Vobornik]

On 06/09/2015 02:02 PM, Jan Cholasta 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.
>
> Can someone please review the 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.

+1, but no-op if default behavior is "preserve"

>>> Now if the second time the preserve option is present, it makes sense to
>>> not delete it.

+1, should be no-op

>>
>> 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.
>
> I would like an opinion on this as well.
>

So the proposal is, e.g.,:

Replace:
   ipa user del fbar --preserve
   ipa user del fbar --permanently
with:
   ipa user del fbar --permanently=False
   ipa user del fbar --permanently=True
and
   ipa user del fbar
uses the default behavior(permanently atm.)

I don't think there is a big difference. A boolean is easier for 
scripting. 2 options are more descriptive for humans. With a single 
boolean, I would be afraid that omitting it would imply False to some 
users which is not always the same as "the default behavior" [1].

With Web UI developer hat I would vote for single boolean but as a CLI 
user I would like the current options.

Given that Web UI or any other API client should not define CLI, I would 
keep the current options.

my 2c

[1] http://www.freeipa.org/page/V4/User_Life-Cycle_Management#Delete_User
-- 
Petr Vobornik