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).