From 717f242881ff58f1ddec994ffc68b82103ca7dab Mon Sep 17 00:00:00 2001
From: Antonia Gaete <antoniagaete@osuosl.org>
Date: Tue, 23 Jan 2024 18:35:49 +0000
Subject: [PATCH] identity: Migrate 'service' commands to SDK

Change-Id: I37d07a6c5cdc98680b8d65d596521cad2b049500
---
 openstackclient/identity/common.py            |  34 ++++
 openstackclient/identity/v3/service.py        |  70 +++++---
 .../tests/functional/identity/v3/common.py    |   4 +-
 .../functional/identity/v3/test_limit.py      |   4 +-
 .../identity/v3/test_registered_limit.py      |   2 +-
 .../functional/identity/v3/test_service.py    |   6 +-
 .../tests/unit/identity/v3/test_service.py    | 150 +++++++++---------
 ...grate-service-to-sdk-6ff62ebf7e41db7c.yaml |  10 ++
 8 files changed, 179 insertions(+), 101 deletions(-)
 create mode 100644 releasenotes/notes/migrate-service-to-sdk-6ff62ebf7e41db7c.yaml

diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py
index a9d8afa6f5..4e5f083de2 100644
--- a/openstackclient/identity/common.py
+++ b/openstackclient/identity/common.py
@@ -20,6 +20,7 @@ from keystoneclient.v3 import domains
 from keystoneclient.v3 import groups
 from keystoneclient.v3 import projects
 from keystoneclient.v3 import users
+from openstack import exceptions as sdk_exceptions
 from osc_lib import exceptions
 from osc_lib import utils
 
@@ -73,6 +74,39 @@ def find_service(identity_client, name_type_or_id):
         raise exceptions.CommandError(msg % name_type_or_id)
 
 
+def find_service_sdk(identity_client, name_type_or_id):
+    """Find a service by id, name or type."""
+
+    try:
+        # search for service name or ID
+        return identity_client.find_service(
+            name_type_or_id, ignore_missing=False
+        )
+    except sdk_exceptions.ResourceNotFound:
+        pass
+    except sdk_exceptions.DuplicateResource as e:
+        raise exceptions.CommandError(e.message)
+
+    # search for service type
+    services = identity_client.services()
+    result = None
+    for service in services:
+        if name_type_or_id == service.type:
+            if result:
+                msg = _(
+                    "Multiple service matches found for '%s', "
+                    "use an ID or name to be more specific."
+                )
+                raise exceptions.CommandError(msg % name_type_or_id)
+            result = service
+
+    if result is None:
+        msg = _("No service with a type, name or ID of '%s' exists.")
+        raise exceptions.CommandError(msg % name_type_or_id)
+
+    return result
+
+
 def get_resource(manager, name_type_or_id):
     # NOTE (vishakha): Due to bug #1799153 and for any another related case
     # where GET resource API does not support the filter by name,
diff --git a/openstackclient/identity/v3/service.py b/openstackclient/identity/v3/service.py
index 42ee1e474d..b7d4befc96 100644
--- a/openstackclient/identity/v3/service.py
+++ b/openstackclient/identity/v3/service.py
@@ -28,6 +28,31 @@ from openstackclient.identity import common
 LOG = logging.getLogger(__name__)
 
 
+def _format_service(service):
+    columns = (
+        'id',
+        'name',
+        'type',
+        'is_enabled',
+        'description',
+    )
+    column_headers = (
+        'ID',
+        'Name',
+        'Type',
+        'Enabled',
+        'Description',
+    )
+
+    return (
+        column_headers,
+        utils.get_item_properties(
+            service,
+            columns,
+        ),
+    )
+
+
 class CreateService(command.ShowOne):
     _description = _("Create new service")
 
@@ -66,17 +91,16 @@ class CreateService(command.ShowOne):
         return parser
 
     def take_action(self, parsed_args):
-        identity_client = self.app.client_manager.identity
+        identity_client = self.app.client_manager.sdk_connection.identity
 
