image: Simplify handling of data provided via stdin

This was unnecessarily complex.

Change-Id: I8289d5ce7356d8bc89425590a7f71bca91a6d396
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

Author: stephenfin

openstackclient/image/v2/image.py

@@ -134,34 +134,32 @@ def _get_member_columns(item):
     )
 
 
-def get_data_file(args):
-    if args.file:
-        return (open(args.file, 'rb'), args.file)
+def get_data_from_stdin():
+    # distinguish cases where:
+    # (1) stdin is not valid (as in cron jobs):
+    #    openstack ... <&-
+    # (2) image data is provided through stdin:
+    #    openstack ... < /tmp/file
+    # (3) no image data provided
+    #    openstack ...
+    try:
+        os.fstat(0)
+    except OSError:
+        # (1) stdin is not valid
+        return None
+
+    if not sys.stdin.isatty():
+        # (2) image data is provided through stdin
+        image = sys.stdin
+        if hasattr(sys.stdin, 'buffer'):
+            image = sys.stdin.buffer
+        if msvcrt:
+            msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY)
+
+        return image
     else:
-        # distinguish cases where:
-        # (1) stdin is not valid (as in cron jobs):
-        #    openstack ... <&-
-        # (2) image data is provided through stdin:
-        #    openstack ... < /tmp/file
-        # (3) no image data provided
-        #    openstack ...
-        try:
-            os.fstat(0)
-        except OSError:
-            # (1) stdin is not valid
-            return (None, None)
-        if not sys.stdin.isatty():
-            # (2) image data is provided through stdin
-            image = sys.stdin
-            if hasattr(sys.stdin, 'buffer'):
-                image = sys.stdin.buffer
-            if msvcrt:
-                msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY)
-
-            return (image, None)
-        else:
-            # (3)
-            return (None, None)
+        # (3)
+        return None
 
 
 class AddProjectToImage(command.ShowOne):
@@ -277,6 +275,7 @@ def get_parser(self, prog_name):
         source_group = parser.add_mutually_exclusive_group()
         source_group.add_argument(
             "--file",
+            dest="filename",
             metavar="<file>",
             help=_("Upload image from local file"),
         )
@@ -299,7 +298,10 @@ def get_parser(self, prog_name):
             "--progress",
             action="store_true",
             default=False,
