Dne 25.3.2015 v 10:04 thierry bordaz napsal(a):
On 03/24/2015 01:45 PM, Jan Cholasta wrote:
Dne 19.3.2015 v 13:07 thierry bordaz napsal(a):
On 03/19/2015 07:37 AM, Jan Cholasta wrote:
Dne 18.3.2015 v 19:39 thierry bordaz napsal(a):
On 03/17/2015 08:01 AM, Jan Cholasta wrote:
Dne 16.3.2015 v 12:06 David Kupka napsal(a):
On 03/06/2015 07:30 PM, thierry bordaz wrote:
On 02/19/2015 04:19 PM, Martin Basti wrote:
On 19/02/15 13:01, thierry bordaz wrote:
On 02/04/2015 05:14 PM, Jan Cholasta wrote:
Hi,

Dne 4.2.2015 v 15:25 David Kupka napsal(a):
On 02/03/2015 11:50 AM, thierry bordaz wrote:
On 09/17/2014 12:32 PM, thierry bordaz wrote:
On 09/01/2014 01:08 PM, Petr Viktorin wrote:
On 08/08/2014 03:54 PM, thierry bordaz wrote:
Hi,

The attached patch is related to 'User Life Cycle'
(https://fedorahosted.org/freeipa/ticket/3813)

It creates a stageuser plugin with a first function
stageuser-add.
Stage
user entries are provisioned under 'cn=staged
users,cn=accounts,cn=provisioning,SUFFIX'.

Thanks
thierry

Avoid `from ipalib.plugins.baseldap import *` in new code;
instead
import the module itself and use e.g. `baseldap.LDAPObject`.

The stageuser help (docstring) is copied from the user
plugin, and
discusses things like account lockout and disabling
users. It
should
rather explain what stageuser itself does. (And I don't very
much
like the Note about the interface being badly designed...)
Also decide if the docs should call it "staged user" or
"stage
user"
or "stageuser".

A lot of the code is copied and pasted over from the users
plugin.
Don't do that. Either import things (e.g.
validate_nsaccountlock)
from the users plugin, or move the reused code into a shared
module.

For the `user` object, since so much is the same, it
might be
best to
create a common base class for user and stageuser; and
similarly
for
the Command plugins.

The default permissions need different names, and you don't
need
another copy of the 'non_object' ones. Also, run the makeaci
script.

Hello,

    This modified patch is mainly moving common base class
into a
new
    plugin: accounts.py. user/stageuser plugin inherits from
accounts.
    It also creates a better description of what are stage
user,
how
    to add a new stage user, updates ACI.txt and separate
active/stage
    user managed permissions.

thanks
thierry






_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Thanks David for the reviews. Here the last patches




_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


The freeipa-tbordaz-0002 patch had trailing whitespaces on few
lines so
I'm attaching fixed version (and unchanged patch
freeipa-tbordaz-0003-3
to keep them together).

The ULC feature is still WIP but these patches look good to me
and
don't
break anything as far as I tested.
We should push them now to avoid further rebases. Thierry can
then
prepare other patches delivering the rest of ULC functionality.

Few comments from just reading the patches:

1) I would name the base class "baseuser", "account" does not
necessarily mean user account.

2) This is very wrong:

-class user_add(LDAPCreate):
+class user_add(user, LDAPCreate):

You are creating a plugin which is both an object and an
command.

3) This is purely subjective, but I don't like the name
"deleteuser", as it has a verb in it. We usually don't do
that and
IMHO we shouldn't do that.

Honza


Thank you for the review. I am attaching the updates patches






_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Hello,
I'm getting errors during make rpms:

if [ "" != "yes" ]; then \
    ./makeapi --validate; \
    ./makeaci --validate; \
fi

