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

[Simo Sorce]

On Thu, 2015-06-18 at 09:30 +0200, Jan Cholasta wrote:
> 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.

Permanently sounds a little bit better said out loud, but no-preserve
conveys better the idea that this option is the opposite of the other
one.
So this works for me.

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York