Require disk and container format on image-create

Currently glanceclient doesn't enforce disk-format or container-format
presence in the command line on image-create when providing image data
(with --file, --location, --copy-from), which means that the POST
request is made with the whole image and error is reported by Glance
API.

This post enforces presence of those arguments when image data is
provided so we can get the error quicker and avoid sending unnecessary
data over the network.

Change-Id: I5914fa9cfef190a028b374005adbf3a804b1b677
Closes-Bug: #1309272

5 files changed, 198 insertions, 2 deletions

diff --git a/glanceclient/common/utils.py b/glanceclient/common/utils.py
index 8daafff..304fcd4 100644
--- a/glanceclient/common/utils.py
+++ b/glanceclient/common/utils.py
@@ -36,11 +36,15 @@ from oslo_utils import encodeutils
 from oslo_utils import strutils
 import prettytable
 
+from glanceclient import _i18n
 from glanceclient import exc
 
+_ = _i18n._
+
 _memoized_property_lock = threading.Lock()
 
 SENSITIVE_HEADERS = ('X-Auth-Token', )
+REQUIRED_FIELDS_ON_DATA = ('disk_format', 'container_format')
 
 
 # Decorator for cli-args
@@ -53,6 +57,47 @@ def arg(*args, **kwargs):
     return _decorator
 
 
+def on_data_require_fields(data_fields, required=REQUIRED_FIELDS_ON_DATA):
+    """Decorator to check commands' validity
+
+    This decorator checks that required fields are present when image
+    data has been supplied via command line arguments or via stdin
+
+    On error throws CommandError exception with meaningful message.
+
+    :param data_fields: Which fields' presence imply image data
+    :type data_fields: iter
+    :param required: Required fields
+    :type required: iter
+    :return: function decorator
+    """
+
+    def args_decorator(func):
+        def prepare_fields(fields):
+            args = ('--' + x.replace('_', '-') for x in fields)
+            return ', '.join(args)
+
+        def func_wrapper(gc, args):
+            # Set of arguments with data
+            fields = set(a[0] for a in vars(args).items() if a[1])
+
+            # Fields the conditional requirements depend on
+            present = fields.intersection(data_fields)
+
+            # How many conditional requirements are missing
+            missing = set(required) - fields
+
+            # We use get_data_file to check if data is provided in stdin
+            if (present or get_data_file(args)) and missing:
+                msg = (_("error: Must provide %(req)s when using %(opt)s.") %
+                       {'req': prepare_fields(missing),
+                        'opt': prepare_fields(present) or 'stdin'})
+                raise exc.CommandError(msg)
+            return func(gc, args)
+        return func_wrapper
+    return args_decorator
+
+
 def schema_args(schema_getter, omit=None):
     omit = omit or []
     typemap = {
diff --git a/glanceclient/tests/unit/v1/test_shell.py b/glanceclient/tests/unit/v1/test_shell.py
index a3a41c2..dd3e3cf 100644
--- a/glanceclient/tests/unit/v1/test_shell.py
+++ b/glanceclient/tests/unit/v1/test_shell.py
@@ -232,6 +232,9 @@ class ShellInvalidEndpointandParameterTest(utils.TestCase):
             'OS_IMAGE_URL': 'http://is.invalid'}
 
         self.shell = shell.OpenStackImagesShell()
+        self.patched = mock.patch('glanceclient.common.utils.get_data_file',
+                                  autospec=True, return_value=None)
+        self.mock_get_data_file = self.patched.start()
 
         self.gc = self._mock_glance_client()
 
@@ -254,6 +257,7 @@ class ShellInvalidEndpointandParameterTest(utils.TestCase):
     def tearDown(self):
         super(ShellInvalidEndpointandParameterTest, self).tearDown()
         os.environ = self.old_environment
+        self.patched.stop()
 
     def run_command(self, cmd):
         self.shell.main(cmd.split())
@@ -324,6 +328,46 @@ class ShellInvalidEndpointandParameterTest(utils.TestCase):
             self.run_command, 'image-create --min-disk 10gb')
 
     @mock.patch('sys.stderr')
