Add ChangeAction to parse sub-options from --change

* Covers both commit and import commands
* Cleaned up export command
* Removed unneeded calls to super().__init__()

Fixes #1702

Signed-off-by: Jhon Honce <jhonce@redhat.com>

6 files changed, 96 insertions, 57 deletions

diff --git a/contrib/python/podman/podman/libs/images.py b/contrib/python/podman/podman/libs/images.py
index 982546cd2..9453fb416 100644
--- a/contrib/python/podman/podman/libs/images.py
+++ b/contrib/python/podman/podman/libs/images.py
@@ -137,7 +137,7 @@ class Images():
             results = podman.DeleteUnusedImages()
         return results['images']
 
-    def import_image(self, source, reference, message=None, changes=None):
+    def import_image(self, source, reference, message='', changes=None):
         """Read image tarball from source and save in image store."""
         with self._client() as podman:
             results = podman.ImportImage(source, reference, message, changes)
diff --git a/contrib/python/pypodman/pypodman/lib/__init__.py b/contrib/python/pypodman/pypodman/lib/__init__.py
index 5525ddaef..be1b5f467 100644
--- a/contrib/python/pypodman/pypodman/lib/__init__.py
+++ b/contrib/python/pypodman/pypodman/lib/__init__.py
@@ -4,14 +4,15 @@ import sys
 import podman
 from pypodman.lib.action_base import AbstractActionBase
 from pypodman.lib.parser_actions import (BooleanAction, BooleanValidate,
-                                         PathAction, PositiveIntAction,
-                                         UnitAction)
+                                         ChangeAction, PathAction,
+                                         PositiveIntAction, UnitAction)
 from pypodman.lib.podman_parser import PodmanArgumentParser
 from pypodman.lib.report import Report, ReportColumn
 
 # Silence pylint overlording...
 assert BooleanAction
 assert BooleanValidate
+assert ChangeAction
 assert PathAction
 assert PositiveIntAction
 assert UnitAction
diff --git a/contrib/python/pypodman/pypodman/lib/actions/commit_action.py b/contrib/python/pypodman/pypodman/lib/actions/commit_action.py
index 0da6a2078..21665ad0b 100644
--- a/contrib/python/pypodman/pypodman/lib/actions/commit_action.py
+++ b/contrib/python/pypodman/pypodman/lib/actions/commit_action.py
@@ -2,7 +2,7 @@
 import sys
 
 import podman
-from pypodman.lib import AbstractActionBase, BooleanAction
+from pypodman.lib import AbstractActionBase, BooleanAction, ChangeAction
 
 
 class Commit(AbstractActionBase):
