[PATCH 1/2] Don't crash if incorrect environment is set in kickstart (#1234890)

Martin Kolman mkolman at redhat.com
Tue Jun 30 11:07:24 UTC 2015


On Mon, 2015-06-29 at 12:47 +0200, Vratislav Podzimek wrote:
> On Fri, 2015-06-26 at 15:31 +0200, Martin Kolman wrote:
> > Instead of crashing prompt the user to select a valid environment 
> > from
> > the list in the software spoke. Also make sure the selected 
> > environment
> > is written in the installation describing kickstart file.
> > 
> > Resolves: rhbz#1234890
> > Signed-off-by: Martin Kolman <mkolman at redhat.com>
> > ---
> >  pyanaconda/ui/gui/spokes/software.py | 76 
> > ++++++++++++++++++++++++++----------
> >  1 file changed, 55 insertions(+), 21 deletions(-)
> > 
> > diff --git a/pyanaconda/ui/gui/spokes/software.py 
> > b/pyanaconda/ui/gui/spokes/software.py
> > index 17498af..4801643 100644
> > --- a/pyanaconda/ui/gui/spokes/software.py
> > +++ b/pyanaconda/ui/gui/spokes/software.py
> > @@ -67,7 +67,6 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >  
> >          self.selectedGroups = []
> >          self.excludedGroups = []
> > -        self.environment = None
> >  
> >          self._environmentListBox = 
> > self.builder.get_object("environmentListBox")
> >          self._addonListBox = 
> > self.builder.get_object("addonListBox")
> > @@ -100,15 +99,44 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >      def _downloading_group_md(self):
> >          hubQ.send_message(self.__class__.__name__, _("Downloading 
> > group metadata..."))
> >  
> > +    @property
> > +    def environment(self):
> > +        """A wrapper for the environment specification in 
> > kickstart"""
> > +        return self.data.packages.environment
> > +
> > +    @environment.setter
> > +    def environment(self, value):
> > +        self.data.packages.environment = value
> > +
> > +    @property
> > +    def environment_valid(self):
> > +        """Return if the currently set environment is valid
> > +        (represents an environment known by the payload)
> > +        """
> > +        # None means the environment has not been set by the user,
> > +        # which means:
> > +        # * set the default environment during interactive 
> > installation
> > +        # * ask user to specify an environment during kickstart 
> > installation
> > +        if self.environment is None:
> > +            return True
> > +        else:
> > +            return self.environment in self.payload.environments
> > +
> >      def _payload_finished(self):
> > -        self.environment = self.data.packages.environment
> > +        if self.environment_valid:
> > +            log.info("using environment from kickstart: %s", 
> > self.data.packages.environment)
> AFAICT this may be misleading because self.environment_valid is True 
> if
> self.environment is None (and thus not set in kickstart at all). Or 
> is
> this callback triggered only when some environment was set/requested?
I think this triggers when the source changes - so on startup on when
the user changes the installation source during the installation.
>From this point of view environment being None is valid - it means it
has not been set and a default value will be used. And if there is no
%package section at all the user will be prompted to verify that the 
default environment is OK.

> 
> Also, it should use self.environment not 
> self.data.packages.environment.
Fixing. :)
> 
> > +        else:
> > +            log.error("unknown environment has been specified in 
> > kickstart and will be ignored: %s",
> > +                      self.data.packages.environment)
> > +            # False means that the environment has been set to an 
> > invalid value and needs to
> > +            # be manually set to a valid one.
> > +            self.environment = False
> >  
> >      def _payload_error(self):
> >          hubQ.send_message(self.__class__.__name__, 
> > payloadMgr.error)
> >  
> >      def _apply(self):
> > -        env = self._get_selected_environment()
> > -        if not env:
> > +        if not self.environment:
> >              return
> >  
> >          addons = self._get_selected_addons()
> > @@ -118,8 +146,7 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >  
> >          self._selectFlag = False
> >          self.payload.data.packages.groupList = []
> > -        self.payload.selectEnvironment(env)
> > -        self.environment = env
> > +        self.payload.selectEnvironment(self.environment)
> >          for group in self.selectedGroups:
> >              self.payload.selectGroup(group)
> >  
> > @@ -161,20 +188,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >          # we should always check processingDone before checking 
> > the other variables,
> >          # as they might be inconsistent until processing is 
> > finished
> >          if flags.automatedInstall:
> > -            return processingDone and self.data.packages.seen
> > +            # we can't let the automated installatin proceed until 
> > a valid environment
>                                            ^typo
Fixing. :)
> 
> > +            # has been set
> > +            return processingDone and self.data.packages.seen and 
> > self.environment_valid
> >          else:
> > -            return processingDone and 
> > self._get_selected_environment() is not None
> > +            return processingDone and self.environment is not None
> >  
> >      @property
> >      def changed(self):
> > -        env = self._get_selected_environment()
> > -        if not env:
> > +        if not self.environment:
> >              return True
> >  
> >          addons = self._get_selected_addons()
> >  
> >          # Don't redo dep solving if nothing's changed.
> > -        if env == self._origEnvironment and set(addons) == 
> > set(self._origAddons) and \
> > +        if self.environment == self._origEnvironment and 
> > set(addons) == set(self._origAddons) and \
> >             self.txid_valid:
> >              return False
> >  
> > @@ -211,17 +239,21 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >          if not self.txid_valid:
> >              return _("Source changed - please verify")
> >  
> > -        env = self._get_selected_environment()
> > -        if not env:
> > +        if not self.environment:
> >              # Kickstart installs with %packages will have a row 
> > selected, unless
> >              # they did an install without a desktop environment. 
> >  This should
> >              # catch that one case.
> >              if flags.automatedInstall and self.data.packages.seen:
> > -                return _("Custom software selected")
> > +                if self.environment_valid:
> > +                    return _("Custom software selected")
> > +                else:
> > +                    # It is impossible to set an invalid 
> > environment in the GUI,
> > +                    # os any invalid environment must have come 
> > from kickstart.
>                         ^^so?
Also fixing. :)
> 
> > +                    return _("Invalid environment specified in 
> > kickstart")
> >  
> >              return _("Nothing selected")
> >  
> > -        return self.payload.environmentDescription(env)[0]
> > +        return 
> > self.payload.environmentDescription(self.environment)[0]
> >  
> >      def initialize(self):
> >          NormalSpoke.initialize(self)
> > @@ -308,6 +340,14 @@ class SoftwareSelectionSpoke(NormalSpoke):
> >          self._environmentListBox.show_all()
> >          self._addonListBox.show_all()
> >  
> > +        # Pre-select the default environment when the user enters 
> > the software spoke
> > +        # to fix an invalid environment set in kickstart. 
> > Otherwise the default environment
> > +        # would still look as selected but the selection would not 
> > be applied after returning
> > +        # to the hub.
> > +        if flags.automatedInstall and self.environment is None:
> > +            row = self._environmentListBox.get_row_at_index(0)
> > +            row.activate()
> Shouldn't this check 'self.environment is False' as set above in
> _payload_finished()?
> 
I think I'll refactor this part a bit more because the current code
here is too insane.





More information about the anaconda-patches mailing list