-            help=_("Show upload progress bar."),
+            help=_(
+                "Show upload progress bar "
+                "(ignored if passing data via stdin)"
+            ),
         )
         parser.add_argument(
             '--sign-key-path',
@@ -463,7 +465,15 @@ def _take_action_image(self, parsed_args):
         # open the file first to ensure any failures are handled before the
         # image is created. Get the file name (if it is file, and not stdin)
         # for easier further handling.
-        fp, fname = get_data_file(parsed_args)
+        if parsed_args.filename:
+            try:
+                fp = open(parsed_args.filename, 'rb')
+            except FileNotFoundError:
+                raise exceptions.CommandError(
+                    '%r is not a valid file' % parsed_args.filename,
+                )
+        else:
+            fp = get_data_from_stdin()
 
         if fp is not None and parsed_args.volume:
             msg = _(
@@ -472,26 +482,24 @@ def _take_action_image(self, parsed_args):
             )
             raise exceptions.CommandError(msg)
 
-        if fp is None and parsed_args.file:
-            LOG.warning(_("Failed to get an image file."))
-            return {}, {}
-
-        if parsed_args.progress and parsed_args.file:
+        if parsed_args.progress and parsed_args.filename:
             # NOTE(stephenfin): we only show a progress bar if the user
             # requested it *and* we're reading from a file (not stdin)
-            filesize = os.path.getsize(fname)
+            filesize = os.path.getsize(parsed_args.filename)
             if filesize is not None:
                 kwargs['validate_checksum'] = False
                 kwargs['data'] = progressbar.VerboseFileWrapper(fp, filesize)
-        elif fname:
-            kwargs['filename'] = fname
+            else:
+                kwargs['data'] = fp
+        elif parsed_args.filename:
+            kwargs['filename'] = parsed_args.filename
         elif fp:
             kwargs['validate_checksum'] = False
             kwargs['data'] = fp
 
         # sign an image using a given local private key file
         if parsed_args.sign_key_path or parsed_args.sign_cert_id:
-            if not parsed_args.file:
+            if not parsed_args.filename:
                 msg = _(
                     "signing an image requires the --file option, "
                     "passing files via stdin when signing is not "
@@ -546,6 +554,10 @@ def _take_action_image(self, parsed_args):
                 kwargs['img_signature_key_type'] = signer.padding_method
 
         image = image_client.create_image(**kwargs)
+
+        if parsed_args.filename:
+            fp.close()
+
         return _format_image(image)
 
     def _take_action_volume(self, parsed_args):
@@ -980,6 +992,7 @@ def get_parser(self, prog_name):
         parser.add_argument(
             "--file",
             metavar="<filename>",
+            dest="filename",
             help=_("Downloaded image save filename (default: stdout)"),
         )
         parser.add_argument(
@@ -993,7 +1006,7 @@ def take_action(self, parsed_args):
         image_client = self.app.client_manager.image
         image = image_client.find_image(parsed_args.image)
 
-        output_file = parsed_args.file
+        output_file = parsed_args.filename
         if output_file is None:
             output_file = getattr(sys.stdout, "buffer", sys.stdout)
 

openstackclient/tests/unit/image/v2/test_image.py

@@ -14,7 +14,6 @@
 
 import copy
 import io
-import os
 import tempfile
 from unittest import mock
 
@@ -104,15 +103,8 @@ def test_image_reserve_no_options(self, raw_input):
             disk_format=image.DEFAULT_DISK_FORMAT,
         )
 
-        # Verify update() was not called, if it was show the args
-        self.assertEqual(self.client.update_image.call_args_list, [])
-
-        self.assertEqual(
-            self.expected_columns,
-            columns)
-        self.assertCountEqual(
-            self.expected_data,
-            data)
+        self.assertEqual(self.expected_columns, columns)
+        self.assertCountEqual(self.expected_data, data)
 
     @mock.patch('sys.stdin', side_effect=[None])
     def test_image_reserve_options(self, raw_input):
@@ -121,10 +113,11 @@ def test_image_reserve_options(self, raw_input):
             '--disk-format', 'ami',
             '--min-disk', '10',
             '--min-ram', '4',
-            ('--protected'
-                if self.new_image.is_protected else '--unprotected'),
-            ('--private'
-                if self.new_image.visibility == 'private' else '--public'),
+            '--protected' if self.new_image.is_protected else '--unprotected',
+            (
+                '--private'
+                if self.new_image.visibility == 'private' else '--public'
+            ),
             '--project', self.new_image.owner_id,
             '--project-domain', self.domain.id,
             self.new_image.name,
@@ -160,12 +153,8 @@ def test_image_reserve_options(self, raw_input):
             visibility=self.new_image.visibility,
         )
 
-        self.assertEqual(
-            self.expected_columns,
-            columns)
-        self.assertCountEqual(
-            self.expected_data,
-            data)
+        self.assertEqual(self.expected_columns, columns)
+        self.assertCountEqual(self.expected_data, data)
 
     def test_image_create_with_unexist_project(self):
         self.project_mock.get.side_effect = exceptions.NotFound(None)
@@ -217,7 +206,7 @@ def test_image_create_file(self):
             self.new_image.name,
         ]
         verifylist = [
-            ('file', imagefile.name),
+            ('filename', imagefile.name),
             ('is_protected', self.new_image.is_protected),
             ('visibility', self.new_image.visibility),
             ('properties', {'Alpha': '1', 'Beta': '2'}),
@@ -252,12 +241,12 @@ def test_image_create_file(self):
             self.expected_data,
             data)
 
-    @mock.patch('openstackclient.image.v2.image.get_data_file')
+    @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
     def test_image_create__progress_ignore_with_stdin(
-        self, mock_get_data_file,
+        self, mock_get_data_from_stdin,
     ):
         fake_stdin = io.StringIO('fake-image-data')
