Re: [PATCH] anaconda realm command

[Vratislav Podzimek]

> From 5a18b92ad5e4e99c447edd29440bb0fb687b70e4 Mon Sep 17 00:00:00 2001
> From: Stef Walter 
> Date: Fri, 16 Nov 2012 11:28:43 +0100
> Subject: [PATCH 2/2] Add support for the realm kickstart command
>
> This command uses realmd to discover information about the domain
> to join, which adds in the right packages, and then joins the domain
> post install.
>
> anaconda.spec.in         |  1 +
>  pyanaconda/install.py    | 11 +++++++-
>  pyanaconda/kickstart.py  | 70 ++++++++++++++++++++++++++++++++++++++++++++++++
>  pyanaconda/yuminstall.py |  2 +-
>  4 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/anaconda.spec.in b/anaconda.spec.in
> index 0b5ef39..c2d8333 100644
> --- a/anaconda.spec.in
> +++ b/anaconda.spec.in
> @@ -122,6 +122,7 @@ Requires: python-pyblock >= %{pythonpyblockver}
>  Requires: libuser-python
>  Requires: newt-python
>  Requires: authconfig
> +Requires: realmd
>  Requires: firewalld >= %{firewalldver}
>  Requires: cryptsetup-luks
>  Requires: python-cryptsetup >= %{pythoncryptsetupver}
> diff --git a/pyanaconda/install.py b/pyanaconda/install.py
> index b6fc3e8..bdd7090 100644
> --- a/pyanaconda/install.py
> +++ b/pyanaconda/install.py
> @@ -78,6 +78,10 @@ def doConfiguration(storage, payload, ksdata, instClass):
>          ksdata.group.execute(storage, ksdata, instClass, u)
>          ksdata.user.execute(storage, ksdata, instClass, u)
> if ksdata.realm.discovered:
>    with progress_report(_("Joining realm: %s" % ksdata.realm.discovered)):
>        ksdata.realm.execute(storage, ksdata, instClass)

You need to increment the number in progress.send_init() at the
begining of doConfiguration if there is one more step. Otherwise the
progress bar will be one step ahead.
>  with progress_report(_("Running post install scripts")):
>      runPostScripts(ksdata.scripts)
>
> @@ -116,10 +120,15 @@ def doInstall(storage, payload, ksdata, instClass):
>  # Do packaging.
>
> # Discover information about realms to join, to determine additional packages
> if ksdata.realm.join_realm:
>    with progress_report(_("Discovering realm to join")):
>        ksdata.realm.discover()

The same goes also for progress.send_init() in doInstall.
>  # anaconda requires storage packages in order to make sure the target
>  # system is bootable and configurable, and some other packages in order
>  # to finish setting up the system.
> packages = storage.packages + ["authconfig", "firewalld"]
> packages = storage.packages + ["authconfig", "firewalld"] + ksdata.realm.packages
> payload.preInstall(packages=packages, groups=payload.languageGroups(ksdata.lang.lang))
> payload.install()
>
> diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py
> index 2600c65..e501947 100644
> --- a/pyanaconda/kickstart.py
> +++ b/pyanaconda/kickstart.py
> @@ -1262,6 +1262,75 @@ class RaidData(commands.raid.F18_RaidData):
>                                       parents=request)
>              storage.createDevice(luksdev)
>
> +class Realm(commands.realm.F19_Realm):
> def __init__(self, *args):
>    commands.realm.F19_Realm.__init__(self, *args)
>    self.packages = []
>    self.discovered = ""
> def discover(self):
>    if not self.join_realm:
>        return
>    try:
>        args = ["discover", "--verbose", "--install", "/"] + \
>               self.discover_options + [self.join_realm]
>        output = iutil.execWithCapture("/usr/sbin/realm", args, stderr="/dev/tty5", fatal=True)