+    def test_image_create_missing_disk_format(self, __):
+        # We test for all possible sources
+        for origin in ('--file', '--location', '--copy-from'):
+            e = self.assertRaises(exc.CommandError, self.run_command,
+                                  '--os-image-api-version 1 image-create ' +
+                                  origin + ' fake_src --container-format bare')
+            self.assertEqual('error: Must provide --disk-format when using '
+                             + origin + '.', e.message)
+
+    @mock.patch('sys.stderr')
+    def test_image_create_missing_container_format(self, __):
+        # We test for all possible sources
+        for origin in ('--file', '--location', '--copy-from'):
+            e = self.assertRaises(exc.CommandError, self.run_command,
+                                  '--os-image-api-version 1 image-create ' +
+                                  origin + ' fake_src --disk-format qcow2')
+            self.assertEqual('error: Must provide --container-format when '
+                             'using ' + origin + '.', e.message)
+
+    @mock.patch('sys.stderr')
+    def test_image_create_missing_container_format_stdin_data(self, __):
+        # Fake that get_data_file method returns data
+        self.mock_get_data_file.return_value = six.StringIO()
+        e = self.assertRaises(exc.CommandError, self.run_command,
+                              '--os-image-api-version 1 image-create'
+                              ' --disk-format qcow2')
+        self.assertEqual('error: Must provide --container-format when '
+                         'using stdin.', e.message)
+
+    @mock.patch('sys.stderr')
+    def test_image_create_missing_disk_format_stdin_data(self, __):
+        # Fake that get_data_file method returns data
+        self.mock_get_data_file.return_value = six.StringIO()
+        e = self.assertRaises(exc.CommandError, self.run_command,
+                              '--os-image-api-version 1 image-create'
+                              ' --container-format bare')
+        self.assertEqual('error: Must provide --disk-format when using stdin.',
+                         e.message)
+
+    @mock.patch('sys.stderr')
     def test_image_update_invalid_size_parameter(self, __):
         self.assertRaises(
             SystemExit,
diff --git a/glanceclient/tests/unit/v2/test_shell_v2.py b/glanceclient/tests/unit/v2/test_shell_v2.py
index faa12a7..e2678af 100644
--- a/glanceclient/tests/unit/v2/test_shell_v2.py
+++ b/glanceclient/tests/unit/v2/test_shell_v2.py
@@ -16,18 +16,73 @@
 import json
 import mock
 import os
+import six
 import tempfile
 import testtools
 
 from glanceclient.common import utils
+from glanceclient import exc
+from glanceclient import shell
+
+# NOTE(geguileo): This is very nasty, but I can't find a better way to set
+# command line arguments in glanceclient.v2.shell.do_image_create that are
+# set by decorator utils.schema_args while preserving the spirits of the test
+
+# Backup original decorator
+original_schema_args = utils.schema_args
+
+
+# Set our own decorator that calls the original but with simulated schema
+def schema_args(schema_getter, omit=None):
+    global original_schema_args
+    # We only add the 2 arguments that are required by image-create
+    my_schema_getter = lambda: {
+        'properties': {
+            'container_format': {
+                'enum': [None, 'ami', 'ari', 'aki', 'bare', 'ovf', 'ova'],
+                'type': 'string',
+                'description': 'Format of the container'},
+            'disk_format': {
+                'enum': [None, 'ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw',
+                         'qcow2', 'vdi', 'iso'],
+                'type': 'string',
+                'description': 'Format of the disk'},
+            'location': {'type': 'string'},
+            'locations': {'type': 'string'},
+            'copy_from': {'type': 'string'}}}
+    return original_schema_args(my_schema_getter, omit)
+utils.schema_args = schema_args
+
 from glanceclient.v2 import shell as test_shell
 
+# Return original decorator.
+utils.schema_args = original_schema_args
+
 
 class ShellV2Test(testtools.TestCase):
     def setUp(self):
         super(ShellV2Test, self).setUp()
         self._mock_utils()
         self.gc = self._mock_glance_client()
+        self.shell = shell.OpenStackImagesShell()
+        os.environ = {
+            'OS_USERNAME': 'username',
+            'OS_PASSWORD': 'password',
+            'OS_TENANT_ID': 'tenant_id',
+            'OS_TOKEN_ID': 'test',
+            'OS_AUTH_URL': 'http://127.0.0.1:5000/v2.0/',
+            'OS_AUTH_TOKEN': 'pass',
+            'OS_IMAGE_API_VERSION': '1',
+            'OS_REGION_NAME': 'test',
+            'OS_IMAGE_URL': 'http://is.invalid'}
+        self.shell = shell.OpenStackImagesShell()
+        self.patched = mock.patch('glanceclient.common.utils.get_data_file',
+                                  autospec=True, return_value=None)
+        self.mock_get_data_file = self.patched.start()
+
+    def tearDown(self):
+        super(ShellV2Test, self).tearDown()
+        self.patched.stop()
 
     def _make_args(self, args):
         # NOTE(venkatesh): this conversion from a dict to an object
@@ -59,6 +114,49 @@ class ShellV2Test(testtools.TestCase):
 
             mocked_utils_exit.assert_called_once_with(err_msg)
 
+    def _run_command(self, cmd):
+        self.shell.main(cmd.split())
+
+    @mock.patch('sys.stderr')
+    def test_image_create_missing_disk_format(self, __):
+        # We test for all possible sources
+        for origin in ('--file', '--location', '--copy-from'):
+            e = self.assertRaises(exc.CommandError, self._run_command,
+                                  '--os-image-api-version 2 image-create ' +
+                                  origin + ' fake_src --container-format bare')
+            self.assertEqual('error: Must provide --disk-format when using '
+                             + origin + '.', e.message)
+
+    @mock.patch('sys.stderr')
+    def test_image_create_missing_container_format(self, __):
+        # We test for all possible sources
+        for origin in ('--file', '--location', '--copy-from'):
+            e = self.assertRaises(exc.CommandError, self._run_command,
+                                  '--os-image-api-version 2 image-create ' +
+                                  origin + ' fake_src --disk-format qcow2')
+            self.assertEqual('error: Must provide --container-format when '
+                             'using ' + origin + '.', e.message)
+
+    @mock.patch('sys.stderr')
+    def test_image_create_missing_container_format_stdin_data(self, __):
+        # Fake that get_data_file method returns data
+        self.mock_get_data_file.return_value = six.StringIO()
+        e = self.assertRaises(exc.CommandError, self._run_command,
+                              '--os-image-api-version 2 image-create'
+                              ' --disk-format qcow2')
+        self.assertEqual('error: Must provide --container-format when '
+                         'using stdin.', e.message)
+
+    @mock.patch('sys.stderr')
+    def test_image_create_missing_disk_format_stdin_data(self, __):
+        # Fake that get_data_file method returns data
+        self.mock_get_data_file.return_value = six.StringIO()
+        e = self.assertRaises(exc.CommandError, self._run_command,
+                              '--os-image-api-version 2 image-create'
+                              ' --container-format bare')
+        self.assertEqual('error: Must provide --disk-format when using stdin.',
+                         e.message)
+
     def test_do_image_list(self):
         input = {
             'limit': None,
@@ -261,6 +359,7 @@ class ShellV2Test(testtools.TestCase):
                 'container_format': 'bare'})
 
     def test_do_image_create_with_file(self):
