[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: OSTree <3 Anaconda



Just a couple things stood out to me from doing a quick review...

> @@ -2361,6 +2368,26 @@ def writeSysconfigKernel(storage, version):
>          f.write("HYPERVISOR_ARGS=logging=vga,serial,memory\n")
>      f.close()
>  
> +def writeBootLoaderFinal(storage, payload, instClass, ksdata):
> +    """ Do the final write of the bootloader. """
> +
> +    from pyanaconda.errors import errorHandler, ERROR_RAISE
> +
> +    # set up dracut/fips boot args
> +    # XXX FIXME: do this from elsewhere?
> +    #storage.bootloader.set_boot_args(keyboard=anaconda.keyboard,
> +    #                                 storage=anaconda.storage,
> +    #                                 language=anaconda.instLanguage,
> +    #                                 network=anaconda.network)
> +    storage.bootloader.set_boot_args(storage=storage,
> +                                     payload=payload,
> +                                     keyboard=ksdata.keyboard)
> +    try:
> +        storage.bootloader.write()
> +    except BootLoaderError as e:
> +        if errorHandler.cb(e) == ERROR_RAISE:
> +            raise
> +
>  def writeBootLoader(storage, payload, instClass, ksdata):
>      """ Write bootloader configuration to disk.
>  
> @@ -2376,6 +2403,13 @@ def writeBootLoader(storage, payload, instClass, ksdata):
>          stage2_device = storage.bootloader.stage2_device
>          log.info("bootloader stage2 target device is %s" % stage2_device.name)
>  
> +    if payload.handlesBootloaderConfiguration:
> +        if storage.bootloader.skip_bootloader:
> +            log.info("skipping bootloader install per user request")
> +            return
> +        writeBootLoaderFinal(storage, payload, instClass, ksdata)
> +        return
> +
>      # get a list of installed kernel packages
>      kernel_versions = payload.kernelVersionList
>      if not kernel_versions:
> @@ -2420,19 +2454,4 @@ def writeBootLoader(storage, payload, instClass, ksdata):
>                                           label=label, short=short)
>          storage.bootloader.add_image(image)
>  
> -    # set up dracut/fips boot args
> -    # XXX FIXME: do this from elsewhere?
> -    #storage.bootloader.set_boot_args(keyboard=anaconda.keyboard,
> -    #                                 storage=anaconda.storage,
> -    #                                 language=anaconda.instLanguage,
> -    #                                 network=anaconda.network)
> -    storage.bootloader.set_boot_args(storage=storage,
> -                                     payload=payload,
> -                                     keyboard=ksdata.keyboard)
> -
> -    try:
> -        storage.bootloader.write()
> -    except BootLoaderError as e:
> -        if errorHandler.cb(e) == ERROR_RAISE:
> -            raise
> -
> +    writeBootLoaderFinal(storage, payload, instClass, ksdata)

Vratislav changed the set_boot_args calls a little recently to remove
passing keyboard, so this won't apply cleanly.

> diff --git a/pyanaconda/kickstart.py b/pyanaconda/kickstart.py
> index df7b5e6..c788074 100644
> --- a/pyanaconda/kickstart.py
> +++ b/pyanaconda/kickstart.py
> @@ -490,6 +490,13 @@ class Realm(commands.realm.F19_Realm):
>  
>          log.info("Joined realm %s", self.join_realm)
>  
> +class OSTreeSetup(commands.ostreesetup.F20_OSTreeSetup):
> +    def __init__(self, *args):
> +        commands.ostreesetup.F20_OSTreeSetup.__init__(self, *args)
> +
> +    def execute(self, *args):
> +        # This is implemented in packaging/ostreepayload.py
> +        pass
>  
>  class ClearPart(commands.clearpart.F17_ClearPart):
>      def parse(self, args):
> @@ -1580,6 +1588,7 @@ commandMap = {
>          "logvol": LogVol,
>          "multipath": MultiPath,
>          "network": Network,
> +        "ostreesetup": OSTreeSetup,
>          "part": Partition,
>          "partition": Partition,
>          "raid": Raid,

If you're not adding anaconda-specific behavior for a kickstart command,
you don't need to do either of these chunks.

> diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
> index 824c8a4..7b863b5 100644
> --- a/pyanaconda/packaging/yumpayload.py
> +++ b/pyanaconda/packaging/yumpayload.py
> @@ -1273,6 +1273,10 @@ reposdir=%s
>      ### METHODS FOR WORKING WITH PACKAGES
>      ###
>      @property
> +    def supportsPackages(self):
> +        return True
> +
> +    @property
>      def packages(self):
>          from yum.Errors import RepoError

dnfpayload will also support packages.

> diff --git a/pyanaconda/ui/gui/hubs/__init__.py b/pyanaconda/ui/gui/hubs/__init__.py
> index f8b016e..e739f1d 100644
> --- a/pyanaconda/ui/gui/hubs/__init__.py
> +++ b/pyanaconda/ui/gui/hubs/__init__.py
> @@ -287,7 +287,17 @@ class Hub(GUIObject, common.Hub):
>  
>      @property
>      def continuePossible(self):
> -        return len(self._incompleteSpokes) == 0 and len(self._notReadySpokes) == 0 and getattr(self._checker, "success", True)
> +        if len(self._incompleteSpokes) > 0:
> +            log.info("Incomplete spokes: %r" % (self._incompleteSpokes, ))
> +            return False
> +        if len(self._notReadySpokes) > 0:
> +            log.info("NotReady spokes: %r" % (self._notReadySpokes, ))
> +            return False
> +        checkSuccess=getattr(self._checker, "success", True)
> +        if not checkSuccess:
> +            log.info("Checker returned %r" % (checkSuccess, ))
> +            return False
> +        return True
>          
>      def _updateContinueButton(self):
>          self.continueButton.set_sensitive(self.continuePossible)

Is this chunk just for debugging?

> diff --git a/pyanaconda/ui/gui/spokes/software.py b/pyanaconda/ui/gui/spokes/software.py
> index b21866b..46e7ede 100644
> --- a/pyanaconda/ui/gui/spokes/software.py
> +++ b/pyanaconda/ui/gui/spokes/software.py
> @@ -164,7 +164,9 @@ class SoftwareSelectionSpoke(NormalSpoke):
>  
>      @property
>      def showable(self):
> -        return not flags.livecdInstall and not self.data.method.method == "liveimg"
> +        return (self.payload.supportsPackages
> +                and not flags.livecdInstall
> +                and not self.data.method.method == "liveimg")
>  
>      @property
>      def status(self):
> diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py
> index 59ff82c..add7fbc 100644
> --- a/pyanaconda/ui/gui/spokes/source.py
> +++ b/pyanaconda/ui/gui/spokes/source.py
> @@ -761,7 +761,9 @@ class SourceSpoke(NormalSpoke):
>  
>      @property
>      def showable(self):
> -        return not flags.livecdInstall and not self.data.method.method == "liveimg"
> +        return (self.payload.supportsPackages
> +                and not flags.livecdInstall
> +                and not self.data.method.method == "liveimg")
>  
>      def _mirror_active(self):
>          return self._protocolComboBox.get_active() == PROTOCOL_MIRROR

"not flags.livecdInstall and not self.data.method.method == 'liveimg'"
is basically the same as saying self.payload.supportsPackages.  It's
certainly what we were going for there.  So you might as well just
completely replace the existing tests with your new one.

> >From cad071d736cb35e67245ca11cf8750931c0d98ba Mon Sep 17 00:00:00 2001
> From: Colin Walters <walters verbum org>
> Date: Tue, 11 Mar 2014 09:33:36 -0400
> Subject: [PATCH] ostreesetup: New command
> 
> This tells the installer to handle an OSTree repository.
> ---
>  pykickstart/commands/__init__.py    |  3 +-
>  pykickstart/commands/ostreesetup.py | 74 +++++++++++++++++++++++++++++++++++++
>  pykickstart/handlers/control.py     |  2 +
>  tests/commands/ostreesetup.py       | 37 +++++++++++++++++++
>  4 files changed, 115 insertions(+), 1 deletion(-)
>  create mode 100644 pykickstart/commands/ostreesetup.py
>  create mode 100644 tests/commands/ostreesetup.py
> 
> diff --git a/pykickstart/commands/__init__.py b/pykickstart/commands/__init__.py
> index c5145ba..ed4d649 100644
> --- a/pykickstart/commands/__init__.py
> +++ b/pykickstart/commands/__init__.py
> @@ -21,6 +21,7 @@ import authconfig, autopart, autostep, bootloader, btrfs, clearpart, cdrom, devi
>  import deviceprobe, displaymode, dmraid, driverdisk, eula, fcoe, firewall, firstboot
>  import group, harddrive, ignoredisk, interactive, iscsi, iscsiname, key, keyboard, lang
>  import langsupport, lilocheck, liveimg, logging, logvol, mediacheck, method, monitor
> -import mouse, multipath, network, nfs, partition, raid, realm, reboot, repo, rescue
> +import mouse, multipath, network, nfs, ostreesetup
> +import partition, raid, realm, reboot, repo, rescue
>  import rootpw, selinux, services, skipx, sshpw, timezone, updates, upgrade, url, user
>  import unsupported_hardware, vnc, volgroup, xconfig, zerombr, zfcp

I changed this file to make pylint happier, so this chunk won't apply
cleanly.  Look at pykickstart in git and you'll see the obvious thing to
change.  It's not hard.

> diff --git a/pykickstart/commands/ostreesetup.py b/pykickstart/commands/ostreesetup.py
> new file mode 100644
> index 0000000..b722c2c
> --- /dev/null
> +++ b/pykickstart/commands/ostreesetup.py
> @@ -0,0 +1,74 @@
> +#
> +# Copyright (C) 2014  Red Hat, Inc.
> +#
> +# This copyrighted material is made available to anyone wishing to use,
> +# modify, copy, or redistribute it subject to the terms and conditions of
> +# the GNU General Public License v.2, or (at your option) any later version.
> +# This program is distributed in the hope that it will be useful, but WITHOUT
> +# ANY WARRANTY expressed or implied, including the implied warranties of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General
> +# Public License for more details.  You should have received a copy of the
> +# GNU General Public License along with this program; if not, write to the
> +# Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +# 02110-1301, USA.  Any Red Hat trademarks that are incorporated in the
> +# source code or documentation are not subject to the GNU General Public
> +# License and may only be used or replicated with the express permission of
> +# Red Hat, Inc.
> +#
> +
> +from pykickstart.base import *
> +from pykickstart.errors import *
> +from pykickstart.options import *

I've converted pykickstart away from doing global imports, so you'll
want to make changes here too.  The changes will look like this:

   from pykickstart.base import KickstartCommand
   from pykickstart.options import KSOptionParser

You're not using anything out of pykickstart.errors.

> +
> +import gettext
> +_ = lambda x: gettext.ldgettext("pykickstart", x)
> +
> +class F20_OSTreeSetup(KickstartCommand):
> +    removedKeywords = KickstartCommand.removedKeywords
> +    removedAttrs = KickstartCommand.removedAttrs
> +
> +    def __init__(self, *args, **kwargs):
> +        KickstartCommand.__init__(self, *args, **kwargs)
> +        self.op = self._getParser()
> +        self.osname = kwargs.get('osname', None)
> +        self.remote = kwargs.get("remote", self.osname)
> +        self.url = kwargs.get('url', None)
> +        self.ref = kwargs.get('ref', None)
> +        self.noGpg = kwargs.get('noGpg', True)
> +
> +    def __str__(self):
> +        retval = KickstartCommand.__str__(self)
> +
> +        if self.osname:
> +            retval += "# OSTree setup\n"
> +            retval += "ostreesetup %s\n" % self._getArgsAsStr()
> +
> +        return retval
> +
> +    def _getArgsAsStr(self):
> +        retval = ""
> +        if self.osname:
> +            retval += "--osname=%s" % self.osname
> +        if self.remote:
> +            retval += "--remote=%s" % self.remote
> +        if self.url:
> +            retval += "--url=%s" % self.url
> +        if self.ref:
> +            retval += "--ref=%s" % self.ref
> +        if self.noGpg:
> +            retval += "--nogpg"
> +        return retval

You will want to put a space in front of the -- in all of these, or
using multiple options will result in a string that looks like:

   ostreesetup --osname=blah--url=http://blah--nogpg

You may wish to add a test case that uses multiple arguments.

Also... can osname, remote, or ref have a space in it?  If so, you'll
want to surround the %s with quotes.

- Chris


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]