-        mock_get_data_file.return_value = (fake_stdin, None)
+        mock_get_data_from_stdin.return_value = fake_stdin
 
         arglist = [
             '--progress',
@@ -322,11 +311,11 @@ def test_image_create_import(self, raw_input):
         )
 
     @mock.patch('osc_lib.utils.find_resource')
-    @mock.patch('openstackclient.image.v2.image.get_data_file')
+    @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
     def test_image_create_from_volume(self, mock_get_data_f, mock_get_vol):
 
         fake_vol_id = 'fake-volume-id'
-        mock_get_data_f.return_value = (None, None)
+        mock_get_data_f.return_value = None
 
         class FakeVolume:
             id = fake_vol_id
@@ -353,12 +342,12 @@ class FakeVolume:
         )
 
     @mock.patch('osc_lib.utils.find_resource')
-    @mock.patch('openstackclient.image.v2.image.get_data_file')
+    @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
     def test_image_create_from_volume_fail(self, mock_get_data_f,
                                            mock_get_vol):
 
         fake_vol_id = 'fake-volume-id'
-        mock_get_data_f.return_value = (None, None)
+        mock_get_data_f.return_value = None
 
         class FakeVolume:
             id = fake_vol_id
@@ -379,15 +368,15 @@ class FakeVolume:
                           parsed_args)
 
     @mock.patch('osc_lib.utils.find_resource')
-    @mock.patch('openstackclient.image.v2.image.get_data_file')
+    @mock.patch('openstackclient.image.v2.image.get_data_from_stdin')
     def test_image_create_from_volume_v31(self, mock_get_data_f,
                                           mock_get_vol):
 
         self.app.client_manager.volume.api_version = (
             api_versions.APIVersion('3.1'))
 
         fake_vol_id = 'fake-volume-id'
-        mock_get_data_f.return_value = (None, None)
+        mock_get_data_f.return_value = None
 
         class FakeVolume:
             id = fake_vol_id
@@ -1798,7 +1787,7 @@ def test_save_data(self):
         arglist = ['--file', '/path/to/file', self.image.id]
 
         verifylist = [
-            ('file', '/path/to/file'),
+            ('filename', '/path/to/file'),
             ('image', self.image.id),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -1813,49 +1802,26 @@ def test_save_data(self):
 
 class TestImageGetData(TestImage):
 
-    def setUp(self):
-        super().setUp()
-        self.args = mock.Mock()
-
-    def test_get_data_file_file(self):
-        (fd, fname) = tempfile.mkstemp(prefix='osc_test_image')
-        self.args.file = fname
-
-        (test_fd, test_name) = image.get_data_file(self.args)
-
-        self.assertEqual(fname, test_name)
-        test_fd.close()
-
-        os.unlink(fname)
-
-    def test_get_data_file_2(self):
-
-        self.args.file = None
-
-        f = io.BytesIO(b"some initial binary data: \x00\x01")
+    def test_get_data_from_stdin(self):
+        fd = io.BytesIO(b"some initial binary data: \x00\x01")
 
         with mock.patch('sys.stdin') as stdin:
-            stdin.return_value = f
+            stdin.return_value = fd
             stdin.isatty.return_value = False
-            stdin.buffer = f
+            stdin.buffer = fd
 
-            (test_fd, test_name) = image.get_data_file(self.args)
+            test_fd = image.get_data_from_stdin()
 
             # Ensure data written to temp file is correct
-            self.assertEqual(f, test_fd)
-            self.assertIsNone(test_name)
-
-    def test_get_data_file_3(self):
-
-        self.args.file = None
+            self.assertEqual(fd, test_fd)
 
-        f = io.BytesIO(b"some initial binary data: \x00\x01")
+    def test_get_data_from_stdin__interactive(self):
+        fd = io.BytesIO(b"some initial binary data: \x00\x01")
 
         with mock.patch('sys.stdin') as stdin:
             # There is stdin, but interactive
-            stdin.return_value = f
+            stdin.return_value = fd
 
-            (test_fd, test_fname) = image.get_data_file(self.args)
+            test_fd = image.get_data_from_stdin()
 
             self.assertIsNone(test_fd)
-            self.assertIsNone(test_fname)