@@ -12,7 +12,9 @@ class Commit(AbstractActionBase):
     def subparser(cls, parent):
         """Add Commit command to parent parser."""
         parser = parent.add_parser(
-            'commit', help='create image from container')
+            'commit',
+            help='create image from container',
+        )
         parser.add_argument(
             '--author',
             help='Set the author for the committed image',
@@ -20,11 +22,7 @@ class Commit(AbstractActionBase):
         parser.add_argument(
             '--change',
             '-c',
-            choices=('CMD', 'ENTRYPOINT', 'ENV', 'EXPOSE', 'LABEL', 'ONBUILD',
-                     'STOPSIGNAL', 'USER', 'VOLUME', 'WORKDIR'),
-            action='append',
-            type=str.upper,
-            help='Apply the following possible changes to the created image',
+            action=ChangeAction,
         )
         parser.add_argument(
             '--format',
@@ -69,27 +67,11 @@ class Commit(AbstractActionBase):
         )
         parser.set_defaults(class_=cls, method='commit')
 
-    def __init__(self, args):
-        """Construct Commit class."""
-        if not args.container:
-            raise ValueError('You must supply one container id'
-                             ' or name to be used as source.')
-        if not args.image:
-            raise ValueError('You must supply one image id'
-                             ' or name to be created.')
-        super().__init__(args)
-
-        # used only on client
-        del self.opts['image']
-        del self.opts['container']
-
     def commit(self):
         """Create image from container."""
         try:
             try:
                 ctnr = self.client.containers.get(self._args.container[0])
-                ident = ctnr.commit(**self.opts)
-                print(ident)
             except podman.ContainerNotFound as e:
                 sys.stdout.flush()
                 print(
@@ -97,6 +79,9 @@ class Commit(AbstractActionBase):
                     file=sys.stderr,
                     flush=True)
                 return 1
+            else:
+                ident = ctnr.commit(self.opts['image'][0], **self.opts)
+                print(ident)
         except podman.ErrorOccurred as e:
             sys.stdout.flush()
             print(
@@ -104,3 +89,4 @@ class Commit(AbstractActionBase):
                 file=sys.stderr,
                 flush=True)
             return 1
+        return 0
diff --git a/contrib/python/pypodman/pypodman/lib/actions/export_action.py b/contrib/python/pypodman/pypodman/lib/actions/export_action.py
index f62cd3535..7ef178c4c 100644
--- a/contrib/python/pypodman/pypodman/lib/actions/export_action.py
+++ b/contrib/python/pypodman/pypodman/lib/actions/export_action.py
@@ -12,13 +12,16 @@ class Export(AbstractActionBase):
     def subparser(cls, parent):
         """Add Export command to parent parser."""
         parser = parent.add_parser(
-            'export', help='export container to tarball')
+            'export',
+            help='export container to tarball',
+        )
         parser.add_argument(
             '--output',
             '-o',
             metavar='PATH',
             nargs=1,
-            help='Write to a file',
+            required=True,
+            help='Write to this file on host',
         )
         parser.add_argument(
             'container',
@@ -27,23 +30,11 @@ class Export(AbstractActionBase):
         )
         parser.set_defaults(class_=cls, method='export')
 
-    def __init__(self, args):
-        """Construct Export class."""
-        if not args.container:
-            raise ValueError('You must supply one container id'
-                             ' or name to be used as source.')
-
-        if not args.output:
-            raise ValueError('You must supply one filename'
-                             ' to be created as tarball using --output.')
-        super().__init__(args)
-
     def export(self):
         """Create tarball from container filesystem."""
         try:
             try:
                 ctnr = self.client.containers.get(self._args.container[0])
-                ctnr.export(self._args.output[0])
             except podman.ContainerNotFound as e:
                 sys.stdout.flush()
                 print(
@@ -51,6 +42,8 @@ class Export(AbstractActionBase):
                     file=sys.stderr,
                     flush=True)
                 return 1
+            else:
+                ctnr.export(self._args.output[0])
         except podman.ErrorOccurred as e:
             sys.stdout.flush()
             print(
@@ -58,3 +51,4 @@ class Export(AbstractActionBase):
                 file=sys.stderr,
                 flush=True)
             return 1
+        return 0
diff --git a/contrib/python/pypodman/pypodman/lib/actions/import_action.py b/contrib/python/pypodman/pypodman/lib/actions/import_action.py
index 49b8a5a57..43448144a 100644
--- a/contrib/python/pypodman/pypodman/lib/actions/import_action.py
+++ b/contrib/python/pypodman/pypodman/lib/actions/import_action.py
@@ -2,7 +2,7 @@
 import sys
 
 import podman
-from pypodman.lib import AbstractActionBase
+from pypodman.lib import AbstractActionBase, ChangeAction
 
 
 class Import(AbstractActionBase):
@@ -12,18 +12,19 @@ class Import(AbstractActionBase):
     def subparser(cls, parent):
         """Add Import command to parent parser."""
         parser = parent.add_parser(
-            'import', help='import tarball as image filesystem')
+            'import',
+            help='import tarball as image filesystem',
+        )
         parser.add_argument(
             '--change',
             '-c',
-            action='append',
-            choices=('CMD', 'ENTRYPOINT', 'ENV', 'EXPOSE', 'LABEL',
-                     'STOPSIGNAL', 'USER', 'VOLUME', 'WORKDIR'),
-            type=str.upper,
-            help='Apply the following possible instructions',
+            action=ChangeAction,
         )
         parser.add_argument(
-            '--message', '-m', help='Set commit message for imported image.')
+            '--message',
+            '-m',
+            help='Set commit message for imported image.',
+        )
         parser.add_argument(
             'source',
             metavar='PATH',
@@ -38,18 +39,25 @@ class Import(AbstractActionBase):
         )
         parser.set_defaults(class_=cls, method='import_')
 
