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

[Jan Cholasta]

Dne 18.6.2015 v 09:30 Jan Cholasta napsal(a):
> Dne 15.6.2015 v 17:29 thierry bordaz napsal(a):
>> On 06/15/2015 05:00 PM, Simo Sorce wrote:
>>> On Mon, 2015-06-15 at 16:48 +0200, Petr Vobornik wrote:
>>>> 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
>>>>
>>> +1 --preserve is 100x better for a human than --permanently=False
>>
>> I also prefere --preserve for usability of  'user del'.
>>
>> In addition we have 'user find|show --preserved' to retrieve users that
>> have been preserved. So it seems to me better that the action that
>> preserved the user uses the option '--preserve' rather
>> '--permanently=False'.
>
> It's ridiculous that the CLI taints the RPC API and it should be fixed.
>
> Also on a more nitpicky side, I think the flag should be called
> --no-preserve rather than --permanently. There is plenty of commands
> (rm, cp, ...) which have --no-preserve as opposite of --preserve.
>
> The attached patch fixes both.

... and it also accidentaly changes the default behavior.

Updated patch attached.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-446.1-User-life-cycle-change-user-del-flags-to-be-CLI-spec.patch
Type: text/x-patch
Size: 4196 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150618/96ba0a30/attachment.bin>