Merge "type should be required for v2.0 service create"

2 files changed, 104 insertions, 12 deletions

diff --git a/openstackclient/identity/v2_0/service.py b/openstackclient/identity/v2_0/service.py
index e8848dde..0b98a903 100644
--- a/openstackclient/identity/v2_0/service.py
+++ b/openstackclient/identity/v2_0/service.py
@@ -15,6 +15,7 @@
 
 """Service action implementations"""
 
+import argparse
 import logging
 import six
 
@@ -36,15 +37,20 @@ class CreateService(show.ShowOne):
     def get_parser(self, prog_name):
         parser = super(CreateService, self).get_parser(prog_name)
         parser.add_argument(
-            'name',
-            metavar='<service-name>',
-            help=_('New service name'),
+            'type_or_name',
+            metavar='<type>',
+            help=_('New service type (compute, image, identity, volume, etc)'),
         )
-        parser.add_argument(
+        type_or_name_group = parser.add_mutually_exclusive_group()
+        type_or_name_group.add_argument(
             '--type',
             metavar='<service-type>',
-            required=True,
-            help=_('New service type (compute, image, identity, volume, etc)'),
+            help=argparse.SUPPRESS,
+        )
+        type_or_name_group.add_argument(
+            '--name',
+            metavar='<name>',
+            help=_('New service name'),
         )
         parser.add_argument(
             '--description',
@@ -57,9 +63,28 @@ class CreateService(show.ShowOne):
         self.log.debug('take_action(%s)', parsed_args)
         identity_client = self.app.client_manager.identity
 
+        type_or_name = parsed_args.type_or_name
+        name = parsed_args.name
+        type = parsed_args.type
+
+        # If only a single positional is present, it's a <type>.
+        # This is not currently legal so it is considered a new case.
+        if not type and not name:
+            type = type_or_name
+        # If --type option is present then positional is handled as <name>;
+        # display deprecation message.
+        elif type:
+            name = type_or_name
+            self.log.warning(_('The argument --type is deprecated, use service'
+                               ' create --name <service-name> type instead.'))
+        # If --name option is present the positional is handled as <type>.
+        # Making --type optional is new, but back-compatible
+        elif name:
+            type = type_or_name
+
         service = identity_client.services.create(
-            parsed_args.name,
-            parsed_args.type,
+            name,
+            type,
             parsed_args.description)
 
         info = {}
diff --git a/openstackclient/tests/identity/v2_0/test_service.py b/openstackclient/tests/identity/v2_0/test_service.py
index 6c93574b..a0adea4e 100644
--- a/openstackclient/tests/identity/v2_0/test_service.py
+++ b/openstackclient/tests/identity/v2_0/test_service.py
@@ -44,14 +44,80 @@ class TestServiceCreate(TestService):
         # Get the command object to test
         self.cmd = service.CreateService(self.app, None)
 
-    def test_service_create_name_type(self):
+    def test_service_create_with_type_positional(self):
+        arglist = [
+            identity_fakes.service_type,
+        ]
+        verifylist = [
+            ('type_or_name', identity_fakes.service_type),
+            ('type', None),
+            ('description', None),
+            ('name', None),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # ServiceManager.create(name, service_type, description)
+        self.services_mock.create.assert_called_with(
+            None,
+            identity_fakes.service_type,
+            None,
+        )
+
+        collist = ('description', 'id', 'name', 'type')
+        self.assertEqual(columns, collist)
+        datalist = (
+            identity_fakes.service_description,
+            identity_fakes.service_id,
+            identity_fakes.service_name,
+            identity_fakes.service_type,
+        )
+        self.assertEqual(data, datalist)
+
+    def test_service_create_with_type_option(self):
         arglist = [
             '--type', identity_fakes.service_type,
             identity_fakes.service_name,
         ]
         verifylist = [
+            ('type_or_name', identity_fakes.service_name),
             ('type', identity_fakes.service_type),
             ('description', None),
+            ('name', None),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+
+        # DisplayCommandBase.take_action() returns two tuples
+        columns, data = self.cmd.take_action(parsed_args)
+
+        # ServiceManager.create(name, service_type, description)
+        self.services_mock.create.assert_called_with(
+            identity_fakes.service_name,
+            identity_fakes.service_type,
+            None,
+        )
+
+        collist = ('description', 'id', 'name', 'type')
+        self.assertEqual(columns, collist)
+        datalist = (
+            identity_fakes.service_description,
+            identity_fakes.service_id,
+            identity_fakes.service_name,
+            identity_fakes.service_type,
+        )
+        self.assertEqual(data, datalist)
+
+    def test_service_create_with_name_option(self):
+        arglist = [
+            '--name', identity_fakes.service_name,
+            identity_fakes.service_type,
+        ]
+        verifylist = [
+            ('type_or_name', identity_fakes.service_type),
+            ('type', None),
+            ('description', None),
             ('name', identity_fakes.service_name),
         ]
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
@@ -78,12 +144,13 @@ class TestServiceCreate(TestService):
 
     def test_service_create_description(self):
         arglist = [
-            '--type', identity_fakes.service_type,
+            '--name', identity_fakes.service_name,
             '--description', identity_fakes.service_description,
-            identity_fakes.service_name,
+            identity_fakes.service_type,
         ]
         verifylist = [
-            ('type', identity_fakes.service_type),
+            ('type_or_name', identity_fakes.service_type),
+            ('type', None),
             ('description', identity_fakes.service_description),
             ('name', identity_fakes.service_name),
         ]