From 31f7a5ada6775b0431ecdeaeb78f731566e9d9d6 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 16 Feb 2016 11:02:29 +0100 Subject: [PATCH] Drop rollback actions for set-XX and extend-XX rules actions Rollback actions were designed to help with rerunning introspection on the same node. However, rollback actions for these actions proved to be confusing and were never properly documented at all. Even worse, the rollback action for set-attribute actually makes this command impossible to use with non-removable attributes (e.g. /driver). This change removes rollback actions from all rules. We need to rethink how we handle rollback in rules later on. Change-Id: I2260f4b463c5dc804edac642c86e0da153e163f6 --- ironic_inspector/plugins/rules.py | 19 -------- ironic_inspector/test/functional.py | 5 --- ironic_inspector/test/test_plugins_rules.py | 44 ------------------- .../notes/no-rollback-e15bc7fee0134545.yaml | 10 +++++ 4 files changed, 10 insertions(+), 68 deletions(-) create mode 100644 releasenotes/notes/no-rollback-e15bc7fee0134545.yaml diff --git a/ironic_inspector/plugins/rules.py b/ironic_inspector/plugins/rules.py index ae4ab781d..abc6521a7 100644 --- a/ironic_inspector/plugins/rules.py +++ b/ironic_inspector/plugins/rules.py @@ -117,16 +117,6 @@ class SetAttributeAction(base.RuleActionPlugin): node_info.patch([{'op': 'add', 'path': params['path'], 'value': params['value']}]) - def rollback(self, node_info, params, **kwargs): - try: - node_info.get_by_path(params['path']) - except KeyError: - LOG.debug('Field %s was not set, no need for rollback', - params['path'], node_info=node_info) - return - - node_info.patch([{'op': 'remove', 'path': params['path']}]) - class SetCapabilityAction(base.RuleActionPlugin): REQUIRED_PARAMS = {'name'} @@ -136,9 +126,6 @@ class SetCapabilityAction(base.RuleActionPlugin): node_info.update_capabilities( **{params['name']: params.get('value')}) - def rollback(self, node_info, params, **kwargs): - node_info.update_capabilities(**{params['name']: None}) - class ExtendAttributeAction(base.RuleActionPlugin): REQUIRED_PARAMS = {'path', 'value'} @@ -153,9 +140,3 @@ class ExtendAttributeAction(base.RuleActionPlugin): return values node_info.replace_field(params['path'], _replace, default=[]) - - def rollback(self, node_info, params, **kwargs): - def _replace(values): - return [v for v in values if v != params['value']] - - node_info.replace_field(params['path'], _replace, default=[]) diff --git a/ironic_inspector/test/functional.py b/ironic_inspector/test/functional.py index 821bd3bcd..17a7a1f2d 100644 --- a/ironic_inspector/test/functional.py +++ b/ironic_inspector/test/functional.py @@ -333,11 +333,6 @@ class Test(Base): self.call_continue(self.data) eventlet.greenthread.sleep(DEFAULT_SLEEP) - # clean up for second rule - self.cli.node.update.assert_any_call( - self.uuid, - [{'op': 'remove', 'path': '/extra/bar'}]) - # applying first rule self.cli.node.update.assert_any_call( self.uuid, [{'op': 'add', 'path': '/extra/foo', 'value': 'bar'}]) diff --git a/ironic_inspector/test/test_plugins_rules.py b/ironic_inspector/test/test_plugins_rules.py index 16d650b35..4a20e96b6 100644 --- a/ironic_inspector/test/test_plugins_rules.py +++ b/ironic_inspector/test/test_plugins_rules.py @@ -144,19 +144,6 @@ class TestSetAttributeAction(test_base.NodeTest): 'path': '/extra/value', 'value': 42}]) - @mock.patch.object(node_cache.NodeInfo, 'patch') - def test_rollback_with_existing(self, mock_patch): - self.node.extra = {'value': 'value'} - self.act.rollback(self.node_info, self.params) - mock_patch.assert_called_once_with([{'op': 'remove', - 'path': '/extra/value'}]) - - @mock.patch.object(node_cache.NodeInfo, 'patch') - def test_rollback_no_existing(self, mock_patch): - self.node.extra = {} - self.act.rollback(self.node_info, self.params) - self.assertFalse(mock_patch.called) - class TestSetCapabilityAction(test_base.NodeTest): act = rules_plugins.SetCapabilityAction() @@ -182,23 +169,6 @@ class TestSetCapabilityAction(test_base.NodeTest): new_caps = utils.capabilities_to_dict(patch[0]['value']) self.assertEqual({'cap1': 'val', 'x': 'y', 'answer': '42'}, new_caps) - @mock.patch.object(node_cache.NodeInfo, 'patch') - def test_rollback_with_existing(self, mock_patch): - self.node.properties = {'capabilities': 'foo:bar,cap1:val'} - self.act.rollback(self.node_info, self.params) - mock_patch.assert_called_once_with( - [{'op': 'add', 'path': '/properties/capabilities', - 'value': 'foo:bar'}]) - - @mock.patch.object(node_cache.NodeInfo, 'patch') - def test_rollback_no_existing(self, mock_patch): - self.node.properties = {'capabilities': 'foo:bar'} - self.act.rollback(self.node_info, self.params) - # TODO(dtantsur): make sure it's not called at all - mock_patch.assert_called_once_with( - [{'op': 'add', 'path': '/properties/capabilities', - 'value': 'foo:bar'}]) - class TestExtendAttributeAction(test_base.NodeTest): act = rules_plugins.ExtendAttributeAction() @@ -228,17 +198,3 @@ class TestExtendAttributeAction(test_base.NodeTest): self.node.extra['value'] = [42] self.act.apply(self.node_info, params) self.assertFalse(mock_patch.called) - - @mock.patch.object(node_cache.NodeInfo, 'patch') - def test_rollback_with_existing(self, mock_patch): - self.node.extra['value'] = [1, 42, 0] - self.act.rollback(self.node_info, self.params) - - mock_patch.assert_called_once_with( - [{'op': 'replace', 'path': '/extra/value', 'value': [1, 0]}]) - - @mock.patch.object(node_cache.NodeInfo, 'patch') - def test_rollback_no_existing(self, mock_patch): - self.node.extra['value'] = [1, 0] - self.act.rollback(self.node_info, self.params) - self.assertFalse(mock_patch.called) diff --git a/releasenotes/notes/no-rollback-e15bc7fee0134545.yaml b/releasenotes/notes/no-rollback-e15bc7fee0134545.yaml new file mode 100644 index 000000000..e85cc1a3b --- /dev/null +++ b/releasenotes/notes/no-rollback-e15bc7fee0134545.yaml @@ -0,0 +1,10 @@ +--- +upgrade: + - Introspection rules actions 'set-attribute', 'set-capability' and + 'extend-attribute' no longer have the opposite effect on nodes that do not + match a rule. +fixes: + - Dropped rollback actions from 'set-attribute', 'set-capability' and + 'extend-attribute' introspection rules actions, as they were confusing, + completely undocumented and broke some real world use cases + (e.g. setting driver field).