-        service = identity_client.services.create(
+        service = identity_client.create_service(
             name=parsed_args.name,
             type=parsed_args.type,
             description=parsed_args.description,
-            enabled=parsed_args.is_enabled,
+            is_enabled=parsed_args.is_enabled,
         )
 
-        service._info.pop('links')
-        return zip(*sorted(service._info.items()))
+        return _format_service(service)
 
 
 class DeleteService(command.Command):
@@ -93,12 +117,12 @@ class DeleteService(command.Command):
         return parser
 
     def take_action(self, parsed_args):
-        identity_client = self.app.client_manager.identity
+        identity_client = self.app.client_manager.sdk_connection.identity
         result = 0
         for i in parsed_args.service:
             try:
-                service = common.find_service(identity_client, i)
-                identity_client.services.delete(service.id)
+                service = common.find_service_sdk(identity_client, i)
+                identity_client.delete_service(service.id)
             except Exception as e:
                 result += 1
                 LOG.error(
@@ -131,13 +155,19 @@ class ListService(command.Lister):
         return parser
 
     def take_action(self, parsed_args):
+        identity_client = self.app.client_manager.sdk_connection.identity
+
         if parsed_args.long:
-            columns = ('ID', 'Name', 'Type', 'Description', 'Enabled')
+            columns = ('id', 'name', 'type', 'description', 'is_enabled')
+            column_headers = ('ID', 'Name', 'Type', 'Description', 'Enabled')
         else:
-            columns = ('ID', 'Name', 'Type')
-        data = self.app.client_manager.identity.services.list()
+            columns = ('id', 'name', 'type')
+            column_headers = ('ID', 'Name', 'Type')
+
+        data = identity_client.services()
+
         return (
-            columns,
+            column_headers,
             (utils.get_item_properties(s, columns) for s in data),
         )
 
@@ -185,9 +215,9 @@ class SetService(command.Command):
         return parser
 
     def take_action(self, parsed_args):
-        identity_client = self.app.client_manager.identity
+        identity_client = self.app.client_manager.sdk_connection.identity
 
-        service = common.find_service(identity_client, parsed_args.service)
+        service = common.find_service_sdk(identity_client, parsed_args.service)
         kwargs = {}
         if parsed_args.type:
             kwargs['type'] = parsed_args.type
@@ -195,10 +225,9 @@ class SetService(command.Command):
             kwargs['name'] = parsed_args.name
         if parsed_args.description:
             kwargs['description'] = parsed_args.description
-        if parsed_args.is_enabled is not None:
-            kwargs['enabled'] = parsed_args.is_enabled
+        kwargs['is_enabled'] = parsed_args.is_enabled
 
-        identity_client.services.update(service.id, **kwargs)
+        identity_client.update_service(service.id, **kwargs)
 
 
 class ShowService(command.ShowOne):
@@ -214,9 +243,8 @@ class ShowService(command.ShowOne):
         return parser
 
     def take_action(self, parsed_args):
-        identity_client = self.app.client_manager.identity
+        identity_client = self.app.client_manager.sdk_connection.identity
 
-        service = common.find_service(identity_client, parsed_args.service)
+        service = common.find_service_sdk(identity_client, parsed_args.service)
 
-        service._info.pop('links')
-        return zip(*sorted(service._info.items()))
+        return _format_service(service)
diff --git a/openstackclient/tests/functional/identity/v3/common.py b/openstackclient/tests/functional/identity/v3/common.py
index 8607c05731..7ae56b8de1 100644
--- a/openstackclient/tests/functional/identity/v3/common.py
+++ b/openstackclient/tests/functional/identity/v3/common.py
@@ -48,7 +48,7 @@ class IdentityTests(base.TestCase):
         'parent_id',
     ]
     ROLE_FIELDS = ['id', 'name', 'domain_id', 'description']
-    SERVICE_FIELDS = ['id', 'enabled', 'name', 'type', 'description']
+    SERVICE_FIELDS = ['ID', 'Enabled', 'Name', 'Type', 'Description']
     REGION_FIELDS = ['description', 'enabled', 'parent_region', 'region']
     ENDPOINT_FIELDS = [
         'id',
@@ -367,7 +367,7 @@ class IdentityTests(base.TestCase):
         if add_clean_up:
             service = self.parse_show_as_object(raw_output)
             self.addCleanup(
-                self.openstack, 'service delete %s' % service['id']
+                self.openstack, 'service delete %s' % service['ID']
             )
         items = self.parse_show(raw_output)
         self.assert_show_fields(items, self.SERVICE_FIELDS)
diff --git a/openstackclient/tests/functional/identity/v3/test_limit.py b/openstackclient/tests/functional/identity/v3/test_limit.py
index 32c65107d0..d096762d2e 100644
--- a/openstackclient/tests/functional/identity/v3/test_limit.py
+++ b/openstackclient/tests/functional/identity/v3/test_limit.py
@@ -32,7 +32,7 @@ class LimitTestCase(common.IdentityTests):
 
         raw_output = self.openstack('service show %s' % service_id)
         items = self.parse_show(raw_output)
-        service_name = self._extract_value_from_items('name', items)
+        service_name = self._extract_value_from_items('Name', items)
 
         project_name = self._create_dummy_project()
         raw_output = self.openstack('project show %s' % project_name)
@@ -73,7 +73,7 @@ class LimitTestCase(common.IdentityTests):
 
         raw_output = self.openstack('service show %s' % service_id)
         items = self.parse_show(raw_output)
-        service_name = self._extract_value_from_items('name', items)
+        service_name = self._extract_value_from_items('Name', items)
 
         project_name = self._create_dummy_project()
 
diff --git a/openstackclient/tests/functional/identity/v3/test_registered_limit.py b/openstackclient/tests/functional/identity/v3/test_registered_limit.py
index 6e3c30aeb0..ba7f8cbd4d 100644
--- a/openstackclient/tests/functional/identity/v3/test_registered_limit.py
+++ b/openstackclient/tests/functional/identity/v3/test_registered_limit.py
@@ -29,7 +29,7 @@ class RegisteredLimitTestCase(common.IdentityTests):
             'service show' ' %(service_name)s' % {'service_name': service_name}
         )
         service_items = self.parse_show(raw_output)
-        service_id = self._extract_value_from_items('id', service_items)
+        service_id = self._extract_value_from_items('ID', service_items)
 
         raw_output = self.openstack(
             'registered limit create'
diff --git a/openstackclient/tests/functional/identity/v3/test_service.py b/openstackclient/tests/functional/identity/v3/test_service.py
index 98fa349d1e..76009bf48b 100644
--- a/openstackclient/tests/functional/identity/v3/test_service.py
+++ b/openstackclient/tests/functional/identity/v3/test_service.py
@@ -61,9 +61,9 @@ class ServiceTests(common.IdentityTests):
         raw_output = self.openstack('service show %s' % new_service_name)
         # assert service details
         service = self.parse_show_as_object(raw_output)
-        self.assertEqual(new_service_type, service['type'])
-        self.assertEqual(new_service_name, service['name'])
-        self.assertEqual(new_service_description, service['description'])
+        self.assertEqual(new_service_type, service['Type'])
+        self.assertEqual(new_service_name, service['Name'])
+        self.assertEqual(new_service_description, service['Description'])
 
     def test_service_show(self):
         service_name = self._create_dummy_service()
diff --git a/openstackclient/tests/unit/identity/v3/test_service.py b/openstackclient/tests/unit/identity/v3/test_service.py
index 57de1a1f7a..d447121244 100644
--- a/openstackclient/tests/unit/identity/v3/test_service.py
+++ b/openstackclient/tests/unit/identity/v3/test_service.py
@@ -13,43 +13,37 @@
 #   under the License.
 #
 
-from keystoneclient import exceptions as identity_exc
+from openstack import exceptions as sdk_exceptions
+from openstack.identity.v3 import service as _service
+from openstack.test import fakes as sdk_fakes
 from osc_lib import exceptions
 
 from openstackclient.identity.v3 import service
 from openstackclient.tests.unit.identity.v3 import fakes as identity_fakes
 
 
-class TestService(identity_fakes.TestIdentityv3):
-    def setUp(self):
-        super().setUp()
-
-        # Get a shortcut to the ServiceManager Mock
-        self.services_mock = self.identity_client.services
-        self.services_mock.reset_mock()
-
-
-class TestServiceCreate(TestService):
+class TestServiceCreate(identity_fakes.TestIdentityv3):
     columns = (
-        'description',
-        'enabled',
-        'id',
-        'name',
-        'type',
+        'ID',
+        'Name',
+        'Type',
+        'Enabled',
+        'Description',
     )
 
     def setUp(self):
         super().setUp()
 
-        self.service = identity_fakes.FakeService.create_one_service()
+        self.service = sdk_fakes.generate_fake_resource(_service.Service)
+
         self.datalist = (
-            self.service.description,
-            True,
             self.service.id,
             self.service.name,
             self.service.type,
+            True,
+            self.service.description,
         )
-        self.services_mock.create.return_value = self.service
+        self.identity_sdk_client.create_service.return_value = self.service
 
         # Get the command object to test
         self.cmd = service.CreateService(self.app, None)
@@ -73,12 +67,11 @@ class TestServiceCreate(TestService):
         # data to be shown.
         columns, data = self.cmd.take_action(parsed_args)
 
-        # ServiceManager.create(name=, type=, enabled=, **kwargs)
-        self.services_mock.create.assert_called_with(
+        self.identity_sdk_client.create_service.assert_called_with(
             name=self.service.name,
             type=self.service.type,
             description=None,
-            enabled=True,
+            is_enabled=True,
         )
 
         self.assertEqual(self.columns, columns)
@@ -103,12 +96,11 @@ class TestServiceCreate(TestService):
         # data to be shown.
         columns, data = self.cmd.take_action(parsed_args)
 
-        # ServiceManager.create(name=, type=, enabled=, **kwargs)
-        self.services_mock.create.assert_called_with(
+        self.identity_sdk_client.create_service.assert_called_with(
             name=None,
             type=self.service.type,
             description=self.service.description,
-            enabled=True,
+            is_enabled=True,
         )
 
         self.assertEqual(self.columns, columns)
@@ -132,12 +124,11 @@ class TestServiceCreate(TestService):
         # data to be shown.
         columns, data = self.cmd.take_action(parsed_args)
 
-        # ServiceManager.create(name=, type=, enabled=, **kwargs)
-        self.services_mock.create.assert_called_with(
+        self.identity_sdk_client.create_service.assert_called_with(
             name=None,
             type=self.service.type,
             description=None,
-            enabled=True,
+            is_enabled=True,
         )
 
         self.assertEqual(self.columns, columns)
@@ -161,27 +152,28 @@ class TestServiceCreate(TestService):
         # data to be shown.
         columns, data = self.cmd.take_action(parsed_args)
 
-        # ServiceManager.create(name=, type=, enabled=, **kwargs)
-        self.services_mock.create.assert_called_with(
+        self.identity_sdk_client.create_service.assert_called_with(
             name=None,
             type=self.service.type,
             description=None,
-            enabled=False,
+            is_enabled=False,
         )
 
         self.assertEqual(self.columns, columns)
         self.assertEqual(self.datalist, data)
 
 
-class TestServiceDelete(TestService):
-    service = identity_fakes.FakeService.create_one_service()
+class TestServiceDelete(identity_fakes.TestIdentityv3):
+    service = sdk_fakes.generate_fake_resource(_service.Service)
 
     def setUp(self):
         super().setUp()
 
-        self.services_mock.get.side_effect = identity_exc.NotFound(None)
-        self.services_mock.find.return_value = self.service
-        self.services_mock.delete.return_value = None
+        self.identity_sdk_client.get_service.side_effect = (
+            sdk_exceptions.ResourceNotFound
+        )
+        self.identity_sdk_client.find_service.return_value = self.service
+        self.identity_sdk_client.delete_service.return_value = None
 
         # Get the command object to test
         self.cmd = service.DeleteService(self.app, None)
@@ -197,19 +189,19 @@ class TestServiceDelete(TestService):
 
         result = self.cmd.take_action(parsed_args)
 
-        self.services_mock.delete.assert_called_with(
+        self.identity_sdk_client.delete_service.assert_called_with(
             self.service.id,
         )
         self.assertIsNone(result)
 
 
-class TestServiceList(TestService):
-    service = identity_fakes.FakeService.create_one_service()
+class TestServiceList(identity_fakes.TestIdentityv3):
+    service = sdk_fakes.generate_fake_resource(_service.Service)
 
     def setUp(self):
         super().setUp()
 
-        self.services_mock.list.return_value = [self.service]
+        self.identity_sdk_client.services.return_value = [self.service]
 
         # Get the command object to test
         self.cmd = service.ListService(self.app, None)
@@ -224,7 +216,7 @@ class TestServiceList(TestService):
         # containing the data to be listed.
         columns, data = self.cmd.take_action(parsed_args)
 
-        self.services_mock.list.assert_called_with()
+        self.identity_sdk_client.services.assert_called_with()
 
         collist = ('ID', 'Name', 'Type')
         self.assertEqual(collist, columns)
@@ -251,7 +243,7 @@ class TestServiceList(TestService):
         # containing the data to be listed.
         columns, data = self.cmd.take_action(parsed_args)
 
-        self.services_mock.list.assert_called_with()
+        self.identity_sdk_client.services.assert_called_with()
 
         collist = ('ID', 'Name', 'Type', 'Description', 'Enabled')
         self.assertEqual(collist, columns)
@@ -267,15 +259,17 @@ class TestServiceList(TestService):
         self.assertEqual(datalist, tuple(data))
 
 
-class TestServiceSet(TestService):
-    service = identity_fakes.FakeService.create_one_service()
+class TestServiceSet(identity_fakes.TestIdentityv3):
+    service = sdk_fakes.generate_fake_resource(_service.Service)
 
     def setUp(self):
         super().setUp()
 
-        self.services_mock.get.side_effect = identity_exc.NotFound(None)
-        self.services_mock.find.return_value = self.service
-        self.services_mock.update.return_value = self.service
+        self.identity_sdk_client.get_service.side_effect = (
+            sdk_exceptions.ResourceNotFound
+        )
+        self.identity_sdk_client.find_service.return_value = self.service
+        self.identity_sdk_client.update_service.return_value = self.service
 
         # Get the command object to test
         self.cmd = service.SetService(self.app, None)
@@ -317,9 +311,11 @@ class TestServiceSet(TestService):
         # Set expected values
         kwargs = {
             'type': self.service.type,
+            'is_enabled': None,
         }
-        # ServiceManager.update(service, name=, type=, enabled=, **kwargs)
-        self.services_mock.update.assert_called_with(self.service.id, **kwargs)
+        self.identity_sdk_client.update_service.assert_called_with(
+            self.service.id, **kwargs
+        )
         self.assertIsNone(result)
 
     def test_service_set_name(self):
@@ -342,9 +338,11 @@ class TestServiceSet(TestService):
         # Set expected values
         kwargs = {
             'name': self.service.name,
+            'is_enabled': None,
         }
-        # ServiceManager.update(service, name=, type=, enabled=, **kwargs)
-        self.services_mock.update.assert_called_with(self.service.id, **kwargs)
+        self.identity_sdk_client.update_service.assert_called_with(
+            self.service.id, **kwargs
+        )
         self.assertIsNone(result)
 
     def test_service_set_description(self):
@@ -367,9 +365,11 @@ class TestServiceSet(TestService):
         # Set expected values
         kwargs = {
             'description': self.service.description,
+            'is_enabled': None,
         }
-        # ServiceManager.update(service, name=, type=, enabled=, **kwargs)
-        self.services_mock.update.assert_called_with(self.service.id, **kwargs)
+        self.identity_sdk_client.update_service.assert_called_with(
+            self.service.id, **kwargs
+        )
         self.assertIsNone(result)
 
     def test_service_set_enable(self):
@@ -390,10 +390,11 @@ class TestServiceSet(TestService):
 
         # Set expected values
         kwargs = {
-            'enabled': True,
+            'is_enabled': True,
         }
-        # ServiceManager.update(service, name=, type=, enabled=, **kwargs)
-        self.services_mock.update.assert_called_with(self.service.id, **kwargs)
+        self.identity_sdk_client.update_service.assert_called_with(
+            self.service.id, **kwargs
+        )
         self.assertIsNone(result)
 
     def test_service_set_disable(self):
@@ -414,21 +415,24 @@ class TestServiceSet(TestService):
 
         # Set expected values
         kwargs = {
-            'enabled': False,
+            'is_enabled': False,
         }
-        # ServiceManager.update(service, name=, type=, enabled=, **kwargs)
-        self.services_mock.update.assert_called_with(self.service.id, **kwargs)
+        self.identity_sdk_client.update_service.assert_called_with(
+            self.service.id, **kwargs
+        )
         self.assertIsNone(result)
 
 
-class TestServiceShow(TestService):
-    service = identity_fakes.FakeService.create_one_service()
+class TestServiceShow(identity_fakes.TestIdentityv3):
+    service = sdk_fakes.generate_fake_resource(_service.Service)
 
     def setUp(self):
         super().setUp()
 
-        self.services_mock.get.side_effect = identity_exc.NotFound(None)
-        self.services_mock.find.return_value = self.service
+        self.identity_sdk_client.get_service.side_effect = (
+            sdk_exceptions.ResourceNotFound
+        )
+        self.identity_sdk_client.find_service.return_value = self.service
 
         # Get the command object to test
         self.cmd = service.ShowService(self.app, None)
@@ -447,22 +451,25 @@ class TestServiceShow(TestService):
         # data to be shown.
         columns, data = self.cmd.take_action(parsed_args)
 
-        # ServiceManager.get(id)
-        self.services_mock.find.assert_called_with(name=self.service.name)
+        self.identity_sdk_client.find_service.assert_called_with(
+            self.service.name, ignore_missing=False
+        )
 
-        collist = ('description', 'enabled', 'id', 'name', 'type')
+        collist = ('ID', 'Name', 'Type', 'Enabled', 'Description')
         self.assertEqual(collist, columns)
         datalist = (
-            self.service.description,
-            True,
             self.service.id,
             self.service.name,
             self.service.type,
+            True,
+            self.service.description,
         )
         self.assertEqual(datalist, data)
 
     def test_service_show_nounique(self):
-        self.services_mock.find.side_effect = identity_exc.NoUniqueMatch(None)
+        self.identity_sdk_client.find_service.side_effect = (
+            sdk_exceptions.DuplicateResource(None)
+        )
         arglist = [
             'nounique_service',
         ]
@@ -476,7 +483,6 @@ class TestServiceShow(TestService):
             self.fail('CommandError should be raised.')
         except exceptions.CommandError as e:
             self.assertEqual(
-                "Multiple service matches found for 'nounique_service',"
-                " use an ID to be more specific.",
+                "DuplicateResource",
                 str(e),
             )
diff --git a/releasenotes/notes/migrate-service-to-sdk-6ff62ebf7e41db7c.yaml b/releasenotes/notes/migrate-service-to-sdk-6ff62ebf7e41db7c.yaml
new file mode 100644
index 0000000000..90a55fcaab
--- /dev/null
+++ b/releasenotes/notes/migrate-service-to-sdk-6ff62ebf7e41db7c.yaml
@@ -0,0 +1,10 @@
+---
+features:
+  - |
+    The following commands have been migrated to SDK:
+
+    - ``service create``
+    - ``service delete``
+    - ``service set``
+    - ``service list``
+    - ``service show``