+        self.mock_get_data_file.return_value = six.StringIO()
         try:
             file_name = None
             with open(tempfile.mktemp(), 'w+') as f:
@@ -305,7 +404,9 @@ class ShellV2Test(testtools.TestCase):
     def test_do_image_create_with_user_props(self, mock_stdin):
         args = self._make_args({'name': 'IMG-01',
                                 'property': ['myprop=myval'],
-                                'file': None})
+                                'file': None,
+                                'container_format': 'bare',
+                                'disk_format': 'qcow2'})
         with mock.patch.object(self.gc.images, 'create') as mocked_create:
             ignore_fields = ['self', 'access', 'file', 'schema']
             expect_image = dict([(field, field) for field in ignore_fields])
@@ -320,7 +421,9 @@ class ShellV2Test(testtools.TestCase):
             test_shell.do_image_create(self.gc, args)
 
             mocked_create.assert_called_once_with(name='IMG-01',
-                                                  myprop='myval')
+                                                  myprop='myval',
+                                                  container_format='bare',
+                                                  disk_format='qcow2')
             utils.print_dict.assert_called_once_with({
                 'id': 'pass', 'name': 'IMG-01', 'myprop': 'myval'})
 
diff --git a/glanceclient/v1/shell.py b/glanceclient/v1/shell.py
index 83b8576..c28dc23 100644
--- a/glanceclient/v1/shell.py
+++ b/glanceclient/v1/shell.py
@@ -32,6 +32,7 @@ import glanceclient.v1.images
 CONTAINER_FORMATS = 'Acceptable formats: ami, ari, aki, bare, and ovf.'
 DISK_FORMATS = ('Acceptable formats: ami, ari, aki, vhd, vmdk, raw, '
                 'qcow2, vdi, and iso.')
+DATA_FIELDS = ('location', 'copy_from', 'file')
 
 _bool_strict = functools.partial(strutils.bool_from_string, strict=True)
 
@@ -221,6 +222,7 @@ def do_image_download(gc, args):
            help='Print image size in a human-friendly format.')
 @utils.arg('--progress', action='store_true', default=False,
            help='Show upload progress bar.')
+@utils.on_data_require_fields(DATA_FIELDS)
 def do_image_create(gc, args):
     """Create a new image."""
     # Filter out None values
diff --git a/glanceclient/v2/shell.py b/glanceclient/v2/shell.py
index 2f6122a..deea24b 100644
--- a/glanceclient/v2/shell.py
+++ b/glanceclient/v2/shell.py
@@ -27,6 +27,7 @@ import os
 
 MEMBER_STATUS_VALUES = image_members.MEMBER_STATUS_VALUES
 IMAGE_SCHEMA = None
+DATA_FIELDS = ('location', 'copy_from', 'file')
 
 
 def get_image_schema():
@@ -55,6 +56,7 @@ def get_image_schema():
                 'to the client via stdin.')
 @utils.arg('--progress', action='store_true', default=False,
            help='Show upload progress bar.')
+@utils.on_data_require_fields(DATA_FIELDS)
 def do_image_create(gc, args):
     """Create a new image."""
     schema = gc.schemas.get("image")