From f4748f4d250d4d8eb592b7170f1e020f9ece8df3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Douglas=20Mendiz=C3=A1bal?= Date: Wed, 31 May 2023 16:00:02 -0500 Subject: [PATCH] Fix "access rule" commands to only use ID This patch modifies the access rule commands to use only the resource ID. The previous logic incorrectly assumed that access rules have a "name" property, which resulted in unexpected behaviors. For example, "access rule delete {non-existent-id}" now results in a "not found" error instead of sometimes deleting an unrelated rule. Story: 2010775 Task: 48163 Change-Id: Ib5c3b7f86acf1dfe7cc76dfa99fa4c118388bd71 (cherry picked from commit 48148e7a1197c92febdb3ab1e72085c4eb3d6bf7) --- openstackclient/identity/common.py | 12 ++++++++++ openstackclient/identity/v3/access_rule.py | 14 +++++++----- .../unit/identity/v3/test_access_rule.py | 22 +++++++++---------- .../fix-story-2010775-953dbdf03b2b6746.yaml | 8 +++++++ 4 files changed, 39 insertions(+), 17 deletions(-) create mode 100644 releasenotes/notes/fix-story-2010775-953dbdf03b2b6746.yaml diff --git a/openstackclient/identity/common.py b/openstackclient/identity/common.py index a75db4f8dd..116d8ad8a9 100644 --- a/openstackclient/identity/common.py +++ b/openstackclient/identity/common.py @@ -87,6 +87,18 @@ def get_resource(manager, name_type_or_id): raise exceptions.CommandError(msg % name_type_or_id) +def get_resource_by_id(manager, resource_id): + """Get resource by ID + + Raises CommandError if the resource is not found + """ + try: + return manager.get(resource_id) + except identity_exc.NotFound: + msg = _("Resource with id {} not found") + raise exceptions.CommandError(msg.format(resource_id)) + + def _get_token_resource(client, resource, parsed_name, parsed_domain=None): """Peek into the user's auth token to get resource IDs diff --git a/openstackclient/identity/v3/access_rule.py b/openstackclient/identity/v3/access_rule.py index ffda04f9e5..e57e1b6178 100644 --- a/openstackclient/identity/v3/access_rule.py +++ b/openstackclient/identity/v3/access_rule.py @@ -37,7 +37,7 @@ class DeleteAccessRule(command.Command): 'access_rule', metavar='', nargs="+", - help=_('Access rule(s) to delete (name or ID)'), + help=_('Access rule ID(s) to delete'), ) return parser @@ -47,8 +47,9 @@ class DeleteAccessRule(command.Command): errors = 0 for ac in parsed_args.access_rule: try: - access_rule = utils.find_resource( - identity_client.access_rules, ac) + access_rule = common.get_resource_by_id( + identity_client.access_rules, ac + ) identity_client.access_rules.delete(access_rule.id) except Exception as e: errors += 1 @@ -103,14 +104,15 @@ class ShowAccessRule(command.ShowOne): parser.add_argument( 'access_rule', metavar='', - help=_('Access rule to display (name or ID)'), + help=_('Access rule ID to display'), ) return parser def take_action(self, parsed_args): identity_client = self.app.client_manager.identity - access_rule = utils.find_resource(identity_client.access_rules, - parsed_args.access_rule) + access_rule = common.get_resource_by_id( + identity_client.access_rules, parsed_args.access_rule + ) access_rule._info.pop('links', None) diff --git a/openstackclient/tests/unit/identity/v3/test_access_rule.py b/openstackclient/tests/unit/identity/v3/test_access_rule.py index 904fe323d9..ed957a90c8 100644 --- a/openstackclient/tests/unit/identity/v3/test_access_rule.py +++ b/openstackclient/tests/unit/identity/v3/test_access_rule.py @@ -14,10 +14,9 @@ # import copy -from unittest import mock +from keystoneclient import exceptions as identity_exc from osc_lib import exceptions -from osc_lib import utils from openstackclient.identity.v3 import access_rule from openstackclient.tests.unit import fakes @@ -69,10 +68,13 @@ class TestAccessRuleDelete(TestAccessRule): ) self.assertIsNone(result) - @mock.patch.object(utils, 'find_resource') - def test_delete_multi_access_rules_with_exception(self, find_mock): - find_mock.side_effect = [self.access_rules_mock.get.return_value, - exceptions.CommandError] + def test_delete_multi_access_rules_with_exception(self): + # mock returns for common.get_resource_by_id + mock_get = self.access_rules_mock.get + mock_get.side_effect = [ + mock_get.return_value, + identity_exc.NotFound, + ] arglist = [ identity_fakes.access_rule_id, 'nonexistent_access_rule', @@ -89,12 +91,10 @@ class TestAccessRuleDelete(TestAccessRule): self.assertEqual('1 of 2 access rules failed to' ' delete.', str(e)) - find_mock.assert_any_call(self.access_rules_mock, - identity_fakes.access_rule_id) - find_mock.assert_any_call(self.access_rules_mock, - 'nonexistent_access_rule') + mock_get.assert_any_call(identity_fakes.access_rule_id) + mock_get.assert_any_call('nonexistent_access_rule') - self.assertEqual(2, find_mock.call_count) + self.assertEqual(2, mock_get.call_count) self.access_rules_mock.delete.assert_called_once_with( identity_fakes.access_rule_id) diff --git a/releasenotes/notes/fix-story-2010775-953dbdf03b2b6746.yaml b/releasenotes/notes/fix-story-2010775-953dbdf03b2b6746.yaml new file mode 100644 index 0000000000..e4c98b7443 --- /dev/null +++ b/releasenotes/notes/fix-story-2010775-953dbdf03b2b6746.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixed a bug in "access rule" subcommands where the client logic incorrectly + assumed that access rules have a "name" property which resulted in + unpredictable behaviors. e.g. "access rule delete {non-existent-id}" now + results in a not-found error instead of sometimes deleting an unrelated + rule.