From 0e1cac43dd8211186d6794602f6da77f612850a7 Mon Sep 17 00:00:00 2001 From: Bo Maryniuk Date: Tue, 21 Nov 2017 12:53:11 +0100 Subject: [PATCH] Bugfix the logic according to the exact described purpose of the function. Rename function from ambiguous name Fix and clarify docstring. Remove unused variable (no exception, within the try/finally block) Bugfix: do not pull '_errors' from unchecked objects Bugfix: unit test mistakenly expects pillar errors as a string, while it is a list Fix unit test: wrong error types in side effect Add unit test for _get_pillar_errors when external and internal pillars are clean Add unit test for _get_pillar_errors when external pillar has errors and internal is clean Add unit test for _get_pillar_errors when both external and internal pillars contains errors Add unit test for _get_pillar_errors when external pillar is clean and internal contains errors Use variable, instead of direct value --- salt/modules/state.py | 82 +++++++++++++++++++--------------------- tests/unit/modules/test_state.py | 75 +++++++++++++++++++++++++++++++----- 2 files changed, 103 insertions(+), 54 deletions(-) diff --git a/salt/modules/state.py b/salt/modules/state.py index fa5b997ef7..31ffc25dfe 100644 --- a/salt/modules/state.py +++ b/salt/modules/state.py @@ -99,17 +99,16 @@ def _set_retcode(ret, highstate=None): __context__['retcode'] = 2 -def _check_pillar(kwargs, pillar=None): +def _get_pillar_errors(kwargs, pillar=None): ''' - Check the pillar for errors, refuse to run the state if there are errors - in the pillar and return the pillar errors + Checks all pillars (external and internal) for errors. + Return an error message, if anywhere or None. + + :param kwargs: dictionary of options + :param pillar: external pillar + :return: None or an error message ''' - if kwargs.get('force'): - return True - pillar_dict = pillar if pillar is not None else __pillar__ - if '_errors' in pillar_dict: - return False - return True + return None if kwargs.get('force') else (pillar or {}).get('_errors', __pillar__.get('_errors')) or None def _wait(jid): @@ -411,10 +410,10 @@ def template(tem, queue=False, **kwargs): context=__context__, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - raise CommandExecutionError('Pillar failed to render', - info=st_.opts['pillar']['_errors']) + raise CommandExecutionError('Pillar failed to render', info=errors) if not tem.endswith('.sls'): tem = '{sls}.sls'.format(sls=tem) @@ -872,11 +871,10 @@ def highstate(test=None, queue=False, **kwargs): mocked=kwargs.get('mock', False), initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - err = ['Pillar failed to render with the following messages:'] - err += __pillar__['_errors'] - return err + return ['Pillar failed to render with the following messages:'] + errors st_.push_active() ret = {} @@ -1071,11 +1069,10 @@ def sls(mods, test=None, exclude=None, queue=False, **kwargs): mocked=kwargs.get('mock', False), initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - err = ['Pillar failed to render with the following messages:'] - err += __pillar__['_errors'] - return err + return ['Pillar failed to render with the following messages:'] + errors orchestration_jid = kwargs.get('orchestration_jid') umask = os.umask(0o77) @@ -1090,7 +1087,6 @@ def sls(mods, test=None, exclude=None, queue=False, **kwargs): mods = mods.split(',') st_.push_active() - ret = {} try: high_, errors = st_.render_highstate({opts['environment']: mods}) @@ -1197,11 +1193,10 @@ def top(topfn, test=None, queue=False, **kwargs): pillar_enc=pillar_enc, context=__context__, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - err = ['Pillar failed to render with the following messages:'] - err += __pillar__['_errors'] - return err + return ['Pillar failed to render with the following messages:'] + errors st_.push_active() st_.opts['state_top'] = salt.utils.url.create(topfn) @@ -1259,10 +1254,10 @@ def show_highstate(queue=False, **kwargs): pillar_enc=pillar_enc, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - raise CommandExecutionError('Pillar failed to render', - info=st_.opts['pillar']['_errors']) + raise CommandExecutionError('Pillar failed to render', info=errors) st_.push_active() try: @@ -1293,10 +1288,10 @@ def show_lowstate(queue=False, **kwargs): st_ = salt.state.HighState(opts, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - raise CommandExecutionError('Pillar failed to render', - info=st_.opts['pillar']['_errors']) + raise CommandExecutionError('Pillar failed to render', info=errors) st_.push_active() try: @@ -1394,11 +1389,10 @@ def sls_id(id_, mods, test=None, queue=False, **kwargs): st_ = salt.state.HighState(opts, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - err = ['Pillar failed to render with the following messages:'] - err += __pillar__['_errors'] - return err + return ['Pillar failed to render with the following messages:'] + errors if isinstance(mods, six.string_types): split_mods = mods.split(',') @@ -1474,10 +1468,10 @@ def show_low_sls(mods, test=None, queue=False, **kwargs): st_ = salt.state.HighState(opts, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - raise CommandExecutionError('Pillar failed to render', - info=st_.opts['pillar']['_errors']) + raise CommandExecutionError('Pillar failed to render', info=errors) if isinstance(mods, six.string_types): mods = mods.split(',') @@ -1561,10 +1555,10 @@ def show_sls(mods, test=None, queue=False, **kwargs): pillar_enc=pillar_enc, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - raise CommandExecutionError('Pillar failed to render', - info=st_.opts['pillar']['_errors']) + raise CommandExecutionError('Pillar failed to render', info=errors) if isinstance(mods, six.string_types): mods = mods.split(',') @@ -1610,10 +1604,10 @@ def show_top(queue=False, **kwargs): st_ = salt.state.HighState(opts, initial_pillar=_get_initial_pillar(opts)) - if not _check_pillar(kwargs, st_.opts['pillar']): + errors = _get_pillar_errors(kwargs, pillar=st_.opts['pillar']) + if errors: __context__['retcode'] = 5 - raise CommandExecutionError('Pillar failed to render', - info=st_.opts['pillar']['_errors']) + raise CommandExecutionError('Pillar failed to render', info=errors) errors = [] top_ = st_.get_top() diff --git a/tests/unit/modules/test_state.py b/tests/unit/modules/test_state.py index 7f4f361c26..e5d10493da 100644 --- a/tests/unit/modules/test_state.py +++ b/tests/unit/modules/test_state.py @@ -695,9 +695,9 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): with patch.object(state, '_check_queue', mock): self.assertEqual(state.top("reverse_top.sls"), "A") - mock = MagicMock(side_effect=[False, True, True]) - with patch.object(state, '_check_pillar', mock): - with patch.dict(state.__pillar__, {"_errors": "E"}): + mock = MagicMock(side_effect=[['E'], None, None]) + with patch.object(state, '_get_pillar_errors', mock): + with patch.dict(state.__pillar__, {"_errors": ['E']}): self.assertListEqual(state.top("reverse_top.sls"), ret) with patch.dict(state.__opts__, {"test": "A"}): @@ -854,14 +854,10 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): True), ["A"]) - mock = MagicMock(side_effect=[False, - True, - True, - True, - True]) - with patch.object(state, '_check_pillar', mock): + mock = MagicMock(side_effect=[['E', '1'], None, None, None, None]) + with patch.object(state, '_get_pillar_errors', mock): with patch.dict(state.__context__, {"retcode": 5}): - with patch.dict(state.__pillar__, {"_errors": "E1"}): + with patch.dict(state.__pillar__, {"_errors": ['E', '1']}): self.assertListEqual(state.sls("core,edit.vim dev", None, None, @@ -979,3 +975,62 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): with patch('salt.utils.fopen', mock_open()): self.assertTrue(state.pkg("/tmp/state_pkg.tgz", 0, "md5")) + + def test_get_pillar_errors_CC(self): + ''' + Test _get_pillar_errors function. + CC: External clean, Internal clean + :return: + ''' + for int_pillar, ext_pillar in [({'foo': 'bar'}, {'fred': 'baz'}), + ({'foo': 'bar'}, None), + ({}, {'fred': 'baz'})]: + with patch('salt.modules.state.__pillar__', int_pillar): + for opts, res in [({'force': True}, None), + ({'force': False}, None), + ({}, None)]: + assert res == state._get_pillar_errors(kwargs=opts, pillar=ext_pillar) + + def test_get_pillar_errors_EC(self): + ''' + Test _get_pillar_errors function. + EC: External erroneous, Internal clean + :return: + ''' + errors = ['failure', 'everywhere'] + for int_pillar, ext_pillar in [({'foo': 'bar'}, {'fred': 'baz', '_errors': errors}), + ({}, {'fred': 'baz', '_errors': errors})]: + with patch('salt.modules.state.__pillar__', int_pillar): + for opts, res in [({'force': True}, None), + ({'force': False}, errors), + ({}, errors)]: + assert res == state._get_pillar_errors(kwargs=opts, pillar=ext_pillar) + + def test_get_pillar_errors_EE(self): + ''' + Test _get_pillar_errors function. + CC: External erroneous, Internal erroneous + :return: + ''' + errors = ['failure', 'everywhere'] + for int_pillar, ext_pillar in [({'foo': 'bar', '_errors': errors}, {'fred': 'baz', '_errors': errors})]: + with patch('salt.modules.state.__pillar__', int_pillar): + for opts, res in [({'force': True}, None), + ({'force': False}, errors), + ({}, errors)]: + assert res == state._get_pillar_errors(kwargs=opts, pillar=ext_pillar) + + def test_get_pillar_errors_CE(self): + ''' + Test _get_pillar_errors function. + CC: External clean, Internal erroneous + :return: + ''' + errors = ['failure', 'everywhere'] + for int_pillar, ext_pillar in [({'foo': 'bar', '_errors': errors}, {'fred': 'baz'}), + ({'foo': 'bar', '_errors': errors}, None)]: + with patch('salt.modules.state.__pillar__', int_pillar): + for opts, res in [({'force': True}, None), + ({'force': False}, errors), + ({}, errors)]: + assert res == state._get_pillar_errors(kwargs=opts, pillar=ext_pillar) -- 2.15.1