-    def __init__(self, args):
-        """Construct Import class."""
-        super().__init__(args)
-
     def import_(self):
         """Import tarball as image filesystem."""
+        # ImportImage() validates it's parameters therefore we need to create
+        # pristine dict() for keywords
+        options = {}
+        if 'message' in self.opts:
+            options['message'] = self.opts['message']
+        if 'change' in self.opts and self.opts['change']:
+            options['changes'] = self.opts['change']
+
+        reference = self.opts['reference'][0] if 'reference' in self.opts\
+            else None
+
         try:
             ident = self.client.images.import_image(
-                self.opts.source,
-                self.opts.reference,
-                message=self.opts.message,
-                changes=self.opts.change)
+                self.opts['source'][0],
+                reference,
+                **options,
+            )
             print(ident)
         except podman.ErrorOccurred as e:
             sys.stdout.flush()
@@ -58,3 +66,4 @@ class Import(AbstractActionBase):
                 file=sys.stderr,
                 flush=True)
             return 1
+        return 0
diff --git a/contrib/python/pypodman/pypodman/lib/parser_actions.py b/contrib/python/pypodman/pypodman/lib/parser_actions.py
index 2a5859e47..c10b85495 100644
--- a/contrib/python/pypodman/pypodman/lib/parser_actions.py
+++ b/contrib/python/pypodman/pypodman/lib/parser_actions.py
@@ -4,9 +4,10 @@ Supplimental argparse.Action converters and validaters.
 The constructors are very verbose but remain for IDE support.
 """
 import argparse
+import copy
 import os
 
-# API defined by argparse.Action shut up pylint
+# API defined by argparse.Action therefore shut up pylint
 # pragma pylint: disable=redefined-builtin
 # pragma pylint: disable=too-few-public-methods
 # pragma pylint: disable=too-many-arguments
@@ -63,6 +64,54 @@ class BooleanAction(argparse.Action):
             setattr(namespace, self.dest, val)
 
 
+class ChangeAction(argparse.Action):
+    """Convert and validate change argument."""
+
+    def __init__(self,
+                 option_strings,
+                 dest,
+                 nargs=None,
+                 const=None,
+                 default=[],
+                 type=None,
+                 choices=None,
+                 required=False,
+                 help=None,
+                 metavar='OPT=VALUE'):
+        """Create ChangeAction object."""
+        help = (help or '') + ('Apply change(s) to the new image.'
+                               ' May be given multiple times.')
+
+        super().__init__(
+            option_strings=option_strings,
+            dest=dest,
+            nargs=nargs,
+            const=const,
+            default=default,
+            type=type,
+            choices=choices,
+            required=required,
+            help=help,
+            metavar=metavar)
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        """Convert and Validate input."""
+        print(self.dest)
+        items = getattr(namespace, self.dest, None) or []
+        items = copy.copy(items)
+
+        choices = ('CMD', 'ENTRYPOINT', 'ENV', 'EXPOSE', 'LABEL', 'ONBUILD',
+                   'STOPSIGNAL', 'USER', 'VOLUME', 'WORKDIR')
+
+        opt, val = values.split('=', 1)
+        if opt not in choices:
+            parser.error('{} is not a supported "--change" option,'
+                         ' valid options are: {}'.format(
+                             opt, ', '.join(choices)))
+        items.append(values)
+        setattr(namespace, self.dest, items)
+
+
 class UnitAction(argparse.Action):
     """Validate number given is positive integer, with optional suffix."""