Could you please split this line to two? The same goes for some others
longer than let's say 90 characters (80 ideally).
>    except RuntimeError as msg:
>        log.error("Error running /usr/sbin/realm %s: %s", args, msg)
>        return
>    except OSError as msg:
>        # TODO: This is a lousy way of propagating what will usually be 'no such realm'
>        log.error("Error running /usr/sbin/realm %s: %s", args, msg)
>        return
>    # Now parse the output for the required software. First line is the
>    # realm name, and following lines are information as "name: value"
>    self.packages = ["realmd"]
>    self.discovered = ""
>    for line in output.split("\n"):
>        if not self.discovered:
>            self.discovered = line.strip()
>        else:
>            parts = line.split (":", 1)
>            if len(parts) == 2 and parts[0].strip() == "required-package":
>                self.packages.append(parts[1].strip())

Are you expecting some empty lines at the begining of the output?
Because otherwise I believe something like:

lines = output.split("\n")
        if not lines:
            return
        	self.discovered = lines.pop(0).strip()

for line in lines:
            parts = line.split(":", 1)
            if len(parts) == 2 and parts[0].strip() == "required-package":
                self.packagse.append(parts[1].strip())

would do the same and could be better readable. This is probably just a
matter of opinion. If you are going to use your version, please remove
the additional space in 'parts = line.split (":", 1)'
>    log.info("Realm %s needs packages %s" % (self.discovered, ", ".join(self.packages)))
> def execute(self, *args):
>    if not self.discovered:
>        return
>    pw_args = ["--no-password"]
>    for arg in self.join_args:
>        if arg.startswith("--no-password") or arg.startswith("--one-time-password"):
>            pw_args = []

'break' should go here? Or, if you want, you could use:

if any(arg.startswith("--no-password")
               or arg.startswith("--one-time-password")
               for arg in self.join_args):
             pw_args = []
        else:
             pw_args = ["--no-password"]
>    args = ["join", "--install", ROOT_PATH, "--verbose"] + pw_args + self.join_args
>    try:
>        rc = iutil.execWithRedirect("/usr/sbin/realm", args,
>                                    stdout="/dev/tty5", stderr="/dev/tty5")
>    except RuntimeError as msg:
>        log.error("Error running /usr/sbin/realm %s: %s", args, msg)
>    if rc != 0:
>        log.error("Command failure: /usr/sbin/realm %s: %d", args, rc)
>        return
>    log.info("Joined realm %s", self.join_realm)
>    for (command, options) in self.after:
>        args = [command, "--install", ROOT_PATH, "--verbose"] + options
>        rc = iutil.execWithRedirect("/usr/sbin/realm", args,
>                                    stdout="/dev/tty5", stderr="/dev/tty5")
>        if rc != 0:
>            log.error("Command failure: /usr/sbin/realm %s: %d", args, rc)
>        log.info("Ran /usr/sbin/realm %s", args)
>
> class RootPw(commands.rootpw.F18_RootPw):
>      def execute(self, storage, ksdata, instClass, users):
>          algo = getPassAlgo(ksdata.authconfig.authconfig)
> @@ -1443,6 +1512,7 @@ commandMap = {
>          "part": Partition,
>          "partition": Partition,
>          "raid": Raid,
>    "realm": Realm,
>    "rootpw": RootPw,
>    "selinux": SELinux,
>    "services": Services,
>
> diff --git a/pyanaconda/yuminstall.py b/pyanaconda/yuminstall.py
> index aef7e8f..f8eb609 100644
> --- a/pyanaconda/yuminstall.py
> +++ b/pyanaconda/yuminstall.py
> @@ -1488,7 +1488,7 @@ reposdir=/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anacon
>      # installed (they could have been removed in kickstart).  So we'll force
>      # it.
>      def selectAnacondaNeeds(self):
>    for pkg in ['authconfig', 'chkconfig', 'system-config-firewall-base']:
>    for pkg in ['authconfig', 'chkconfig', 'realmd', 'system-config-firewall-base']:

Do we really want 'realmd' installed on every system?

Otherwise it looks good to me but as I've already mentioned in the
reply for the pykickstart patches I believe this should be a separate
addon and not part of the Anaconda itself.