[master/rhel7] Add platform specific group selection (#884385)

[Brian C. Lane]

Virtualization platforms would like to have their packages installed
automatically when running inside them. This uses systemd-detect-virt to
identify the platform and creates a group id to be selected by prefixing
it with "platform-" so that, for example, running on kvm will result in
a group named "platform-kvm" being selected if it exists.
---
 pyanaconda/iutil.py                | 18 ++++++++++++++++++
 pyanaconda/packaging/__init__.py   | 11 +++++++++++
 pyanaconda/packaging/dnfpayload.py |  2 +-
 pyanaconda/packaging/yumpayload.py |  2 +-
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
index 642e7b0..325270d 100644
--- a/pyanaconda/iutil.py
+++ b/pyanaconda/iutil.py
@@ -878,3 +878,21 @@ def xprogressive_delay():
     while True:
         yield 0.25*(2**counter)
         counter += 1
+
+def get_platform_groupid():
+    """ Return a platform group id string
+
+        This runs systemd-detect-virt and if the result is not 'none' it
+        prefixes the lower case result with "platform-" for use as a group id.
+
+        :returns: str
+    """
+    try:
+        platform = execWithCapture("systemd-detect-virt", []).strip()
+    except (IOError, AttributeError):
+        return ""
+
+    if platform == "none":
+        return ""
+
+    return "platform-"+platform.lower()
diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py
index c368e0d..d235183 100644
--- a/pyanaconda/packaging/__init__.py
+++ b/pyanaconda/packaging/__init__.py
@@ -725,6 +725,17 @@ class PackagePayload(Payload):
         else:
             self.rpmMacros.append(('__file_context_path', '%{nil}'))

+        # Add platform specific group
+        groupid = iutil.get_platform_groupid()
+        if groupid and groupid  in self.groups:
+            if isinstance(groups, list):
+                log.info("Adding platform group %s", groupid)
+                groups.append(groupid)
+            else:
+                log.warning("Could not add %s to groups, not a list.", groupid)
+        elif groupid:
+            log.warning("Platform group %s not available.", groupid)
+
     @property
     def kernelPackages(self):
         if "kernel" in self.data.packages.excludedList:
diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py
index 64f7433..4efc524 100644
--- a/pyanaconda/packaging/dnfpayload.py
+++ b/pyanaconda/packaging/dnfpayload.py
@@ -556,7 +556,7 @@ class DNFPayload(packaging.PackagePayload):
         return list(gids)

def preInstall(self, packages=None, groups=None):
-        super(DNFPayload, self).preInstall()
+        super(DNFPayload, self).preInstall(packages, groups)
         self.requiredPackages = packages
         self.requiredGroups = groups
         self.addDriverRepos()
diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
index 6dca7f6..67f7a02 100644
--- a/pyanaconda/packaging/yumpayload.py
+++ b/pyanaconda/packaging/yumpayload.py
@@ -1332,7 +1332,7 @@ reposdir=%s

def preInstall(self, packages=None, groups=None):
         """ Perform pre-installation tasks. """
-        super(YumPayload, self).preInstall()
+        super(YumPayload, self).preInstall(packages, groups)
         progressQ.send_message(_("Starting package installation process"))

self.requiredPackages = packages

[Vratislav Podzimek]

On Tue, 2014-07-22 at 14:10 -0700, Brian C. Lane wrote:
> Virtualization platforms would like to have their packages installed
> automatically when running inside them. This uses systemd-detect-virt to
> identify the platform and creates a group id to be selected by prefixing
> it with "platform-" so that, for example, running on kvm will result in
> a group named "platform-kvm" being selected if it exists.
>
> pyanaconda/iutil.py                | 18 ++++++++++++++++++
>  pyanaconda/packaging/__init__.py   | 11 +++++++++++
>  pyanaconda/packaging/dnfpayload.py |  2 +-
>  pyanaconda/packaging/yumpayload.py |  2 +-
>  4 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
> index 642e7b0..325270d 100644
> --- a/pyanaconda/iutil.py
> +++ b/pyanaconda/iutil.py
> @@ -878,3 +878,21 @@ def xprogressive_delay():
>      while True:
>          yield 0.25*(2**counter)
>          counter += 1
>
> +def get_platform_groupid():
> """ Return a platform group id string
>    This runs systemd-detect-virt and if the result is not 'none' it
>    prefixes the lower case result with "platform-" for use as a group id.
>    :returns: str

I think this should be ':rtype: str'
> """
> try:
>    platform = execWithCapture("systemd-detect-virt", []).strip()
> except (IOError, AttributeError):
>    return ""
> if platform == "none":
>    return ""
> return "platform-"+platform.lower()

Please add spaces here  ^^^
> diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py
> index c368e0d..d235183 100644
> --- a/pyanaconda/packaging/__init__.py
> +++ b/pyanaconda/packaging/__init__.py
> @@ -725,6 +725,17 @@ class PackagePayload(Payload):
>          else:
>              self.rpmMacros.append(('__file_context_path', '%{nil}'))
>    # Add platform specific group
>    groupid = iutil.get_platform_groupid()
>    if groupid and groupid  in self.groups:

Extra space                 here ^^
>        if isinstance(groups, list):

Looking at the code 'groups' can be either a list or None so I think
'if groups:'
would be a nicer check here. And it could be anything that has the
'append' method that way.
>            log.info("Adding platform group %s", groupid)
>            groups.append(groupid)
>        else:
>            log.warning("Could not add %s to groups, not a list.", groupid)
>    elif groupid:
>        log.warning("Platform group %s not available.", groupid)
> @property
>    def kernelPackages(self):
>        if "kernel" in self.data.packages.excludedList:
>
> diff --git a/pyanaconda/packaging/dnfpayload.py b/pyanaconda/packaging/dnfpayload.py
> index 64f7433..4efc524 100644
> --- a/pyanaconda/packaging/dnfpayload.py
> +++ b/pyanaconda/packaging/dnfpayload.py
> @@ -556,7 +556,7 @@ class DNFPayload(packaging.PackagePayload):
>          return list(gids)
>  def preInstall(self, packages=None, groups=None):
>    super(DNFPayload, self).preInstall()
>    super(DNFPayload, self).preInstall(packages, groups)
>    self.requiredPackages = packages
>    self.requiredGroups = groups
>    self.addDriverRepos()
>
> diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
> index 6dca7f6..67f7a02 100644
> --- a/pyanaconda/packaging/yumpayload.py
> +++ b/pyanaconda/packaging/yumpayload.py
> @@ -1332,7 +1332,7 @@ reposdir=%s
>  def preInstall(self, packages=None, groups=None):
>      """ Perform pre-installation tasks. """
>    super(YumPayload, self).preInstall()
>    super(YumPayload, self).preInstall(packages, groups)
>    progressQ.send_message(_("Starting package installation process"))
>
>    self.requiredPackages = packages

Otherwise this looks good to me. A nice win-win solution I'd say.

[Brian C. Lane]

On Wed, Jul 23, 2014 at 09:41:58AM +0200, Vratislav Podzimek wrote:
> On Tue, 2014-07-22 at 14:10 -0700, Brian C. Lane wrote:
> > +def get_platform_groupid():
> > """ Return a platform group id string
> >    This runs systemd-detect-virt and if the result is not 'none' it
> >    prefixes the lower case result with "platform-" for use as a group id.
> >    :returns: str
>
> I think this should be ':rtype: str'

oops, right.
> > """
> > try:
> >    platform = execWithCapture("systemd-detect-virt", []).strip()
> > except (IOError, AttributeError):
> >    return ""
> > if platform == "none":
> >    return ""
> > return "platform-"+platform.lower()
>
> Please add spaces here  ^^^

will do.
> > diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py
> > index c368e0d..d235183 100644
> > --- a/pyanaconda/packaging/__init__.py
> > +++ b/pyanaconda/packaging/__init__.py
> > @@ -725,6 +725,17 @@ class PackagePayload(Payload):
> >          else:
> >              self.rpmMacros.append(('__file_context_path', '%{nil}'))
> >    # Add platform specific group
> >    groupid = iutil.get_platform_groupid()
> >    if groupid and groupid  in self.groups:
>
> Extra space                 here ^^
> >        if isinstance(groups, list):
>
> Looking at the code 'groups' can be either a list or None so I think
> 'if groups:'
> would be a nicer check here. And it could be anything that has the
> 'append' method that way.

'if groups' won't catch the case where an empty list is passed in and if
None is passed you can't pass a new object back out. groups is supposed
to be a list so I don't see any benefit in expanding it to cover other
things with append (what is there? set() doesn't have it and neither
does dict).

[Vratislav Podzimek]

On Wed, 2014-07-23 at 08:54 -0700, Brian C. Lane wrote:
> On Wed, Jul 23, 2014 at 09:41:58AM +0200, Vratislav Podzimek wrote:
> > On Tue, 2014-07-22 at 14:10 -0700, Brian C. Lane wrote:
> > > +def get_platform_groupid():
> > > """ Return a platform group id string
> > >    This runs systemd-detect-virt and if the result is not 'none' it
> > >    prefixes the lower case result with "platform-" for use as a group id.
> > >    :returns: str
> >
> > I think this should be ':rtype: str'
>
> oops, right.
> > > """
> > > try:
> > >    platform = execWithCapture("systemd-detect-virt", []).strip()
> > > except (IOError, AttributeError):
> > >    return ""
> > > if platform == "none":
> > >    return ""
> > > return "platform-"+platform.lower()
> >
> > Please add spaces here  ^^^
>
> will do.
> > > diff --git a/pyanaconda/packaging/__init__.py b/pyanaconda/packaging/__init__.py
> > > index c368e0d..d235183 100644
> > > --- a/pyanaconda/packaging/__init__.py
> > > +++ b/pyanaconda/packaging/__init__.py
> > > @@ -725,6 +725,17 @@ class PackagePayload(Payload):
> > >          else:
> > >              self.rpmMacros.append(('__file_context_path', '%{nil}'))
> > >    # Add platform specific group
> > >    groupid = iutil.get_platform_groupid()
> > >    if groupid and groupid  in self.groups:
> >
> > Extra space                 here ^^
> > >        if isinstance(groups, list):
> >
> > Looking at the code 'groups' can be either a list or None so I think
> > 'if groups:'
> > would be a nicer check here. And it could be anything that has the
> > 'append' method that way.
>
> 'if groups' won't catch the case where an empty list is passed in and if
> None is passed you can't pass a new object back out. groups is supposed
> to be a list so I don't see any benefit in expanding it to cover other
> things with append (what is there? set() doesn't have it and neither
> does dict).

It could still be 'if groups is not None', but you are right. I forgot
that anything derived from the list class would work with the
isinstance() check.