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
$ ipa user-add tuser --first Test --last User
$ ipa stageuser-add tuser --first Test --last User
$ ipa stageuser-activate tuser
ipa: ERROR: Active user

already exists
Hi David, Jan,

Thanks you so much for all those tests and feedback. I agree, some
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
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
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
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
+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
one boolean parameter rather than two mutually exclusive flags in
I would like an opinion on this as well.

So the proposal is, e.g.,:

    ipa user del fbar --preserve
    ipa user del fbar --permanently
    ipa user del fbar --permanently=False
    ipa user del fbar --permanently=True
    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

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

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.

Jan Cholasta
>From 0a59f9394fc2e8f84142d45b2c588dac19c0c611 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 18 Jun 2015 07:20:27 +0000
Subject: [PATCH] User life cycle: change user-del flags to be CLI-specific

Rename --permanently to --no-preserve.

 API.txt                |  4 ++--
 VERSION                |  4 ++--
 ipalib/plugins/user.py | 37 +++++++++++++++++++++++++++----------
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/API.txt b/API.txt
index c7f02b9..3bcb3bd 100644
--- a/API.txt
+++ b/API.txt
@@ -5155,8 +5155,8 @@ command: user_del
 args: 1,4,3
 arg: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=True, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, query=True, required=True)
 option: Flag('continue', autofill=True, cli_name='continue', default=False)
-option: Flag('permanently?', autofill=True, cli_name='permanently', default=False)
-option: Flag('preserve?', autofill=True, cli_name='preserve', default=False)
+option: Flag('no_preserve?', autofill=True, default=False, include='cli')
+option: Flag('preserve?', autofill=True, default=False, include='cli')
 option: Str('version?', exclude='webui')
 output: Output('result', <type 'dict'>, None)
 output: Output('summary', (<type 'unicode'>, <type 'NoneType'>), None)
diff --git a/VERSION b/VERSION
index f38b494..f8f398d 100644
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
-# Last change: jcholast - User life cycle: provide preserved user virtual attribute
+# Last change: jcholast - User life cycle: Make user-del flags CLI-specific
diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index f4a8a2c..50b7aca 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -565,18 +565,32 @@ class user_del(baseuser_del):
     msg_summary = _('Deleted user "%(value)s"')
     takes_options = baseuser_del.takes_options + (
+        Bool('preserve?',
+            exclude='cli',
+        ),
+            include='cli',
             doc=_('Delete a user, keeping the entry available for future use'),
-            cli_name='preserve',
-            default=False,
-        Flag('permanently?',
+        Flag('no_preserve?',
+            include='cli',
             doc=_('Delete a user'),
-            cli_name='permanently',
-            default=False,
+    def forward(self, *keys, **options):
+        if self.api.env.context == 'cli':
+            if options['no_preserve'] and options['preserve']:
+                raise errors.MutuallyExclusiveError(
+                    reason=_("preserve and no-preserve cannot be both set"))
+            elif options['no_preserve']:
+                options['preserve'] = False
+            elif not options['preserve']:
+                del options['preserve']
+            del options['no_preserve']
+        return super(user_del, self).forward(*keys, **options)
     def pre_callback(self, ldap, dn, *keys, **options):
         assert isinstance(dn, DN)
@@ -606,13 +620,14 @@ class user_del(baseuser_del):
         dn = self.obj.get_dn(*keys, **options)
-        if options['permanently'] or dn.endswith(DN(self.obj.delete_container_dn, api.env.basedn)):
-            # We are going to permanent delete or the user is already in the delete container.
-            # So we issue a true DEL on that entry
-            return super(user_del, self).execute(*keys, **options)
+        if 'preserve' in options:
+            preserve = options['preserve']
+        else:
+            preserve = not dn.endswith(DN(self.obj.delete_container_dn,
+                                          self.api.env.basedn))
         # The user to delete is active and there is no 'permanently' option
-        if options['preserve']:
+        if preserve:
             ldap = self.obj.backend
@@ -662,6 +677,8 @@ class user_del(baseuser_del):
             val = dict(result=dict(failed=[]), value=[keys[-1][0]])
             return val
+            # We are going to permanent delete or the user is already in the delete container.
+            # So we issue a true DEL on that entry
             return super(user_del, self).execute(*keys, **options)

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to