[Freeipa-devel] [PATCH] 0003-2 User life cycle: new stageuser plugin with add verb

[thierry bordaz]

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 at 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 at 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




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150219/5699bb72/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-User-Life-Cycle-Exclude-subtree-for-ipaUniqueID-gene.patch
Type: text/x-patch
Size: 2588 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150219/5699bb72/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-User-life-cycle-stageuser-add-verb.patch
Type: text/x-patch
Size: 90093 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150219/5699bb72/attachment-0001.bin>