/root/freeipa/ipalib/plugins/baseuser.py:641 command
"baseuser_add"
doc is not internationalized
/root/freeipa/ipalib/plugins/baseuser.py:653 command
"baseuser_find"
doc is not internationalized
/root/freeipa/ipalib/plugins/baseuser.py:647 command
"baseuser_mod"
doc is not internationalized
0 commands without doc, 3 commands whose doc is not i18n
Command baseuser_add in ipalib, not in API
Command baseuser_find in ipalib, not in API
Command baseuser_mod in ipalib, not in API

There are one or more new commands defined.
Update API.txt and increment the minor version in VERSION.

There are one or more documentation problems.
You must fix these before preceeding

Issues probably caused by this:
1)
You should not use the register decorator, if this class is
just for
inheritance
@register()
class baseuser_add(LDAPCreate):

@register()
class baseuser_mod(LDAPUpdate):

@register()
class baseuser_find(LDAPSearch):

see dns.py plugin and "DNSZoneBase" and "dnszone" classes

2)
there might be an issue with
@register()
class baseuser(LDAPObject):

the register decorator should not be there, I was warned by
Petr^3 to
not use permission in parent class. The same permission should be
specified only in one place (for example user class), (otherwise
they
will be generated twice??) I don't know more details about it.

--
Martin Basti

Hello Martin, Jan,

Thanks for your review.
I changed the patch so that it does not register baseuser_*. Also
increase the minor version because of new command.
Finally I moved the managed_permission definition out of the parent
baseuser class.






Martin, could you please verify that the issues you encountered are
fixed?

Thanks!


You bumped wrong version variable:

-IPA_VERSION_MINOR=1
+IPA_VERSION_MINOR=2

It should have been IPA_API_VERSION_MINOR (at the bottom of the
file),
including the last change comment below it.


IMO baseuser should include superclasses for all the usual commands
(add, mod, del, show, find) and stageuser/deleteuser commands should
inherit from them.


You don't need to override class properties like active_container_dn
and takes_params on baseuser subclasses when they have the same value
as in baseuser.


Honza

Hello Honza,

    Thanks for the review. I did the modifications you recommended
    within that attached patches

      * Change version

Please also update the comment below (e.g. "# Last change: tbordaz -
Add stageuser_add command")

      * create the baseuser_* plugins commands and use them in the
        user/stageuser plugin commands
      * Do not redefine the class properties in the subclasses.

There are still some in baseuser command classes:

+class baseuser_add(LDAPCreate):
+    """
+    Prototype command plugin to be implemented by real plugin
+    """
+    active_container_dn       = api.env.container_user
+    has_output_params = LDAPCreate.has_output_params

You don't need to set active_container_dn here, you only need to set
it in baseuser. Then in stageuser_add and other subclasses you use
"self.obj.active_container_dn" instead of "self.active_container_dn".

You also don't need to override has_output_params if you are not
changing its value - you are inheriting from LDAPCreate, so
baseuser_add.has_output_params implicitly has the same value as
LDAPCreate.has_output_params.


    Thanks
    thierry



Hello Honza,

    Thanks for your patience .. :-)
    I understand my mistake. Just a question, in a plugin command
    (user_add), is 'self.obj' referring to the plugin object (like
'user') ?

Yes, that's correct.


    updated patches (with the appropriate naming and patch versioning).

    thanks
    theirry


One more thing:

Instead of:

class stageuser(baseuser):
    ...
    # take_params does not support 'nsaccountlock' option
    stageuser_takes_params_list = []
    for elt in baseuser.takes_params:
        if isinstance(elt, Bool) and elt.param_spec == 'nsaccountlock?':
            pass
        else:
            stageuser_takes_params_list.append(elt)
    takes_params              = tuple(stageuser_takes_params_list)

I would remove nsaccountlock from baseuser.takes_params and add it in
user.takes_params:

class user(baseuser):
    ...
    takes_params = baseuser.takes_params + (
        Bool('nsaccountlock?',
            label=_('Account disabled'),
            flags=['no_option'],
        ),
    )


Right, making this option specific to active user makes sense.

Thanks
thierry

Thanks, ACK from me.

--
Jan Cholasta

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

Reply via email to