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

[Petr Viktorin]

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.

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?

[...]
> 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.

> +    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.

-- 
Petr³