[Freeipa-devel] [PATCH] 0059 Create DS user and group during ipa-restore

[Ana Krivokapic]

On 08/30/2013 04:37 PM, Petr Viktorin wrote:
> On 08/29/2013 02:16 PM, Ana Krivokapic wrote:
>> Hello,
>>
>> This patch addresses tickethttps://fedorahosted.org/freeipa/ticket/3856.
>>
>
> Thanks for the patch!
> I haven't tested it yet, but I've read it and I have some comments below.
>
>
>> diff --git a/install/share/copy-schema-to-ca.py
>> b/install/share/copy-schema-to-ca.py
>> index
>> 1888f12513aa3edf22149e9330afea99f62bf41d..fe99a9256f1298bae1c746ea0c4d41339a4fbebb
>> 100755
>> --- a/install/share/copy-schema-to-ca.py
>> +++ b/install/share/copy-schema-to-ca.py
>> @@ -15,10 +15,11 @@
>>   import pwd
>>   import shutil
>>
>> -from ipapython import services, ipautil, dogtag
>> +from ipapython import services, ipautil
>>   from ipapython.ipa_log_manager import root_logger, standard_logging_setup
>> -from ipaserver.install.dsinstance import DS_USER, schema_dirname
>> +from ipaserver.install.dsinstance import schema_dirname
>>   from ipaserver.install.cainstance import PKI_USER
>> +from ipaserver.install.installutils import DS_USER
>>   from ipalib import api
>
> copy-schema-to-ca.py is intended to be copied to older systems, to prepare the
> CA DSs of old IPA masters for replication with new masters with unified DSs.
> This means that we can't remove anything we import here; the constants need to
> be available at the old locations.

Thanks for clearing that up, I didn't know about this requirement.

>
> That's the hard requirement, but anyway I think the constants, and the
> create_ds_user & create_ds_group functions, are specific to the DS and so
> belong in dsinstance.
> Did I overlook a reason for moving them to installutils?

You are right. I refactored these functions so that they can be more easily used
from other modules, but there is no special reason to put them in installutils.
I moved them back to dsinstance.

>
> [...]
>> diff --git a/ipaserver/install/installutils.py
>> b/ipaserver/install/installutils.py
>> index
>> 268279dc9d22b9f983406303cbfc80c00a2b8fa0..84846221d2800443ba6e291ec9c28b37a482d735
>> 100644
> [...]
>> +def create_ds_user():
>> +    """
>> +    Create DS user if it doesn't exist yet
>> +    """
>> +    try:
>> +        pwd.getpwnam(DS_USER)
>> +        root_logger.debug("DS user %s exists" % DS_USER)
>
> Use comma for debug() arguments, i.e. debug("DS group %s exists", DS_GROUP).
> Same in other places.

Fixed.

>
>> +    except KeyError:
>> +        root_logger.debug("Adding DS user %s" % DS_USER)
>> +        args = ["/usr/sbin/useradd", "-g", DS_GROUP,
>> +                                     "-c", "DS System User",
>> +                                     "-d", "/var/lib/dirsrv",
>> +                                     "-s", "/sbin/nologin",
>> +                                     "-M", "-r", DS_USER]
>> +        try:
>> +            ipautil.run(args)
>> +            root_logger.debug("Done adding DS user")
>> +        except ipautil.CalledProcessError, e:
>> +            root_logger.critical("Failed to add DS user %s" % e)
>> +
>> +
>> +def create_ds_group():
>> +    """
>> +    Create DS group if it doesn't exist yet
>> +    """
>
> Please document the return value in the docstring, it's not obvious that this
> returns True iff it didn't do anything.
>

Done.

Updated patch attached.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0059-02-Create-DS-user-and-group-during-ipa-restore.patch
Type: text/x-patch
Size: 9076 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130902/775e9ee1/attachment.bin>