From 89e0781be777448fc3065a78bad2b7e8ba646532 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Tue, 14 Nov 2017 17:02:50 +0100 Subject: [PATCH 01/34] ignore invalid characters when getting decoding error in cmd run --- salt/modules/cmdmod.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 3ae58905c3..f45ae6e84d 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -589,11 +589,18 @@ def _run(cmd, out = proc.stdout.decode(__salt_system_encoding__) except AttributeError: out = u'' + except UnicodeDecodeError: + log.error("UnicodeDecodeError while decoding output of cmd {0}".format(cmd)) + out = proc.stdout.decode(__salt_system_encoding__, 'ignore') try: err = proc.stderr.decode(__salt_system_encoding__) except AttributeError: err = u'' + except UnicodeDecodeError: + log.error("UnicodeDecodeError while decoding error of cmd {0}".format(cmd)) + err = proc.stderr.decode(__salt_system_encoding__, 'ignore') + if rstrip: if out is not None: From 9b67ec0d3d433eed5ed124a0f50c1f90d357acbc Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Wed, 29 Nov 2017 16:57:31 +0100 Subject: [PATCH 02/34] use 'replace' strategy for invalid encodings --- salt/modules/cmdmod.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index f45ae6e84d..2275204c83 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -591,7 +591,7 @@ def _run(cmd, out = u'' except UnicodeDecodeError: log.error("UnicodeDecodeError while decoding output of cmd {0}".format(cmd)) - out = proc.stdout.decode(__salt_system_encoding__, 'ignore') + out = proc.stdout.decode(__salt_system_encoding__, 'replace') try: err = proc.stderr.decode(__salt_system_encoding__) @@ -599,7 +599,7 @@ def _run(cmd, err = u'' except UnicodeDecodeError: log.error("UnicodeDecodeError while decoding error of cmd {0}".format(cmd)) - err = proc.stderr.decode(__salt_system_encoding__, 'ignore') + err = proc.stderr.decode(__salt_system_encoding__, 'replace') if rstrip: From bc5f6a1da4d021cf10f64ea3a0adefe0f26ffe7c Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 30 Nov 2017 09:24:03 +0100 Subject: [PATCH 03/34] use single quotes --- salt/modules/cmdmod.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 2275204c83..d7193174eb 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -590,7 +590,7 @@ def _run(cmd, except AttributeError: out = u'' except UnicodeDecodeError: - log.error("UnicodeDecodeError while decoding output of cmd {0}".format(cmd)) + log.error('UnicodeDecodeError while decoding output of cmd {0}'.format(cmd)) out = proc.stdout.decode(__salt_system_encoding__, 'replace') try: @@ -598,7 +598,7 @@ def _run(cmd, except AttributeError: err = u'' except UnicodeDecodeError: - log.error("UnicodeDecodeError while decoding error of cmd {0}".format(cmd)) + log.error('UnicodeDecodeError while decoding error of cmd {0}'.format(cmd)) err = proc.stderr.decode(__salt_system_encoding__, 'replace') From 3be96f7903e10ca695c31e8e1e07c47142a8e270 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 30 Nov 2017 16:48:58 +0100 Subject: [PATCH 04/34] lint fix --- salt/modules/cmdmod.py | 1 - 1 file changed, 1 deletion(-) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index d7193174eb..1b6ce66302 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -601,7 +601,6 @@ def _run(cmd, log.error('UnicodeDecodeError while decoding error of cmd {0}'.format(cmd)) err = proc.stderr.decode(__salt_system_encoding__, 'replace') - if rstrip: if out is not None: out = out.rstrip() From 094d21afb2ed5c1af90841bd58835954873cb107 Mon Sep 17 00:00:00 2001 From: Ari Maniatis Date: Thu, 12 Oct 2017 11:15:19 +1000 Subject: [PATCH 05/34] Better exception After spending an hour trying to figure out why I had a key error in an orchestration state I realised that saltstack requires a runner id with a dot. A clearer error message will save the next person wasting the same time. --- salt/loader.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/salt/loader.py b/salt/loader.py index 81e44ebb27..4e783e5dff 100644 --- a/salt/loader.py +++ b/salt/loader.py @@ -1588,8 +1588,10 @@ class LazyLoader(salt.utils.lazy.LazyDict): Load a single item if you have it ''' # if the key doesn't have a '.' then it isn't valid for this mod dict - if not isinstance(key, six.string_types) or u'.' not in key: - raise KeyError + if not isinstance(key, six.string_types): + raise KeyError(u'The key must be a string.') + if u'.' not in key: + raise KeyError(u'The key \'%s\' should contain a \'.\'', key) mod_name, _ = key.split(u'.', 1) if mod_name in self.missing_modules: return True From dc3097ceddb5e706bc3a036fe57e6e03e675d471 Mon Sep 17 00:00:00 2001 From: rallytime Date: Fri, 10 Nov 2017 13:41:45 -0500 Subject: [PATCH 06/34] Write a unittest for error message raised with missing "." in function --- tests/unit/loader/test_loader.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/unit/loader/test_loader.py b/tests/unit/loader/test_loader.py index 2403dbba21..c5705144ab 100644 --- a/tests/unit/loader/test_loader.py +++ b/tests/unit/loader/test_loader.py @@ -278,6 +278,38 @@ class LazyLoaderWhitelistTest(TestCase): self.assertNotIn('grains.get', self.loader) +class LazyLoaderSingleItem(TestCase): + ''' + Test loading a single item via the _load() function + ''' + @classmethod + def setUpClass(cls): + cls.opts = salt.config.minion_config(None) + cls.opts['grains'] = grains(cls.opts) + + def setUp(self): + self.loader = LazyLoader(_module_dirs(copy.deepcopy(self.opts), 'modules', 'module'), + copy.deepcopy(self.opts), + tag='module') + + def tearDown(self): + del self.loader + + def test_single_item_no_dot(self): + ''' + Checks that a KeyError is raised when the function key does not contain a '.' + ''' + with self.assertRaises(KeyError) as err: + inspect.isfunction(self.loader['testing_no_dot']) + + if six.PY2: + self.assertEqual(err.exception[0], + 'The key \'%s\' should contain a \'.\'') + else: + self.assertEqual(str(err.exception), + str(("The key '%s' should contain a '.'", 'testing_no_dot'))) + + module_template = ''' __load__ = ['test', 'test_alias'] __func_alias__ = dict(test_alias='working_alias') From 683e2cc502062a929633bf4fff72ecdc2f5efe54 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Wed, 29 Nov 2017 22:09:09 -0600 Subject: [PATCH 07/34] Rename 'environment' config option to 'saltenv' 'environment' predates the 'saltenv' nomenclature, and since we have been using saltenv on the CLI for years, and now also have pillarenv, it makes sense to make this change. --- salt/client/ssh/__init__.py | 2 +- salt/config/__init__.py | 47 ++++++++++++++++++++++++++++++++++--- salt/daemons/flo/core.py | 2 +- salt/minion.py | 14 +++++------ salt/modules/cmdmod.py | 2 +- salt/modules/cp.py | 2 +- salt/modules/dockermod.py | 2 +- salt/modules/event.py | 2 +- salt/modules/pillar.py | 4 ++-- salt/modules/saltcheck.py | 2 +- salt/modules/state.py | 32 ++++++++++++------------- salt/pillar/__init__.py | 12 +++++----- salt/pillar/git_pillar.py | 2 +- salt/state.py | 18 +++++++------- salt/utils/gitfs.py | 2 +- tests/saltsh.py | 2 +- tests/unit/test_pillar.py | 8 +++---- 17 files changed, 98 insertions(+), 57 deletions(-) diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py index 4c38a8b1ce..92d8328d6c 100644 --- a/salt/client/ssh/__init__.py +++ b/salt/client/ssh/__init__.py @@ -1050,7 +1050,7 @@ class Single(object): popts, opts_pkg[u'grains'], opts_pkg[u'id'], - opts_pkg.get(u'environment', u'base') + opts_pkg.get(u'saltenv', u'base') ) pillar_data = pillar.compile_pillar() diff --git a/salt/config/__init__.py b/salt/config/__init__.py index 5680f727b9..5084426e26 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -242,7 +242,10 @@ VALID_OPTS = { 'autoload_dynamic_modules': bool, # Force the minion into a single environment when it fetches files from the master - 'environment': str, + 'saltenv': str, + + # Prevent saltenv from being overriden on the command line + 'lock_saltenv': bool, # Force the minion into a single pillar root when it fetches pillar data from the master 'pillarenv': str, @@ -1177,7 +1180,8 @@ DEFAULT_MINION_OPTS = { 'random_startup_delay': 0, 'failhard': False, 'autoload_dynamic_modules': True, - 'environment': None, + 'saltenv': None, + 'lock_saltenv': False, 'pillarenv': None, 'pillarenv_from_saltenv': False, 'pillar_opts': False, @@ -1454,7 +1458,8 @@ DEFAULT_MASTER_OPTS = { }, 'top_file_merging_strategy': 'merge', 'env_order': [], - 'environment': None, + 'saltenv': None, + 'lock_saltenv': False, 'default_top': 'base', 'file_client': 'local', 'git_pillar_base': 'master', @@ -3590,6 +3595,24 @@ def apply_minion_config(overrides=None, if overrides: opts.update(overrides) + if u'environment' in opts: + if u'saltenv' in opts: + log.warning( + u'The \'saltenv\' and \'environment\' minion config options ' + u'cannot both be used. Ignoring \'environment\' in favor of ' + u'\'saltenv\'.', + ) + # Set environment to saltenv in case someone's custom module is + # refrencing __opts__['environment'] + opts[u'environment'] = opts[u'saltenv'] + else: + log.warning( + u'The \'environment\' minion config option has been renamed ' + u'to \'saltenv\'. Using %s as the \'saltenv\' config value.', + opts[u'environment'] + ) + opts[u'saltenv'] = opts[u'environment'] + opts['__cli'] = os.path.basename(sys.argv[0]) # No ID provided. Will getfqdn save us? @@ -3742,6 +3765,24 @@ def apply_master_config(overrides=None, defaults=None): if overrides: opts.update(overrides) + if u'environment' in opts: + if u'saltenv' in opts: + log.warning( + u'The \'saltenv\' and \'environment\' master config options ' + u'cannot both be used. Ignoring \'environment\' in favor of ' + u'\'saltenv\'.', + ) + # Set environment to saltenv in case someone's custom runner is + # refrencing __opts__['environment'] + opts[u'environment'] = opts[u'saltenv'] + else: + log.warning( + u'The \'environment\' master config option has been renamed ' + u'to \'saltenv\'. Using %s as the \'saltenv\' config value.', + opts[u'environment'] + ) + opts[u'saltenv'] = opts[u'environment'] + if len(opts['sock_dir']) > len(opts['cachedir']) + 10: opts['sock_dir'] = os.path.join(opts['cachedir'], '.salt-unix') diff --git a/salt/daemons/flo/core.py b/salt/daemons/flo/core.py index 1a11e08aed..dad36d32a4 100644 --- a/salt/daemons/flo/core.py +++ b/salt/daemons/flo/core.py @@ -737,7 +737,7 @@ class SaltLoadPillar(ioflo.base.deeding.Deed): 'dst': (master.name, None, 'remote_cmd')} load = {'id': self.opts.value['id'], 'grains': self.grains.value, - 'saltenv': self.opts.value['environment'], + 'saltenv': self.opts.value['saltenv'], 'ver': '2', 'cmd': '_pillar'} self.road_stack.value.transmit({'route': route, 'load': load}, diff --git a/salt/minion.py b/salt/minion.py index bd4cfcc01b..71b856ac78 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -736,8 +736,8 @@ class SMinion(MinionBase): if not os.path.isdir(pdir): os.makedirs(pdir, 0o700) ptop = os.path.join(pdir, u'top.sls') - if self.opts[u'environment'] is not None: - penv = self.opts[u'environment'] + if self.opts[u'saltenv'] is not None: + penv = self.opts[u'saltenv'] else: penv = u'base' cache_top = {penv: {self.opts[u'id']: [u'cache']}} @@ -773,7 +773,7 @@ class SMinion(MinionBase): self.opts, self.opts[u'grains'], self.opts[u'id'], - self.opts[u'environment'], + self.opts[u'saltenv'], pillarenv=self.opts.get(u'pillarenv'), ).compile_pillar() @@ -1144,7 +1144,7 @@ class Minion(MinionBase): self.opts, self.opts[u'grains'], self.opts[u'id'], - self.opts[u'environment'], + self.opts[u'saltenv'], pillarenv=self.opts.get(u'pillarenv') ).compile_pillar() @@ -2032,7 +2032,7 @@ class Minion(MinionBase): self.opts, self.opts[u'grains'], self.opts[u'id'], - self.opts[u'environment'], + self.opts[u'saltenv'], pillarenv=self.opts.get(u'pillarenv'), ).compile_pillar() except SaltClientError: @@ -3349,7 +3349,7 @@ class ProxyMinion(Minion): self.opts, self.opts[u'grains'], self.opts[u'id'], - saltenv=self.opts[u'environment'], + saltenv=self.opts[u'saltenv'], pillarenv=self.opts.get(u'pillarenv'), ).compile_pillar() @@ -3391,7 +3391,7 @@ class ProxyMinion(Minion): # we can then sync any proxymodules down from the master # we do a sync_all here in case proxy code was installed by # SPM or was manually placed in /srv/salt/_modules etc. - self.functions[u'saltutil.sync_all'](saltenv=self.opts[u'environment']) + self.functions[u'saltutil.sync_all'](saltenv=self.opts[u'saltenv']) # Pull in the utils self.utils = salt.loader.utils(self.opts) diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 3ae58905c3..d0489285ea 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -219,7 +219,7 @@ def _gather_pillar(pillarenv, pillar_override): __opts__, __grains__, __opts__['id'], - __opts__['environment'], + __opts__['saltenv'], pillar_override=pillar_override, pillarenv=pillarenv ) diff --git a/salt/modules/cp.py b/salt/modules/cp.py index cd894bea58..5d95d2a31c 100644 --- a/salt/modules/cp.py +++ b/salt/modules/cp.py @@ -49,7 +49,7 @@ def _gather_pillar(pillarenv, pillar_override): __opts__, __grains__, __opts__['id'], - __opts__['environment'], + __opts__['saltenv'], pillar_override=pillar_override, pillarenv=pillarenv ) diff --git a/salt/modules/dockermod.py b/salt/modules/dockermod.py index dadb3c8c17..261d0f5b3b 100644 --- a/salt/modules/dockermod.py +++ b/salt/modules/dockermod.py @@ -5290,7 +5290,7 @@ def _gather_pillar(pillarenv, pillar_override, **grains): grains, # Not sure if these two are correct __opts__['id'], - __opts__['environment'], + __opts__['saltenv'], pillar_override=pillar_override, pillarenv=pillarenv ) diff --git a/salt/modules/event.py b/salt/modules/event.py index 25c9153ab7..d75e9be1ec 100644 --- a/salt/modules/event.py +++ b/salt/modules/event.py @@ -225,7 +225,7 @@ def send(tag, data_dict['pillar'] = __pillar__ if with_env_opts: - data_dict['saltenv'] = __opts__.get('environment', 'base') + data_dict['saltenv'] = __opts__.get('saltenv', 'base') data_dict['pillarenv'] = __opts__.get('pillarenv') if kwargs: diff --git a/salt/modules/pillar.py b/salt/modules/pillar.py index 035e082a40..a3dc6f57c1 100644 --- a/salt/modules/pillar.py +++ b/salt/modules/pillar.py @@ -237,7 +237,7 @@ def items(*args, **kwargs): pillarenv = kwargs.get('pillarenv') if pillarenv is None: if __opts__.get('pillarenv_from_saltenv', False): - pillarenv = kwargs.get('saltenv') or __opts__['environment'] + pillarenv = kwargs.get('saltenv') or __opts__['saltenv'] else: pillarenv = __opts__['pillarenv'] @@ -468,7 +468,7 @@ def ext(external, pillar=None): __opts__, __grains__, __opts__['id'], - __opts__['environment'], + __opts__['saltenv'], ext=external, pillar_override=pillar) diff --git a/salt/modules/saltcheck.py b/salt/modules/saltcheck.py index f59d2d7e91..a6d2fe1c69 100644 --- a/salt/modules/saltcheck.py +++ b/salt/modules/saltcheck.py @@ -509,7 +509,7 @@ class SaltCheck(object): # state cache should be updated before running this method search_list = [] cachedir = __opts__.get('cachedir', None) - environment = __opts__['environment'] + environment = __opts__['saltenv'] if environment: path = cachedir + os.sep + "files" + os.sep + environment search_list.append(path) diff --git a/salt/modules/state.py b/salt/modules/state.py index af7ce0fe38..107332c2fd 100644 --- a/salt/modules/state.py +++ b/salt/modules/state.py @@ -271,9 +271,9 @@ def _get_opts(**kwargs): if 'saltenv' in kwargs: saltenv = kwargs['saltenv'] if saltenv is not None and not isinstance(saltenv, six.string_types): - opts['environment'] = str(kwargs['saltenv']) + opts['saltenv'] = str(kwargs['saltenv']) else: - opts['environment'] = kwargs['saltenv'] + opts['saltenv'] = kwargs['saltenv'] if 'pillarenv' in kwargs or opts.get('pillarenv_from_saltenv', False): pillarenv = kwargs.get('pillarenv') or kwargs.get('saltenv') @@ -840,7 +840,7 @@ def highstate(test=None, queue=False, **kwargs): kwargs.pop('env') if 'saltenv' in kwargs: - opts['environment'] = kwargs['saltenv'] + opts['saltenv'] = kwargs['saltenv'] if 'pillarenv' in kwargs: opts['pillarenv'] = kwargs['pillarenv'] @@ -1032,8 +1032,8 @@ def sls(mods, test=None, exclude=None, queue=False, **kwargs): # Since this is running a specific SLS file (or files), fall back to the # 'base' saltenv if none is configured and none was passed. - if opts['environment'] is None: - opts['environment'] = 'base' + if opts['saltenv'] is None: + opts['saltenv'] = 'base' pillar_override = kwargs.get('pillar') pillar_enc = kwargs.get('pillar_enc') @@ -1089,7 +1089,7 @@ def sls(mods, test=None, exclude=None, queue=False, **kwargs): st_.push_active() ret = {} try: - high_, errors = st_.render_highstate({opts['environment']: mods}) + high_, errors = st_.render_highstate({opts['saltenv']: mods}) if errors: __context__['retcode'] = 1 @@ -1411,8 +1411,8 @@ def sls_id(id_, mods, test=None, queue=False, **kwargs): # Since this is running a specific ID within a specific SLS file, fall back # to the 'base' saltenv if none is configured and none was passed. - if opts['environment'] is None: - opts['environment'] = 'base' + if opts['saltenv'] is None: + opts['saltenv'] = 'base' pillar_override = kwargs.get('pillar') pillar_enc = kwargs.get('pillar_enc') @@ -1446,7 +1446,7 @@ def sls_id(id_, mods, test=None, queue=False, **kwargs): split_mods = mods.split(',') st_.push_active() try: - high_, errors = st_.render_highstate({opts['environment']: split_mods}) + high_, errors = st_.render_highstate({opts['saltenv']: split_mods}) finally: st_.pop_active() errors += st_.state.verify_high(high_) @@ -1472,7 +1472,7 @@ def sls_id(id_, mods, test=None, queue=False, **kwargs): if not ret: raise SaltInvocationError( 'No matches for ID \'{0}\' found in SLS \'{1}\' within saltenv ' - '\'{2}\''.format(id_, mods, opts['environment']) + '\'{2}\''.format(id_, mods, opts['saltenv']) ) return ret @@ -1523,8 +1523,8 @@ def show_low_sls(mods, test=None, queue=False, **kwargs): # Since this is dealing with a specific SLS file (or files), fall back to # the 'base' saltenv if none is configured and none was passed. - if opts['environment'] is None: - opts['environment'] = 'base' + if opts['saltenv'] is None: + opts['saltenv'] = 'base' pillar_override = kwargs.get('pillar') pillar_enc = kwargs.get('pillar_enc') @@ -1555,7 +1555,7 @@ def show_low_sls(mods, test=None, queue=False, **kwargs): mods = mods.split(',') st_.push_active() try: - high_, errors = st_.render_highstate({opts['environment']: mods}) + high_, errors = st_.render_highstate({opts['saltenv']: mods}) finally: st_.pop_active() errors += st_.state.verify_high(high_) @@ -1610,8 +1610,8 @@ def show_sls(mods, test=None, queue=False, **kwargs): # Since this is dealing with a specific SLS file (or files), fall back to # the 'base' saltenv if none is configured and none was passed. - if opts['environment'] is None: - opts['environment'] = 'base' + if opts['saltenv'] is None: + opts['saltenv'] = 'base' pillar_override = kwargs.get('pillar') pillar_enc = kwargs.get('pillar_enc') @@ -1644,7 +1644,7 @@ def show_sls(mods, test=None, queue=False, **kwargs): mods = mods.split(',') st_.push_active() try: - high_, errors = st_.render_highstate({opts['environment']: mods}) + high_, errors = st_.render_highstate({opts['saltenv']: mods}) finally: st_.pop_active() errors += st_.state.verify_high(high_) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 24c7c485e3..903d0f5b1c 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -138,7 +138,7 @@ class AsyncRemotePillar(RemotePillarMixin): def __init__(self, opts, grains, minion_id, saltenv, ext=None, functions=None, pillar_override=None, pillarenv=None, extra_minion_data=None): self.opts = opts - self.opts['environment'] = saltenv + self.opts['saltenv'] = saltenv self.ext = ext self.grains = grains self.minion_id = minion_id @@ -165,7 +165,7 @@ class AsyncRemotePillar(RemotePillarMixin): ''' load = {'id': self.minion_id, 'grains': self.grains, - 'saltenv': self.opts['environment'], + 'saltenv': self.opts['saltenv'], 'pillarenv': self.opts['pillarenv'], 'pillar_override': self.pillar_override, 'extra_minion_data': self.extra_minion_data, @@ -198,7 +198,7 @@ class RemotePillar(RemotePillarMixin): def __init__(self, opts, grains, minion_id, saltenv, ext=None, functions=None, pillar_override=None, pillarenv=None, extra_minion_data=None): self.opts = opts - self.opts['environment'] = saltenv + self.opts['saltenv'] = saltenv self.ext = ext self.grains = grains self.minion_id = minion_id @@ -224,7 +224,7 @@ class RemotePillar(RemotePillarMixin): ''' load = {'id': self.minion_id, 'grains': self.grains, - 'saltenv': self.opts['environment'], + 'saltenv': self.opts['saltenv'], 'pillarenv': self.opts['pillarenv'], 'pillar_override': self.pillar_override, 'extra_minion_data': self.extra_minion_data, @@ -445,9 +445,9 @@ class Pillar(object): else: opts['grains'] = grains # Allow minion/CLI saltenv/pillarenv to take precedence over master - opts['environment'] = saltenv \ + opts['saltenv'] = saltenv \ if saltenv is not None \ - else opts.get('environment') + else opts.get('saltenv') opts['pillarenv'] = pillarenv \ if pillarenv is not None \ else opts.get('pillarenv') diff --git a/salt/pillar/git_pillar.py b/salt/pillar/git_pillar.py index 732183a089..5450de5ed9 100644 --- a/salt/pillar/git_pillar.py +++ b/salt/pillar/git_pillar.py @@ -404,7 +404,7 @@ def ext_pillar(minion_id, pillar, *repos): # pylint: disable=unused-argument # Map env if env == '__env__' before checking the env value if env == '__env__': env = opts.get('pillarenv') \ - or opts.get('environment') \ + or opts.get('saltenv') \ or opts.get('git_pillar_base') log.debug('__env__ maps to %s', env) diff --git a/salt/state.py b/salt/state.py index f0e405e77d..cbc34bede9 100644 --- a/salt/state.py +++ b/salt/state.py @@ -763,7 +763,7 @@ class State(object): self.opts, self.opts[u'grains'], self.opts[u'id'], - self.opts[u'environment'], + self.opts[u'saltenv'], pillar_override=self._pillar_override, pillarenv=self.opts.get(u'pillarenv')) return pillar.compile_pillar() @@ -2900,32 +2900,32 @@ class BaseHighState(object): found = 0 # did we find any contents in the top files? # Gather initial top files merging_strategy = self.opts[u'top_file_merging_strategy'] - if merging_strategy == u'same' and not self.opts[u'environment']: + if merging_strategy == u'same' and not self.opts[u'saltenv']: if not self.opts[u'default_top']: raise SaltRenderError( u'top_file_merging_strategy set to \'same\', but no ' u'default_top configuration option was set' ) - if self.opts[u'environment']: + if self.opts[u'saltenv']: contents = self.client.cache_file( self.opts[u'state_top'], - self.opts[u'environment'] + self.opts[u'saltenv'] ) if contents: found = 1 - tops[self.opts[u'environment']] = [ + tops[self.opts[u'saltenv']] = [ compile_template( contents, self.state.rend, self.state.opts[u'renderer'], self.state.opts[u'renderer_blacklist'], self.state.opts[u'renderer_whitelist'], - saltenv=self.opts[u'environment'] + saltenv=self.opts[u'saltenv'] ) ] else: - tops[self.opts[u'environment']] = [{}] + tops[self.opts[u'saltenv']] = [{}] else: found = 0 @@ -3257,8 +3257,8 @@ class BaseHighState(object): matches = DefaultOrderedDict(OrderedDict) # pylint: disable=cell-var-from-loop for saltenv, body in six.iteritems(top): - if self.opts[u'environment']: - if saltenv != self.opts[u'environment']: + if self.opts[u'saltenv']: + if saltenv != self.opts[u'saltenv']: continue for match, data in six.iteritems(body): def _filter_matches(_match, _data, _opts): diff --git a/salt/utils/gitfs.py b/salt/utils/gitfs.py index f619ec4b76..38687d54c4 100644 --- a/salt/utils/gitfs.py +++ b/salt/utils/gitfs.py @@ -910,7 +910,7 @@ class GitProvider(object): ''' if self.branch == '__env__': target = self.opts.get('pillarenv') \ - or self.opts.get('environment') \ + or self.opts.get('saltenv') \ or 'base' return self.opts['{0}_base'.format(self.role)] \ if target == 'base' \ diff --git a/tests/saltsh.py b/tests/saltsh.py index 4167cebc92..6b70f87fcb 100644 --- a/tests/saltsh.py +++ b/tests/saltsh.py @@ -90,7 +90,7 @@ def get_salt_vars(): __opts__, __grains__, __opts__.get('id'), - __opts__.get('environment'), + __opts__.get('saltenv'), ).compile_pillar() else: __pillar__ = {} diff --git a/tests/unit/test_pillar.py b/tests/unit/test_pillar.py index 476941f8f4..f4ad72a995 100644 --- a/tests/unit/test_pillar.py +++ b/tests/unit/test_pillar.py @@ -55,7 +55,7 @@ class PillarTestCase(TestCase): 'os': 'Ubuntu', } pillar = salt.pillar.Pillar(opts, grains, 'mocked-minion', 'dev') - self.assertEqual(pillar.opts['environment'], 'dev') + self.assertEqual(pillar.opts['saltenv'], 'dev') self.assertEqual(pillar.opts['pillarenv'], 'dev') def test_ext_pillar_no_extra_minion_data_val_dict(self): @@ -416,7 +416,7 @@ class PillarTestCase(TestCase): 'state_top': '', 'pillar_roots': [], 'extension_modules': '', - 'environment': 'base', + 'saltenv': 'base', 'file_roots': [], } grains = { @@ -584,7 +584,7 @@ class RemotePillarTestCase(TestCase): salt.pillar.RemotePillar({}, self.grains, 'mocked-minion', 'dev') mock_get_extra_minion_data.assert_called_once_with( - {'environment': 'dev'}) + {'saltenv': 'dev'}) def test_multiple_keys_in_opts_added_to_pillar(self): opts = { @@ -702,7 +702,7 @@ class AsyncRemotePillarTestCase(TestCase): salt.pillar.RemotePillar({}, self.grains, 'mocked-minion', 'dev') mock_get_extra_minion_data.assert_called_once_with( - {'environment': 'dev'}) + {'saltenv': 'dev'}) def test_pillar_send_extra_minion_data_from_config(self): opts = { From 5156edce4586bba83de3ef650105f9dad43d89cd Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Thu, 30 Nov 2017 13:18:37 -0600 Subject: [PATCH 08/34] Prevent saltenv from being manually specified when running states This uses the newly-added "lock_saltenv" minion config option. --- salt/modules/state.py | 10 +++++++--- salt/state.py | 35 +++++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/salt/modules/state.py b/salt/modules/state.py index 107332c2fd..72cdd1fd9f 100644 --- a/salt/modules/state.py +++ b/salt/modules/state.py @@ -270,9 +270,13 @@ def _get_opts(**kwargs): if 'saltenv' in kwargs: saltenv = kwargs['saltenv'] - if saltenv is not None and not isinstance(saltenv, six.string_types): - opts['saltenv'] = str(kwargs['saltenv']) - else: + if saltenv is not None: + if not isinstance(saltenv, six.string_types): + saltenv = six.text_type(saltenv) + if opts['lock_saltenv'] and saltenv != opts['saltenv']: + raise CommandExecutionError( + 'saltenv is locked and cannot be changed' + ) opts['saltenv'] = kwargs['saltenv'] if 'pillarenv' in kwargs or opts.get('pillarenv_from_saltenv', False): diff --git a/salt/state.py b/salt/state.py index cbc34bede9..a95d06e968 100644 --- a/salt/state.py +++ b/salt/state.py @@ -1892,20 +1892,27 @@ class State(object): (u'onlyif' in low and u'{0[state]}.mod_run_check'.format(low) not in self.states): ret.update(self._run_check(low)) - if u'saltenv' in low: - inject_globals[u'__env__'] = six.text_type(low[u'saltenv']) - elif isinstance(cdata[u'kwargs'].get(u'env', None), six.string_types): - # User is using a deprecated env setting which was parsed by - # format_call. - # We check for a string type since module functions which - # allow setting the OS environ also make use of the "env" - # keyword argument, which is not a string - inject_globals[u'__env__'] = six.text_type(cdata[u'kwargs'][u'env']) - elif u'__env__' in low: - # The user is passing an alternative environment using __env__ - # which is also not the appropriate choice, still, handle it - inject_globals[u'__env__'] = six.text_type(low[u'__env__']) - else: + if not self.opts.get(u'lock_saltenv', False): + # NOTE: Overriding the saltenv when lock_saltenv is blocked in + # salt/modules/state.py, before we ever get here, but this + # additional check keeps use of the State class outside of the + # salt/modules/state.py from getting around this setting. + if u'saltenv' in low: + inject_globals[u'__env__'] = six.text_type(low[u'saltenv']) + elif isinstance(cdata[u'kwargs'].get(u'env', None), six.string_types): + # User is using a deprecated env setting which was parsed by + # format_call. + # We check for a string type since module functions which + # allow setting the OS environ also make use of the "env" + # keyword argument, which is not a string + inject_globals[u'__env__'] = six.text_type(cdata[u'kwargs'][u'env']) + elif u'__env__' in low: + # The user is passing an alternative environment using + # __env__ which is also not the appropriate choice, still, + # handle it + inject_globals[u'__env__'] = six.text_type(low[u'__env__']) + + if u'__env__' not in inject_globals: # Let's use the default environment inject_globals[u'__env__'] = u'base' From 3770cd7dd314f12fbb4f683d8f771b39ef341c34 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Thu, 30 Nov 2017 14:28:09 -0600 Subject: [PATCH 09/34] Add unit test for lock_saltenv Also move up the _get_opts in state.pkg to make it easier to unit test --- salt/modules/state.py | 4 +- tests/unit/modules/test_state.py | 91 +++++++++++++++++++++++++++++--- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/salt/modules/state.py b/salt/modules/state.py index 72cdd1fd9f..ebd2588884 100644 --- a/salt/modules/state.py +++ b/salt/modules/state.py @@ -1598,7 +1598,7 @@ def show_sls(mods, test=None, queue=False, **kwargs): .. code-block:: bash - salt '*' state.show_sls core,edit.vim dev + salt '*' state.show_sls core,edit.vim saltenv=dev ''' if 'env' in kwargs: # "env" is not supported; Use "saltenv". @@ -1814,6 +1814,7 @@ def pkg(pkg_path, salt '*' state.pkg /tmp/salt_state.tgz 760a9353810e36f6d81416366fc426dc md5 ''' # TODO - Add ability to download from salt master or other source + popts = _get_opts(**kwargs) if not os.path.isfile(pkg_path): return {} if not salt.utils.hashutils.get_hash(pkg_path, hash_type) == pkg_sum: @@ -1848,7 +1849,6 @@ def pkg(pkg_path, with salt.utils.files.fopen(roster_grains_json, 'r') as fp_: roster_grains = json.load(fp_, object_hook=salt.utils.data.decode_dict) - popts = _get_opts(**kwargs) if os.path.isfile(roster_grains_json): popts['grains'] = roster_grains popts['fileclient'] = 'local' diff --git a/tests/unit/modules/test_state.py b/tests/unit/modules/test_state.py index f72351f070..85236cab56 100644 --- a/tests/unit/modules/test_state.py +++ b/tests/unit/modules/test_state.py @@ -25,7 +25,7 @@ import salt.utils.hashutils import salt.utils.odict import salt.utils.platform import salt.modules.state as state -from salt.exceptions import SaltInvocationError +from salt.exceptions import CommandExecutionError, SaltInvocationError from salt.ext import six @@ -362,7 +362,7 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): state: { '__opts__': { 'cachedir': '/D', - 'environment': None, + 'saltenv': None, '__cli': 'salt', }, '__utils__': utils, @@ -632,7 +632,7 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): with patch.dict(state.__opts__, {"test": "A"}): mock = MagicMock( return_value={'test': True, - 'environment': None} + 'saltenv': None} ) with patch.object(state, '_get_opts', mock): mock = MagicMock(return_value=True) @@ -659,7 +659,7 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): with patch.dict(state.__opts__, {"test": "A"}): mock = MagicMock( return_value={'test': True, - 'environment': None} + 'saltenv': None} ) with patch.object(state, '_get_opts', mock): MockState.State.flag = True @@ -681,7 +681,7 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): with patch.dict(state.__opts__, {"test": "A"}): mock = MagicMock( return_value={'test': True, - 'environment': None} + 'saltenv': None} ) with patch.object(state, '_get_opts', mock): mock = MagicMock(return_value=True) @@ -881,7 +881,7 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): with patch.dict(state.__opts__, {"test": None}): mock = MagicMock(return_value={"test": "", - "environment": None}) + "saltenv": None}) with patch.object(state, '_get_opts', mock): mock = MagicMock(return_value=True) with patch.object(salt.utils, @@ -993,3 +993,82 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): else: with patch('salt.utils.files.fopen', mock_open()): self.assertTrue(state.pkg(tar_file, 0, "md5")) + + def test_lock_saltenv(self): + ''' + Tests lock_saltenv in each function which accepts saltenv on the CLI + ''' + lock_msg = 'saltenv is locked and cannot be changed' + empty_list_mock = MagicMock(return_value=[]) + with patch.dict(state.__opts__, {'lock_saltenv': True}), \ + patch.dict(state.__salt__, {'grains.get': empty_list_mock}), \ + patch.object(state, 'running', empty_list_mock): + + # Test high + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.high( + [{"vim": {"pkg": ["installed"]}}], saltenv='base') + + # Test template + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.template('foo', saltenv='base') + + # Test template_str + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.template_str('foo', saltenv='base') + + # Test apply_ with SLS + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.apply_('foo', saltenv='base') + + # Test apply_ with Highstate + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.apply_(saltenv='base') + + # Test highstate + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.highstate(saltenv='base') + + # Test sls + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.sls('foo', saltenv='base') + + # Test top + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.top('foo.sls', saltenv='base') + + # Test show_highstate + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.show_highstate(saltenv='base') + + # Test show_lowstate + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.show_lowstate(saltenv='base') + + # Test sls_id + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.sls_id('foo', 'bar', saltenv='base') + + # Test show_low_sls + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.show_low_sls('foo', saltenv='base') + + # Test show_sls + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.show_sls('foo', saltenv='base') + + # Test show_top + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.show_top(saltenv='base') + + # Test single + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.single('foo.bar', name='baz', saltenv='base') + + # Test pkg + with self.assertRaisesRegex(CommandExecutionError, lock_msg): + state.pkg( + '/tmp/salt_state.tgz', + '760a9353810e36f6d81416366fc426dc', + 'md5', + saltenv='base') From c1e4bea83e1d3b9694187deb79b1954768e95dd4 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Thu, 30 Nov 2017 14:30:44 -0600 Subject: [PATCH 10/34] Change lock message --- salt/modules/state.py | 2 +- tests/unit/modules/test_state.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/state.py b/salt/modules/state.py index ebd2588884..6ed5e7e500 100644 --- a/salt/modules/state.py +++ b/salt/modules/state.py @@ -275,7 +275,7 @@ def _get_opts(**kwargs): saltenv = six.text_type(saltenv) if opts['lock_saltenv'] and saltenv != opts['saltenv']: raise CommandExecutionError( - 'saltenv is locked and cannot be changed' + 'lock_saltenv is enabled, saltenv cannot be changed' ) opts['saltenv'] = kwargs['saltenv'] diff --git a/tests/unit/modules/test_state.py b/tests/unit/modules/test_state.py index 85236cab56..6a70baac83 100644 --- a/tests/unit/modules/test_state.py +++ b/tests/unit/modules/test_state.py @@ -998,7 +998,7 @@ class StateTestCase(TestCase, LoaderModuleMockMixin): ''' Tests lock_saltenv in each function which accepts saltenv on the CLI ''' - lock_msg = 'saltenv is locked and cannot be changed' + lock_msg = 'lock_saltenv is enabled, saltenv cannot be changed' empty_list_mock = MagicMock(return_value=[]) with patch.dict(state.__opts__, {'lock_saltenv': True}), \ patch.dict(state.__salt__, {'grains.get': empty_list_mock}), \ From bf8384d23092c16930c4a491a91e1664bf8b9b7e Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Thu, 30 Nov 2017 14:53:01 -0600 Subject: [PATCH 11/34] Add documentation for lock_saltenv Also add documentation for renaming of environment to saltenv --- doc/ref/configuration/minion.rst | 30 +++++++++++++++++++++++++++--- doc/topics/releases/oxygen.rst | 19 +++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/doc/ref/configuration/minion.rst b/doc/ref/configuration/minion.rst index 21e376561d..c73b5eb6e8 100644 --- a/doc/ref/configuration/minion.rst +++ b/doc/ref/configuration/minion.rst @@ -1725,9 +1725,15 @@ enabled and can be disabled by changing this value to ``False``. If ``extmod_whitelist`` is specified, modules which are not whitelisted will also be cleaned here. .. conf_minion:: environment +.. conf_minion:: saltenv -``environment`` ---------------- +``saltenv`` +----------- + +.. versionchanged:: Oxygen + Renamed from ``environment`` to ``saltenv``. If ``environment`` is used, + ``saltenv`` will take its value. If both are used, ``environment`` will be + ignored and ``saltenv`` will be used. Normally the minion is not isolated to any single environment on the master when running states, but the environment can be isolated on the minion side @@ -1736,7 +1742,25 @@ environments is to isolate via the top file. .. code-block:: yaml - environment: dev + saltenv: dev + +.. conf_minion:: lock_saltenv + +``lock_saltenv`` +---------------- + +.. versionadded:: Oxygen + +Default: ``False`` + +For purposes of running states, this option prevents using the ``saltenv`` +argument to manually set the environment. This is useful to keep a minion which +has the :conf_minion:`saltenv` option set to ``dev`` from running states from +an environment other than ``dev``. + +.. code-block:: yaml + + lock_saltenv: True .. conf_minion:: snapper_states diff --git a/doc/topics/releases/oxygen.rst b/doc/topics/releases/oxygen.rst index 188ef506eb..36dc5bce79 100644 --- a/doc/topics/releases/oxygen.rst +++ b/doc/topics/releases/oxygen.rst @@ -46,6 +46,25 @@ noon PST so the Stormpath external authentication module has been removed. https://stormpath.com/oktaplusstormpath +:conf_minion:`environment` config option renamed to :conf_minion:`saltenv` +-------------------------------------------------------------------------- + +The :conf_minion:`environment` config option predates referring to a salt +fileserver environment as a **saltenv**. To pin a minion to a single +environment for running states, one would use :conf_minion:`environment`, but +overriding that environment would be done with the ``saltenv`` argument. For +consistency, :conf_minion:`environment` is now simply referred to as +:conf_minion:`saltenv`. There are no plans to deprecate or remove +:conf_minion:`environment`, if used it will log a warning and its value will be +used as :conf_minion:`saltenv`. + +:conf_minion:`lock_saltenv` config option added +----------------------------------------------- + +If set to ``True``, this option will prevent a minion from allowing the +``saltenv`` argument to override the value set in :conf_minion:`saltenv` when +running states. + New Grains ---------- From 3f9bf00c151616933f0c45b29df5248f65607c5f Mon Sep 17 00:00:00 2001 From: rallytime Date: Fri, 1 Dec 2017 15:09:14 -0500 Subject: [PATCH 12/34] Reduce the number of days an issue is stale by 15 --- .github/stale.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/stale.yml b/.github/stale.yml index 44334c7e6b..27f1de5df0 100644 --- a/.github/stale.yml +++ b/.github/stale.yml @@ -1,8 +1,8 @@ # Probot Stale configuration file # Number of days of inactivity before an issue becomes stale -# 875 is approximately 2 years and 5 months -daysUntilStale: 875 +# 860 is approximately 2 years and 4 months +daysUntilStale: 860 # Number of days of inactivity before a stale issue is closed daysUntilClose: 7 From b8db7ab7bf4edee2613d20d0c09fda21cddfc3c1 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Tue, 14 Nov 2017 09:04:06 +0100 Subject: [PATCH 13/34] support more options for zypper search module --- salt/modules/zypper.py | 88 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 82 insertions(+), 6 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index 71b280c040..6c717b9818 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -1750,7 +1750,7 @@ def list_installed_patterns(): return _get_patterns(installed_only=True) -def search(criteria, refresh=False): +def search(criteria, refresh=False, **kwargs): ''' List known packags, available to the system. @@ -1759,6 +1759,47 @@ def search(criteria, refresh=False): If set to False (default) it depends on zypper if a refresh is executed. + match (str) + One of `exact`, `words`, `substrings`. Search for an `exact` match + or for the whole `words` only. Default to `substrings` to patch + partial words. + + provides (bool) + Search for packages which provide the search strings. + + recommends (bool) + Search for packages which recommend the search strings. + + requires (bool) + Search for packages which require the search strings. + + suggests (bool) + Search for packages which suggest the search strings. + + conflicts (bool) + Search packages conflicting with search strings. + + obsoletes (bool) + Search for packages which obsolete the search strings. + + file_list (bool) + Search for a match in the file list of packages. + + search_descriptions (bool) + Search also in package summaries and descriptions. + + case_sensitive (bool) + Perform case-sensitive search. + + installed_only (bool) + Show only installed packages. + + not_installed_only (bool) + Show only packages which are not installed. + + details (bool) + Show version and repository + CLI Examples: .. code-block:: bash @@ -1768,17 +1809,52 @@ def search(criteria, refresh=False): if refresh: refresh_db() - solvables = __zypper__.nolock.xml.call('se', criteria).getElementsByTagName('solvable') + cmd = ['se'] + if kwargs.get('match') == 'exact': + cmd.append('--match-exact') + elif kwargs.get('match') == 'words': + cmd.append('--match-words') + elif kwargs.get('match') == 'substrings': + cmd.append('--match-substrings') + + if kwargs.get('provides'): + cmd.append('--provides') + if kwargs.get('recommends'): + cmd.append('--recommends') + if kwargs.get('requires'): + cmd.append('--requires') + if kwargs.get('suggests'): + cmd.append('--suggests') + if kwargs.get('conflicts'): + cmd.append('--conflicts') + if kwargs.get('obsoletes'): + cmd.append('--obsoletes') + + if kwargs.get('file_list'): + cmd.append('--file-list') + if kwargs.get('search_descriptions'): + cmd.append('--search-descriptions') + if kwargs.get('case_sensitive'): + cmd.append('--case-sensitive') + if kwargs.get('installed_only'): + cmd.append('--installed-only') + if kwargs.get('not_installed_only'): + cmd.append('--not-installed-only') + if kwargs.get('details'): + cmd.append('--details') + + cmd.append(criteria) + solvables = __zypper__.nolock.noraise.xml.call(*cmd).getElementsByTagName('solvable') if not solvables: raise CommandExecutionError( 'No packages found matching \'{0}\''.format(criteria) ) out = {} - for solvable in [slv for slv in solvables - if slv.getAttribute('status') == 'not-installed' - and slv.getAttribute('kind') == 'package']: - out[solvable.getAttribute('name')] = {'summary': solvable.getAttribute('summary')} + for solvable in solvables: + out[solvable.getAttribute('name')] = dict() + for k,v in solvable.attributes.items(): + out[solvable.getAttribute('name')][k] = v return out From 3d28be0938dcf805ae19bffc3382300eb122e769 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Tue, 14 Nov 2017 14:41:31 +0100 Subject: [PATCH 14/34] implement resolve_capabilities option for packages --- salt/modules/zypper.py | 98 +++++++++++++++++++++++++++++++++++++++++- salt/states/pkg.py | 88 ++++++++++++++++++++++++++++++++++++- 2 files changed, 183 insertions(+), 3 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index 6c717b9818..e4c7876943 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -20,6 +20,7 @@ import re import os import time import datetime +import copy # Import 3rd-party libs # pylint: disable=import-error,redefined-builtin,no-name-in-module @@ -1049,6 +1050,10 @@ def install(name=None, operator (<, >, <=, >=, =) and a version number (ex. '>1.2.3-4'). This parameter is ignored if ``pkgs`` or ``sources`` is passed. + resolve_capabilities + If this option is set to True zypper will take capabilites into + account. In this case names which are just provided by a package + will get installed. Default is False. Multiple Package Installation Options: @@ -1164,7 +1169,13 @@ def install(name=None, log.info('Targeting repo \'{0}\''.format(fromrepo)) else: fromrepoopt = '' - cmd_install = ['install', '--name', '--auto-agree-with-licenses'] + cmd_install = ['install', '--auto-agree-with-licenses'] + + if kwargs.get('resolve_capabilities', False): + cmd_install.append('--capability') + else: + cmd_install.append('--name') + if not refresh: cmd_install.insert(0, '--no-refresh') if skip_verify: @@ -1195,6 +1206,7 @@ def install(name=None, __zypper__(no_repo_failure=ignore_repo_failure).call(*cmd) __context__.pop('pkg.list_pkgs', None) + __context__.pop('pkg.list_list_provides', None) new = list_pkgs(attr=diff_attr) if not downloadonly else list_downloaded() # Handle packages which report multiple new versions @@ -1312,6 +1324,7 @@ def upgrade(refresh=True, __zypper__(systemd_scope=_systemd_scope()).noraise.call(*cmd_update) __context__.pop('pkg.list_pkgs', None) + __context__.pop('pkg.list_list_provides', None) new = list_pkgs() # Handle packages which report multiple new versions @@ -1361,6 +1374,7 @@ def _uninstall(name=None, pkgs=None): targets = targets[500:] __context__.pop('pkg.list_pkgs', None) + __context__.pop('pkg.list_list_provides', None) ret = salt.utils.data.compare_dicts(old, list_pkgs()) if errors: @@ -2109,3 +2123,85 @@ def list_installed_patches(): salt '*' pkg.list_installed_patches ''' return _get_patches(installed_only=True) + +def list_provides(**kwargs): + ''' + List package provides currently installed as a dict. + {'': [' 1: + log.warn("Found ambiguous match for capability '{0}'.".format(pkg)) + + if version: + ret.append({name: version}) + else: + ret.append(name) + return ret diff --git a/salt/states/pkg.py b/salt/states/pkg.py index 3ca512f676..ea61398828 100644 --- a/salt/states/pkg.py +++ b/salt/states/pkg.py @@ -508,8 +508,11 @@ def _find_install_targets(name=None, # add it to the kwargs. kwargs['refresh'] = refresh + + resolve_capabilities = kwargs.get('resolve_capabilities', False) and 'pkg.list_provides' in __salt__ try: cur_pkgs = __salt__['pkg.list_pkgs'](versions_as_list=True, **kwargs) + cur_prov = resolve_capabilities and __salt__['pkg.list_provides'](**kwargs) or dict() except CommandExecutionError as exc: return {'name': name, 'changes': {}, @@ -669,6 +672,9 @@ def _find_install_targets(name=None, failed_verify = False for key, val in six.iteritems(desired): cver = cur_pkgs.get(key, []) + if resolve_capabilities and not cver and key in cur_prov: + cver = cur_pkgs.get(cur_prov.get(key)[0], []) + # Package not yet installed, so add to targets if not cver: targets[key] = val @@ -786,7 +792,7 @@ def _find_install_targets(name=None, warnings, was_refreshed) -def _verify_install(desired, new_pkgs, ignore_epoch=False): +def _verify_install(desired, new_pkgs, ignore_epoch=False, new_caps={}): ''' Determine whether or not the installed packages match what was requested in the SLS file. @@ -809,6 +815,8 @@ def _verify_install(desired, new_pkgs, ignore_epoch=False): cver = new_pkgs.get(pkgname.split('=')[0]) else: cver = new_pkgs.get(pkgname) + if not cver and pkgname in new_caps: + cver = new_pkgs.get(new_caps.get(pkgname)[0]) if not cver: failed.append(pkgname) @@ -872,6 +880,27 @@ def _nested_output(obj): ret = nested.output(obj).rstrip() return ret +def _resolve_capabilities(pkgs, refresh=False, **kwargs): + ''' + Resolve capabilities in ``pkgs`` and exchange them with real package + names, when the result is distinct. + This feature can be turned on while setting the paramter + ``resolve_capabilities`` to True. + + Return the input dictionary with replaced capability names and as + second return value a bool which indicate if a refresh was run. + + In case of ``resolve_capabilities`` is False (disabled) or not + supported by the implementation the input is returned unchanged. + ''' + was_refreshed = False + if not pkgs or 'pkg.resolve_capabilities' not in __salt__: + return (pkgs, was_refreshed) + + ret = __salt__['pkg.resolve_capabilities'](pkgs, refresh=refresh, **kwargs) + was_refreshed = refresh + return (ret, was_refreshed) + def installed( name, @@ -1105,6 +1134,11 @@ def installed( .. versionadded:: 2014.1.1 + :param bool resolve_capabilities: + Turn on resolving capabilities. This allow to name "provides" or alias names for packages. + + .. versionadded:: Oxygen + :param bool allow_updates: Allow the package to be updated outside Salt's control (e.g. auto updates on Windows). This means a package on the Minion can have a @@ -1448,6 +1482,14 @@ def installed( kwargs['saltenv'] = __env__ refresh = salt.utils.pkg.check_refresh(__opts__, refresh) + + # check if capabilities should be checked and modify the requested packages + # accordingly. + if pkgs: + (pkgs, was_refreshed) = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) + if was_refreshed: + refresh = False + if not isinstance(pkg_verify, list): pkg_verify = pkg_verify is True if (pkg_verify or isinstance(pkg_verify, list)) \ @@ -1707,8 +1749,13 @@ def installed( if __grains__['os'] == 'FreeBSD': kwargs['with_origin'] = True new_pkgs = __salt__['pkg.list_pkgs'](versions_as_list=True, **kwargs) + if kwargs.get('resolve_capabilities', False) and 'pkg.list_provides' in __salt__: + new_caps = __salt__['pkg.list_provides'](**kwargs) + else: + new_caps = {} ok, failed = _verify_install(desired, new_pkgs, - ignore_epoch=ignore_epoch) + ignore_epoch=ignore_epoch, + new_caps=new_caps) modified = [x for x in ok if x in targets] not_modified = [x for x in ok if x not in targets @@ -1927,6 +1974,11 @@ def downloaded(name, - dos2unix - salt-minion: 2015.8.5-1.el6 + :param bool resolve_capabilities: + Turn on resolving capabilities. This allow to name "provides" or alias names for packages. + + .. versionadded:: Oxygen + CLI Example: .. code-block:: yaml @@ -1952,11 +2004,24 @@ def downloaded(name, ret['comment'] = 'No packages to download provided' return ret + # If just a name (and optionally a version) is passed, just pack them into + # the pkgs argument. + if name and not pkgs: + if version: + pkgs = [{name: version}] + version = None + else: + pkgs = [name] + # It doesn't make sense here to received 'downloadonly' as kwargs # as we're explicitely passing 'downloadonly=True' to execution module. if 'downloadonly' in kwargs: del kwargs['downloadonly'] + (pkgs, was_refreshed) = _resolve_capabilities(pkgs, **kwargs) + if was_refreshed: + refresh = False + # Only downloading not yet downloaded packages targets = _find_download_targets(name, version, @@ -2203,6 +2268,10 @@ def latest( This parameter is available only on Debian based distributions and has no effect on the rest. + :param bool resolve_capabilities: + Turn on resolving capabilities. This allow to name "provides" or alias names for packages. + + .. versionadded:: Oxygen Multiple Package Installation Options: @@ -2300,6 +2369,12 @@ def latest( kwargs['saltenv'] = __env__ + # check if capabilities should be checked and modify the requested packages + # accordingly. + (desired_pkgs, was_refreshed) = _resolve_capabilities(desired_pkgs, refresh=refresh, **kwargs) + if was_refreshed: + refresh = False + try: avail = __salt__['pkg.latest_version'](*desired_pkgs, fromrepo=fromrepo, @@ -2822,6 +2897,11 @@ def uptodate(name, refresh=False, pkgs=None, **kwargs): This parameter available only on Debian based distributions, and have no effect on the rest. + :param bool resolve_capabilities: + Turn on resolving capabilities. This allow to name "provides" or alias names for packages. + + .. versionadded:: Oxygen + kwargs Any keyword arguments to pass through to ``pkg.upgrade``. @@ -2841,7 +2921,11 @@ def uptodate(name, refresh=False, pkgs=None, **kwargs): ret['comment'] = '\'fromrepo\' argument not supported on this platform' return ret + if isinstance(refresh, bool): + (pkgs, was_refreshed) = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) + if was_refreshed: + refresh = False try: packages = __salt__['pkg.list_upgrades'](refresh=refresh, **kwargs) if isinstance(pkgs, list): From 3d04f7d1fe9e107667a80e5d5dbae6f9073be70b Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Mon, 13 Nov 2017 14:12:26 +0100 Subject: [PATCH 15/34] test package capability feature --- tests/integration/states/test_pkg.py | 265 ++++++++++++++++++++++++++- 1 file changed, 264 insertions(+), 1 deletion(-) diff --git a/tests/integration/states/test_pkg.py b/tests/integration/states/test_pkg.py index 8406f6d631..5412916252 100644 --- a/tests/integration/states/test_pkg.py +++ b/tests/integration/states/test_pkg.py @@ -37,11 +37,15 @@ _PKG_TARGETS = { 'Debian': ['python-plist', 'apg'], 'RedHat': ['units', 'zsh-html'], 'FreeBSD': ['aalib', 'pth'], - 'Suse': ['aalib', 'python-pssh'], + 'Suse': ['aalib', 'rpm-python'], 'MacOS': ['libpng', 'jpeg'], 'Windows': ['firefox', '7zip'], } +_PKG_CAP_TARGETS = { + 'Suse': [('w3m_ssl', 'w3m')], +} + _PKG_TARGETS_32 = { 'CentOS': 'xz-devel.i686' } @@ -793,3 +797,262 @@ class PkgTest(ModuleCase, SaltReturnAssertsMixin): self.assertEqual(ret_comment, 'An error was encountered while installing/updating group ' '\'handle_missing_pkg_group\': Group \'handle_missing_pkg_group\' ' 'not found.') + + @skipIf(salt.utils.platform.is_windows(), 'minion is windows') + @requires_system_grains + def test_pkg_cap_001_installed(self, grains=None): + ''' + This is a destructive test as it installs and then removes a package + ''' + # Skip test if package manager not available + if not pkgmgr_avail(self.run_function, self.run_function('grains.items')): + self.skipTest('Package manager is not available') + + os_family = grains.get('os_family', '') + pkg_cap_targets = _PKG_CAP_TARGETS.get(os_family, []) + if not len(pkg_cap_targets) > 0: + self.skipTest('Capability not provided') + + target, realpkg = pkg_cap_targets[0] + version = self.run_function('pkg.version', [target]) + realver = self.run_function('pkg.version', [realpkg]) + + # If this assert fails, we need to find new targets, this test needs to + # be able to test successful installation of packages, so this package + # needs to not be installed before we run the states below + self.assertFalse(version) + self.assertFalse(realver) + + ret = self.run_state('pkg.installed', name=target, refresh=False, resolve_capabilities=True, test=True) + self.assertInSaltComment("The following packages would be installed/updated: {0}".format(realpkg), ret) + ret = self.run_state('pkg.installed', name=target, refresh=False, resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + ret = self.run_state('pkg.removed', name=realpkg) + self.assertSaltTrueReturn(ret) + + @skipIf(salt.utils.platform.is_windows(), 'minion is windows') + @requires_system_grains + def test_pkg_cap_002_already_installed(self, grains=None): + ''' + This is a destructive test as it installs and then removes a package + ''' + # Skip test if package manager not available + if not pkgmgr_avail(self.run_function, self.run_function('grains.items')): + self.skipTest('Package manager is not available') + + os_family = grains.get('os_family', '') + pkg_cap_targets = _PKG_CAP_TARGETS.get(os_family, []) + if not len(pkg_cap_targets) > 0: + self.skipTest('Capability not provided') + + target, realpkg = pkg_cap_targets[0] + version = self.run_function('pkg.version', [target]) + realver = self.run_function('pkg.version', [realpkg]) + + # If this assert fails, we need to find new targets, this test needs to + # be able to test successful installation of packages, so this package + # needs to not be installed before we run the states below + self.assertFalse(version) + self.assertFalse(realver) + + # install the package already + ret = self.run_state('pkg.installed', name=realpkg, refresh=False) + + ret = self.run_state('pkg.installed', name=target, refresh=False, resolve_capabilities=True, test=True) + self.assertInSaltComment("All specified packages are already installed", ret) + + ret = self.run_state('pkg.installed', name=target, refresh=False, resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + + self.assertInSaltComment("packages are already installed", ret) + ret = self.run_state('pkg.removed', name=realpkg) + self.assertSaltTrueReturn(ret) + + @skipIf(salt.utils.platform.is_windows(), 'minion is windows') + @requires_system_grains + def test_pkg_cap_003_installed_multipkg_with_version(self, grains=None): + ''' + This is a destructive test as it installs and then removes two packages + ''' + # Skip test if package manager not available + if not pkgmgr_avail(self.run_function, self.run_function('grains.items')): + self.skipTest('Package manager is not available') + + os_family = grains.get('os_family', '') + pkg_cap_targets = _PKG_CAP_TARGETS.get(os_family, []) + if not len(pkg_cap_targets) > 0: + self.skipTest('Capability not provided') + pkg_targets = _PKG_TARGETS.get(os_family, []) + + # Don't perform this test on FreeBSD since version specification is not + # supported. + if os_family == 'FreeBSD': + return + + # Make sure that we have targets that match the os_family. If this + # fails then the _PKG_TARGETS dict above needs to have an entry added, + # with two packages that are not installed before these tests are run + self.assertTrue(bool(pkg_cap_targets)) + self.assertTrue(bool(pkg_targets)) + + if os_family == 'Arch': + for idx in range(13): + if idx == 12: + raise Exception('Package database locked after 60 seconds, ' + 'bailing out') + if not os.path.isfile('/var/lib/pacman/db.lck'): + break + time.sleep(5) + + capability, realpkg = pkg_cap_targets[0] + version = latest_version(self.run_function, pkg_targets[0]) + realver = latest_version(self.run_function, realpkg) + + # If this assert fails, we need to find new targets, this test needs to + # be able to test successful installation of packages, so these + # packages need to not be installed before we run the states below + self.assertTrue(bool(version)) + self.assertTrue(bool(realver)) + + pkgs = [{pkg_targets[0]: version}, pkg_targets[1], {capability: realver}] + ret = self.run_state('pkg.installed', + name='test_pkg_cap_003_installed_multipkg_with_version-install', + pkgs=pkgs, + refresh=False) + self.assertSaltFalseReturn(ret) + + ret = self.run_state('pkg.installed', + name='test_pkg_cap_003_installed_multipkg_with_version-install-capability', + pkgs=pkgs, + refresh=False, resolve_capabilities=True, test=True) + self.assertInSaltComment("packages would be installed/updated", ret) + self.assertInSaltComment("{0}={1}".format(realpkg, realver), ret) + + ret = self.run_state('pkg.installed', + name='test_pkg_cap_003_installed_multipkg_with_version-install-capability', + pkgs=pkgs, + refresh=False, resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + cleanup_pkgs = pkg_targets + cleanup_pkgs.append(realpkg) + ret = self.run_state('pkg.removed', + name='test_pkg_cap_003_installed_multipkg_with_version-remove', + pkgs=cleanup_pkgs) + self.assertSaltTrueReturn(ret) + + @skipIf(salt.utils.platform.is_windows(), 'minion is windows') + @requires_system_grains + def test_pkg_cap_004_latest(self, grains=None): + ''' + This tests pkg.latest with a package that has no epoch (or a zero + epoch). + ''' + # Skip test if package manager not available + if not pkgmgr_avail(self.run_function, self.run_function('grains.items')): + self.skipTest('Package manager is not available') + + os_family = grains.get('os_family', '') + pkg_cap_targets = _PKG_CAP_TARGETS.get(os_family, []) + if not len(pkg_cap_targets) > 0: + self.skipTest('Capability not provided') + + target, realpkg = pkg_cap_targets[0] + version = self.run_function('pkg.version', [target]) + realver = self.run_function('pkg.version', [realpkg]) + + # If this assert fails, we need to find new targets, this test needs to + # be able to test successful installation of packages, so this package + # needs to not be installed before we run the states below + self.assertFalse(version) + self.assertFalse(realver) + + ret = self.run_state('pkg.latest', name=target, refresh=False, resolve_capabilities=True, test=True) + self.assertInSaltComment("The following packages would be installed/upgraded: {0}".format(realpkg), ret) + ret = self.run_state('pkg.latest', name=target, refresh=False, resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + + ret = self.run_state('pkg.latest', name=target, refresh=False, resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + self.assertInSaltComment("is already up-to-date", ret) + + ret = self.run_state('pkg.removed', name=realpkg) + self.assertSaltTrueReturn(ret) + + @skipIf(salt.utils.platform.is_windows(), 'minion is windows') + @requires_system_grains + def test_pkg_cap_005_downloaded(self, grains=None): + ''' + This is a destructive test as it installs and then removes a package + ''' + # Skip test if package manager not available + if not pkgmgr_avail(self.run_function, self.run_function('grains.items')): + self.skipTest('Package manager is not available') + + os_family = grains.get('os_family', '') + pkg_cap_targets = _PKG_CAP_TARGETS.get(os_family, []) + if not len(pkg_cap_targets) > 0: + self.skipTest('Capability not provided') + + target, realpkg = pkg_cap_targets[0] + version = self.run_function('pkg.version', [target]) + realver = self.run_function('pkg.version', [realpkg]) + + # If this assert fails, we need to find new targets, this test needs to + # be able to test successful installation of packages, so this package + # needs to not be installed before we run the states below + self.assertFalse(version) + self.assertFalse(realver) + + ret = self.run_state('pkg.downloaded', name=target, refresh=False) + self.assertSaltFalseReturn(ret) + + ret = self.run_state('pkg.downloaded', name=target, refresh=False, resolve_capabilities=True, test=True) + self.assertInSaltComment("The following packages would be downloaded: {0}".format(realpkg), ret) + + ret = self.run_state('pkg.downloaded', name=target, refresh=False, resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + + @skipIf(salt.utils.platform.is_windows(), 'minion is windows') + @requires_system_grains + def test_pkg_cap_006_uptodate(self, grains=None): + ''' + This is a destructive test as it installs and then removes a package + ''' + # Skip test if package manager not available + if not pkgmgr_avail(self.run_function, self.run_function('grains.items')): + self.skipTest('Package manager is not available') + + os_family = grains.get('os_family', '') + pkg_cap_targets = _PKG_CAP_TARGETS.get(os_family, []) + if not len(pkg_cap_targets) > 0: + self.skipTest('Capability not provided') + + target, realpkg = pkg_cap_targets[0] + version = self.run_function('pkg.version', [target]) + realver = self.run_function('pkg.version', [realpkg]) + + # If this assert fails, we need to find new targets, this test needs to + # be able to test successful installation of packages, so this package + # needs to not be installed before we run the states below + self.assertFalse(version) + self.assertFalse(realver) + + ret = self.run_state('pkg.installed', name=target, + refresh=False, resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + ret = self.run_state('pkg.uptodate', + name='test_pkg_cap_006_uptodate', + pkgs=[target], + refresh=False, + resolve_capabilities=True) + self.assertSaltTrueReturn(ret) + self.assertInSaltComment("System is already up-to-date", ret) + ret = self.run_state('pkg.removed', name=realpkg) + self.assertSaltTrueReturn(ret) + ret = self.run_state('pkg.uptodate', + name='test_pkg_cap_006_uptodate', + refresh=False, + test=True) + self.assertInSaltComment("System update will be performed", ret) + + From 8433491caeabed3a42a9486a2a4960b103c4a1d9 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 09:02:06 +0100 Subject: [PATCH 16/34] fix list_provides cache handling --- salt/modules/zypper.py | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index e4c7876943..ff38ebc809 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -1206,7 +1206,7 @@ def install(name=None, __zypper__(no_repo_failure=ignore_repo_failure).call(*cmd) __context__.pop('pkg.list_pkgs', None) - __context__.pop('pkg.list_list_provides', None) + __context__.pop('pkg.list_provides', None) new = list_pkgs(attr=diff_attr) if not downloadonly else list_downloaded() # Handle packages which report multiple new versions @@ -1324,7 +1324,7 @@ def upgrade(refresh=True, __zypper__(systemd_scope=_systemd_scope()).noraise.call(*cmd_update) __context__.pop('pkg.list_pkgs', None) - __context__.pop('pkg.list_list_provides', None) + __context__.pop('pkg.list_provides', None) new = list_pkgs() # Handle packages which report multiple new versions @@ -1374,7 +1374,7 @@ def _uninstall(name=None, pkgs=None): targets = targets[500:] __context__.pop('pkg.list_pkgs', None) - __context__.pop('pkg.list_list_provides', None) + __context__.pop('pkg.list_provides', None) ret = salt.utils.data.compare_dicts(old, list_pkgs()) if errors: @@ -2129,22 +2129,20 @@ def list_provides(**kwargs): List package provides currently installed as a dict. {'': [' Date: Thu, 16 Nov 2017 09:02:38 +0100 Subject: [PATCH 17/34] improve search function --- salt/modules/zypper.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index ff38ebc809..e8621b2758 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -1823,7 +1823,7 @@ def search(criteria, refresh=False, **kwargs): if refresh: refresh_db() - cmd = ['se'] + cmd = ['search'] if kwargs.get('match') == 'exact': cmd.append('--match-exact') elif kwargs.get('match') == 'words': @@ -1853,7 +1853,8 @@ def search(criteria, refresh=False, **kwargs): if kwargs.get('installed_only'): cmd.append('--installed-only') if kwargs.get('not_installed_only'): - cmd.append('--not-installed-only') + # long parameter was renamed in the past + cmd.append('-u') if kwargs.get('details'): cmd.append('--details') From 06b2d25371fa5e5ec78f6effa49779b36ebd505e Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 09:03:01 +0100 Subject: [PATCH 18/34] simplify argument handling --- salt/modules/zypper.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index e8621b2758..8150742836 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -1171,10 +1171,7 @@ def install(name=None, fromrepoopt = '' cmd_install = ['install', '--auto-agree-with-licenses'] - if kwargs.get('resolve_capabilities', False): - cmd_install.append('--capability') - else: - cmd_install.append('--name') + cmd_install.append(kwargs.get('resolve_capabilities') and '--capability' or '--name') if not refresh: cmd_install.insert(0, '--no-refresh') From ab9f5768b71d008a1b1b857f75c1f9da78cc72c5 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 09:10:11 +0100 Subject: [PATCH 19/34] remove unused variable and simplify dict size checking --- salt/modules/zypper.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index 8150742836..dbb26c0ff2 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -2187,13 +2187,13 @@ def resolve_capabilities(pkgs, refresh, **kwargs): if kwargs.get('resolve_capabilities', False): try: search(name, match='exact') - except CommandExecutionError as e: + except CommandExecutionError: # no package this such a name found # search for a package which provides this name result = search(name, provides=True, match='exact') - if len(result.keys()) == 1: + if len(result) == 1: name = result.keys()[0] - elif len(result.keys()) > 1: + elif len(result) > 1: log.warn("Found ambiguous match for capability '{0}'.".format(pkg)) if version: From e0764a7362d26be8a09e39d6d384ae72d1f11ac9 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 09:12:04 +0100 Subject: [PATCH 20/34] catch search exceptions --- salt/modules/zypper.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index dbb26c0ff2..4a23afc14b 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -2190,11 +2190,15 @@ def resolve_capabilities(pkgs, refresh, **kwargs): except CommandExecutionError: # no package this such a name found # search for a package which provides this name - result = search(name, provides=True, match='exact') - if len(result) == 1: - name = result.keys()[0] - elif len(result) > 1: - log.warn("Found ambiguous match for capability '{0}'.".format(pkg)) + try: + result = search(name, provides=True, match='exact') + if len(result) == 1: + name = result.keys()[0] + elif len(result) > 1: + log.warn("Found ambiguous match for capability '{0}'.".format(pkg)) + except CommandExecutionError: + # when search throw an exception stay with original name and version + pass if version: ret.append({name: version}) From facbf90186f5a5d24cbc85fa48a8bb34272392b0 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 09:25:21 +0100 Subject: [PATCH 21/34] remove extra parenthesis --- salt/states/pkg.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/salt/states/pkg.py b/salt/states/pkg.py index ea61398828..a3a704088f 100644 --- a/salt/states/pkg.py +++ b/salt/states/pkg.py @@ -1486,7 +1486,7 @@ def installed( # check if capabilities should be checked and modify the requested packages # accordingly. if pkgs: - (pkgs, was_refreshed) = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) + pkgs, was_refreshed = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) if was_refreshed: refresh = False @@ -2018,7 +2018,7 @@ def downloaded(name, if 'downloadonly' in kwargs: del kwargs['downloadonly'] - (pkgs, was_refreshed) = _resolve_capabilities(pkgs, **kwargs) + pkgs, was_refreshed = _resolve_capabilities(pkgs, **kwargs) if was_refreshed: refresh = False @@ -2371,7 +2371,7 @@ def latest( # check if capabilities should be checked and modify the requested packages # accordingly. - (desired_pkgs, was_refreshed) = _resolve_capabilities(desired_pkgs, refresh=refresh, **kwargs) + desired_pkgs, was_refreshed = _resolve_capabilities(desired_pkgs, refresh=refresh, **kwargs) if was_refreshed: refresh = False @@ -2923,7 +2923,7 @@ def uptodate(name, refresh=False, pkgs=None, **kwargs): if isinstance(refresh, bool): - (pkgs, was_refreshed) = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) + pkgs, was_refreshed = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) if was_refreshed: refresh = False try: From c77afffe485c523fbf58735b6ba33836d8a251f2 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 09:36:57 +0100 Subject: [PATCH 22/34] simplify refresh handling --- salt/states/pkg.py | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/salt/states/pkg.py b/salt/states/pkg.py index a3a704088f..730610daef 100644 --- a/salt/states/pkg.py +++ b/salt/states/pkg.py @@ -888,18 +888,16 @@ def _resolve_capabilities(pkgs, refresh=False, **kwargs): ``resolve_capabilities`` to True. Return the input dictionary with replaced capability names and as - second return value a bool which indicate if a refresh was run. + second return value a bool which say if a refresh need to be run. In case of ``resolve_capabilities`` is False (disabled) or not supported by the implementation the input is returned unchanged. ''' - was_refreshed = False if not pkgs or 'pkg.resolve_capabilities' not in __salt__: - return (pkgs, was_refreshed) + return pkgs, refresh ret = __salt__['pkg.resolve_capabilities'](pkgs, refresh=refresh, **kwargs) - was_refreshed = refresh - return (ret, was_refreshed) + return ret, False def installed( @@ -1486,9 +1484,7 @@ def installed( # check if capabilities should be checked and modify the requested packages # accordingly. if pkgs: - pkgs, was_refreshed = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) - if was_refreshed: - refresh = False + pkgs, refresh = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) if not isinstance(pkg_verify, list): pkg_verify = pkg_verify is True @@ -2018,9 +2014,7 @@ def downloaded(name, if 'downloadonly' in kwargs: del kwargs['downloadonly'] - pkgs, was_refreshed = _resolve_capabilities(pkgs, **kwargs) - if was_refreshed: - refresh = False + pkgs, _refresh = _resolve_capabilities(pkgs, **kwargs) # Only downloading not yet downloaded packages targets = _find_download_targets(name, @@ -2371,9 +2365,7 @@ def latest( # check if capabilities should be checked and modify the requested packages # accordingly. - desired_pkgs, was_refreshed = _resolve_capabilities(desired_pkgs, refresh=refresh, **kwargs) - if was_refreshed: - refresh = False + desired_pkgs, refresh = _resolve_capabilities(desired_pkgs, refresh=refresh, **kwargs) try: avail = __salt__['pkg.latest_version'](*desired_pkgs, @@ -2923,7 +2915,7 @@ def uptodate(name, refresh=False, pkgs=None, **kwargs): if isinstance(refresh, bool): - pkgs, was_refreshed = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) + pkgs, refresh = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) if was_refreshed: refresh = False try: From 966d85d39386256f5140b63678a5c610af48d166 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 09:59:06 +0100 Subject: [PATCH 23/34] better way to get key/value from a one item dict --- salt/modules/zypper.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index 4a23afc14b..a02d238966 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -2178,8 +2178,8 @@ def resolve_capabilities(pkgs, refresh, **kwargs): ret = list() for pkg in pkgs: if isinstance(pkg, dict): - for name, version in pkg.items(): - break + name = next(iter(pkg)) + version = pkg[name] else: name = pkg version = None From b6365aee3c46d37a9409f222439a025561b4ea1f Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 13:13:11 +0100 Subject: [PATCH 24/34] fix order of parameters in unit tests --- tests/unit/modules/test_zypper.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/modules/test_zypper.py b/tests/unit/modules/test_zypper.py index 37852e155e..e68f8a4d9f 100644 --- a/tests/unit/modules/test_zypper.py +++ b/tests/unit/modules/test_zypper.py @@ -669,8 +669,8 @@ Repository 'DUMMY' not found by its alias, number, or URI. zypper_mock.assert_called_once_with( '--no-refresh', 'install', - '--name', '--auto-agree-with-licenses', + '--name', '--download-only', 'vim' ) @@ -699,8 +699,8 @@ Repository 'DUMMY' not found by its alias, number, or URI. zypper_mock.assert_called_once_with( '--no-refresh', 'install', - '--name', '--auto-agree-with-licenses', + '--name', '--download-only', 'vim' ) @@ -724,8 +724,8 @@ Repository 'DUMMY' not found by its alias, number, or URI. zypper_mock.assert_called_once_with( '--no-refresh', 'install', - '--name', '--auto-agree-with-licenses', + '--name', 'patch:SUSE-PATCH-1234' ) self.assertDictEqual(ret, {"vim": {"old": "1.1", "new": "1.2"}}) From f3f1abe5ff8da00245be199f4ece3352c6248453 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 14:25:19 +0100 Subject: [PATCH 25/34] pylint fixes --- salt/modules/zypper.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index a02d238966..9cb767da17 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -20,7 +20,6 @@ import re import os import time import datetime -import copy # Import 3rd-party libs # pylint: disable=import-error,redefined-builtin,no-name-in-module @@ -1865,7 +1864,7 @@ def search(criteria, refresh=False, **kwargs): out = {} for solvable in solvables: out[solvable.getAttribute('name')] = dict() - for k,v in solvable.attributes.items(): + for k, v in solvable.attributes.items(): out[solvable.getAttribute('name')][k] = v return out @@ -2122,6 +2121,7 @@ def list_installed_patches(): ''' return _get_patches(installed_only=True) + def list_provides(**kwargs): ''' List package provides currently installed as a dict. @@ -2144,6 +2144,7 @@ def list_provides(**kwargs): return ret + def resolve_capabilities(pkgs, refresh, **kwargs): ''' .. versionadded:: Oxygen From b6e60fa7a86df5c10355f15cdcf109e16ddecf1b Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 14:30:53 +0100 Subject: [PATCH 26/34] write function to clean caches --- salt/modules/zypper.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index 9cb767da17..ff77042dcd 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -399,6 +399,14 @@ def _systemd_scope(): and __salt__['config.get']('systemd.scope', True) +def _clean_cache(): + ''' + Clean cached results + ''' + for cache_name in ['list_pkgs', 'list_provides']: + __context__.pop(cache_name, None) + + def list_upgrades(refresh=True, **kwargs): ''' List all available package upgrades on this system @@ -1201,8 +1209,7 @@ def install(name=None, downgrades = downgrades[500:] __zypper__(no_repo_failure=ignore_repo_failure).call(*cmd) - __context__.pop('pkg.list_pkgs', None) - __context__.pop('pkg.list_provides', None) + _clean_cache() new = list_pkgs(attr=diff_attr) if not downloadonly else list_downloaded() # Handle packages which report multiple new versions @@ -1319,8 +1326,7 @@ def upgrade(refresh=True, old = list_pkgs() __zypper__(systemd_scope=_systemd_scope()).noraise.call(*cmd_update) - __context__.pop('pkg.list_pkgs', None) - __context__.pop('pkg.list_provides', None) + _clean_cache() new = list_pkgs() # Handle packages which report multiple new versions @@ -1369,8 +1375,7 @@ def _uninstall(name=None, pkgs=None): __zypper__(systemd_scope=systemd_scope).call('remove', *targets[:500]) targets = targets[500:] - __context__.pop('pkg.list_pkgs', None) - __context__.pop('pkg.list_provides', None) + _clean_cache() ret = salt.utils.data.compare_dicts(old, list_pkgs()) if errors: From 55553bab96cb125bd06ff8f2317371917e958b75 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 14:33:52 +0100 Subject: [PATCH 27/34] log debug message when ignoring exceptions --- salt/modules/zypper.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index ff77042dcd..42afb1f799 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -2202,8 +2202,9 @@ def resolve_capabilities(pkgs, refresh, **kwargs): name = result.keys()[0] elif len(result) > 1: log.warn("Found ambiguous match for capability '{0}'.".format(pkg)) - except CommandExecutionError: - # when search throw an exception stay with original name and version + except CommandExecutionError as exc: + # when search throws an exception stay with original name and version + log.debug("Search failed with: {0}".format(exc)) pass if version: From 1cdd741dcfb3752001af89779455ac3733071933 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 14:36:44 +0100 Subject: [PATCH 28/34] fix cache name --- salt/modules/zypper.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index 42afb1f799..16e2b987df 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -403,8 +403,8 @@ def _clean_cache(): ''' Clean cached results ''' - for cache_name in ['list_pkgs', 'list_provides']: - __context__.pop(cache_name, None) + for cache_name in ['pkg.list_pkgs', 'pkg.list_provides']: + __context__.pop(cache_name, None) def list_upgrades(refresh=True, **kwargs): From e42fe24dd7273eda33991f62d8c2f7f51fc9d7c6 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 15:10:24 +0100 Subject: [PATCH 29/34] use mapping for allowed search option --- salt/modules/zypper.py | 43 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index 16e2b987df..c2017bc917 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -1821,6 +1821,20 @@ def search(criteria, refresh=False, **kwargs): salt '*' pkg.search ''' + ALLOWED_SEARCH_OPTIONS = { + 'provides': '--provides', + 'recommends': '--recommends', + 'requires': '--requires', + 'suggests': '--suggests', + 'conflicts': '--conflicts', + 'obsoletes': '--obsoletes', + 'file_list': '--file-list', + 'search_descriptions': '--search-descriptions', + 'case_sensitive': '--case-sensitive', + 'installed_only': '--installed-only', + 'not_installed_only': '-u', + 'details': '--details' + } if refresh: refresh_db() @@ -1832,32 +1846,9 @@ def search(criteria, refresh=False, **kwargs): elif kwargs.get('match') == 'substrings': cmd.append('--match-substrings') - if kwargs.get('provides'): - cmd.append('--provides') - if kwargs.get('recommends'): - cmd.append('--recommends') - if kwargs.get('requires'): - cmd.append('--requires') - if kwargs.get('suggests'): - cmd.append('--suggests') - if kwargs.get('conflicts'): - cmd.append('--conflicts') - if kwargs.get('obsoletes'): - cmd.append('--obsoletes') - - if kwargs.get('file_list'): - cmd.append('--file-list') - if kwargs.get('search_descriptions'): - cmd.append('--search-descriptions') - if kwargs.get('case_sensitive'): - cmd.append('--case-sensitive') - if kwargs.get('installed_only'): - cmd.append('--installed-only') - if kwargs.get('not_installed_only'): - # long parameter was renamed in the past - cmd.append('-u') - if kwargs.get('details'): - cmd.append('--details') + for opt in kwargs: + if opt in ALLOWED_SEARCH_OPTIONS: + cmd.append(ALLOWED_SEARCH_OPTIONS.get(opt)) cmd.append(criteria) solvables = __zypper__.nolock.noraise.xml.call(*cmd).getElementsByTagName('solvable') From bbf542d5d156419a971bdfcbb07c2df8a9ab9519 Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Thu, 16 Nov 2017 19:06:00 +0100 Subject: [PATCH 30/34] pylint fixes --- salt/modules/zypper.py | 1 - salt/states/pkg.py | 9 ++++----- tests/integration/states/test_pkg.py | 2 -- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index c2017bc917..b5437ca613 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -2196,7 +2196,6 @@ def resolve_capabilities(pkgs, refresh, **kwargs): except CommandExecutionError as exc: # when search throws an exception stay with original name and version log.debug("Search failed with: {0}".format(exc)) - pass if version: ret.append({name: version}) diff --git a/salt/states/pkg.py b/salt/states/pkg.py index 730610daef..d438cd8210 100644 --- a/salt/states/pkg.py +++ b/salt/states/pkg.py @@ -508,7 +508,6 @@ def _find_install_targets(name=None, # add it to the kwargs. kwargs['refresh'] = refresh - resolve_capabilities = kwargs.get('resolve_capabilities', False) and 'pkg.list_provides' in __salt__ try: cur_pkgs = __salt__['pkg.list_pkgs'](versions_as_list=True, **kwargs) @@ -792,13 +791,15 @@ def _find_install_targets(name=None, warnings, was_refreshed) -def _verify_install(desired, new_pkgs, ignore_epoch=False, new_caps={}): +def _verify_install(desired, new_pkgs, ignore_epoch=False, new_caps=None): ''' Determine whether or not the installed packages match what was requested in the SLS file. ''' ok = [] failed = [] + if not new_caps: + new_caps = dict() for pkgname, pkgver in desired.items(): # FreeBSD pkg supports `openjdk` and `java/openjdk7` package names. # Homebrew for Mac OSX does something similar with tap names @@ -880,6 +881,7 @@ def _nested_output(obj): ret = nested.output(obj).rstrip() return ret + def _resolve_capabilities(pkgs, refresh=False, **kwargs): ''' Resolve capabilities in ``pkgs`` and exchange them with real package @@ -2913,11 +2915,8 @@ def uptodate(name, refresh=False, pkgs=None, **kwargs): ret['comment'] = '\'fromrepo\' argument not supported on this platform' return ret - if isinstance(refresh, bool): pkgs, refresh = _resolve_capabilities(pkgs, refresh=refresh, **kwargs) - if was_refreshed: - refresh = False try: packages = __salt__['pkg.list_upgrades'](refresh=refresh, **kwargs) if isinstance(pkgs, list): diff --git a/tests/integration/states/test_pkg.py b/tests/integration/states/test_pkg.py index 5412916252..24243c424d 100644 --- a/tests/integration/states/test_pkg.py +++ b/tests/integration/states/test_pkg.py @@ -1054,5 +1054,3 @@ class PkgTest(ModuleCase, SaltReturnAssertsMixin): refresh=False, test=True) self.assertInSaltComment("System update will be performed", ret) - - From dbcd6d7311d615a20492cd99af683cc6fec427ff Mon Sep 17 00:00:00 2001 From: Michael Calmer Date: Tue, 21 Nov 2017 09:02:51 +0100 Subject: [PATCH 31/34] improve documentation of list_provides --- salt/modules/zypper.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/salt/modules/zypper.py b/salt/modules/zypper.py index b5437ca613..b831142c83 100644 --- a/salt/modules/zypper.py +++ b/salt/modules/zypper.py @@ -2120,8 +2120,16 @@ def list_installed_patches(): def list_provides(**kwargs): ''' - List package provides currently installed as a dict. - {'': ['': ['', '', ...]} + + CLI Examples: + + .. code-block:: bash + + salt '*' pkg.list_provides ''' ret = __context__.get('pkg.list_provides') if not ret: From ee3486a671a5b27e0ee9931113af9c33e3dc04af Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Wed, 29 Nov 2017 14:16:29 -0600 Subject: [PATCH 32/34] Include failed state/function orchestration results in changes dict Before this commit they would be in the comment, making it difficult to programatically analyze the failures. --- salt/states/saltmod.py | 30 +---------- .../file/base/_modules/runtests_helpers.py | 23 ++++++++ .../orch/issue43204/fail_with_changes.sls | 2 + .../files/file/base/orch/issue43204/init.sls | 11 ++++ tests/integration/runners/test_state.py | 53 +++++++++++++++++++ 5 files changed, 91 insertions(+), 28 deletions(-) create mode 100644 tests/integration/files/file/base/orch/issue43204/fail_with_changes.sls create mode 100644 tests/integration/files/file/base/orch/issue43204/init.sls diff --git a/salt/states/saltmod.py b/salt/states/saltmod.py index 0470c7f870..d9e12abdc0 100644 --- a/salt/states/saltmod.py +++ b/salt/states/saltmod.py @@ -351,7 +351,6 @@ def state(name, changes = {} fail = set() - failures = {} no_change = set() if fail_minions is None: @@ -393,7 +392,7 @@ def state(name, if not m_state: if minion not in fail_minions: fail.add(minion) - failures[minion] = m_ret or 'Minion did not respond' + changes[minion] = m_ret continue try: for state_item in six.itervalues(m_ret): @@ -418,18 +417,6 @@ def state(name, state_ret['comment'] += ' Updating {0}.'.format(', '.join(changes)) if no_change: state_ret['comment'] += ' No changes made to {0}.'.format(', '.join(no_change)) - if failures: - state_ret['comment'] += '\nFailures:\n' - for minion, failure in six.iteritems(failures): - state_ret['comment'] += '\n'.join( - (' ' * 4 + l) - for l in salt.output.out_format( - {minion: failure}, - 'highstate', - __opts__, - ).splitlines() - ) - state_ret['comment'] += '\n' if test or __opts__.get('test'): if state_ret['changes'] and state_ret['result'] is True: # Test mode with changes is the only case where result should ever be none @@ -570,7 +557,6 @@ def function( changes = {} fail = set() - failures = {} if fail_minions is None: fail_minions = () @@ -598,7 +584,7 @@ def function( if not m_func: if minion not in fail_minions: fail.add(minion) - failures[minion] = m_ret and m_ret or 'Minion did not respond' + changes[minion] = m_ret continue changes[minion] = m_ret if not cmd_ret: @@ -614,18 +600,6 @@ def function( func_ret['comment'] = 'Function ran successfully.' if changes: func_ret['comment'] += ' Function {0} ran on {1}.'.format(name, ', '.join(changes)) - if failures: - func_ret['comment'] += '\nFailures:\n' - for minion, failure in six.iteritems(failures): - func_ret['comment'] += '\n'.join( - (' ' * 4 + l) - for l in salt.output.out_format( - {minion: failure}, - 'highstate', - __opts__, - ).splitlines() - ) - func_ret['comment'] += '\n' return func_ret diff --git a/tests/integration/files/file/base/_modules/runtests_helpers.py b/tests/integration/files/file/base/_modules/runtests_helpers.py index f3d4909537..f3e21ebe81 100644 --- a/tests/integration/files/file/base/_modules/runtests_helpers.py +++ b/tests/integration/files/file/base/_modules/runtests_helpers.py @@ -125,3 +125,26 @@ def modules_available(*names): if not fnmatch.filter(list(__salt__), name): not_found.append(name) return not_found + + +def nonzero_retcode_return_true(): + ''' + Sets a nonzero retcode before returning. Designed to test orchestration. + ''' + __context__['retcode'] = 1 + return True + + +def nonzero_retcode_return_false(): + ''' + Sets a nonzero retcode before returning. Designed to test orchestration. + ''' + __context__['retcode'] = 1 + return False + + +def fail_function(*args, **kwargs): # pylint: disable=unused-argument + ''' + Return False no matter what is passed to it + ''' + return False diff --git a/tests/integration/files/file/base/orch/issue43204/fail_with_changes.sls b/tests/integration/files/file/base/orch/issue43204/fail_with_changes.sls new file mode 100644 index 0000000000..365210e8b2 --- /dev/null +++ b/tests/integration/files/file/base/orch/issue43204/fail_with_changes.sls @@ -0,0 +1,2 @@ +test fail with changes: + test.fail_with_changes diff --git a/tests/integration/files/file/base/orch/issue43204/init.sls b/tests/integration/files/file/base/orch/issue43204/init.sls new file mode 100644 index 0000000000..5617c90bd9 --- /dev/null +++ b/tests/integration/files/file/base/orch/issue43204/init.sls @@ -0,0 +1,11 @@ +Step01: + salt.state: + - tgt: 'minion' + - sls: + - orch.issue43204.fail_with_changes + +Step02: + salt.function: + - name: runtests_helpers.nonzero_retcode_return_false + - tgt: 'minion' + - fail_function: runtests_helpers.fail_function diff --git a/tests/integration/runners/test_state.py b/tests/integration/runners/test_state.py index 4d1e543c0f..9ed20b6761 100644 --- a/tests/integration/runners/test_state.py +++ b/tests/integration/runners/test_state.py @@ -6,6 +6,7 @@ Tests for the state runner # Import Python Libs from __future__ import absolute_import import errno +import json import os import shutil import signal @@ -81,6 +82,58 @@ class StateRunnerTest(ShellCase): self.assertFalse(os.path.exists('/tmp/ewu-2016-12-13')) self.assertNotEqual(code, 0) + def test_orchestrate_state_and_function_failure(self): + ''' + Ensure that returns from failed minions are in the changes dict where + they belong, so they can be programatically analyzed. + + See https://github.com/saltstack/salt/issues/43204 + ''' + self.run_run('saltutil.sync_modules') + ret = json.loads( + '\n'.join( + self.run_run(u'state.orchestrate orch.issue43204 --out=json') + ) + ) + # Drill down to the changes dict + state_ret = ret[u'data'][u'master'][u'salt_|-Step01_|-Step01_|-state'][u'changes'] + func_ret = ret[u'data'][u'master'][u'salt_|-Step02_|-runtests_helpers.nonzero_retcode_return_false_|-function'][u'changes'] + + # Remove duration and start time from the results, since they would + # vary with each run and that would make it impossible to test. + for item in ('duration', 'start_time'): + state_ret['ret']['minion']['test_|-test fail with changes_|-test fail with changes_|-fail_with_changes'].pop(item) + + self.assertEqual( + state_ret, + { + u'out': u'highstate', + u'ret': { + u'minion': { + u'test_|-test fail with changes_|-test fail with changes_|-fail_with_changes': { + u'__id__': u'test fail with changes', + u'__run_num__': 0, + u'__sls__': u'orch.issue43204.fail_with_changes', + u'changes': { + u'testing': { + u'new': u'Something pretended to change', + u'old': u'Unchanged' + } + }, + u'comment': u'Failure!', + u'name': u'test fail with changes', + u'result': False, + } + } + } + } + ) + + self.assertEqual( + func_ret, + {u'out': u'highstate', u'ret': {u'minion': False}} + ) + def test_orchestrate_target_exists(self): ''' test orchestration when target exists From abed8e5952daa9ce768f962506470c459e5d444d Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Thu, 30 Nov 2017 23:48:54 -0600 Subject: [PATCH 33/34] Add documentation / release notes --- doc/topics/orchestrate/orchestrate_runner.rst | 303 ++++++++++++++++++ doc/topics/releases/oxygen.rst | 12 + 2 files changed, 315 insertions(+) diff --git a/doc/topics/orchestrate/orchestrate_runner.rst b/doc/topics/orchestrate/orchestrate_runner.rst index 82d8facde9..9a651db571 100644 --- a/doc/topics/orchestrate/orchestrate_runner.rst +++ b/doc/topics/orchestrate/orchestrate_runner.rst @@ -137,6 +137,43 @@ can specify the "name" argument to avoid conflicting IDs: - kwarg: remove_existing: true +.. _orchestrate-runner-fail-functions: + +Fail Functions +************** + +When running a remote execution function in orchestration, certain return +values for those functions may indicate failure, while the function itself +doesn't set a return code. For those circumstances, using a "fail function" +allows for a more flexible means of assessing success or failure. + +A fail function can be written as part of a :ref:`custom execution module +`. The function should accept one argument, and +return a boolean result. For example: + +.. code-block:: python + + def check_func_result(retval): + if some_condition: + return True + else: + return False + + +The function can then be referenced in orchestration SLS like so: + +.. code-block:: yaml + + do_stuff: + salt.function: + - name: modname.funcname + - tgt: '*' + - fail_function: mymod.check_func_result + +.. important:: + Fail functions run *on the master*, so they must be synced using ``salt-run + saltutil.sync_modules``. + State ^^^^^ @@ -298,3 +335,269 @@ Given the above setup, the orchestration will be carried out as follows: .. note:: Remember, salt-run is always executed on the master. + +.. _orchestrate-runner-parsing-results-programatically: + +Parsing Results Programatically +------------------------------- + +Orchestration jobs return output in a specific data structure. That data +structure is represented differently depending on the outputter used. With the +default outputter for orchestration, you get a nice human-readable output. +Assume the following orchestration SLS: + +.. code-block:: yaml + + good_state: + salt.state: + - tgt: myminion + - sls: + - succeed_with_changes + + bad_state: + salt.state: + - tgt: myminion + - sls: + - fail_with_changes + + mymod.myfunc: + salt.function: + - tgt: myminion + + mymod.myfunc_false_result: + salt.function: + - tgt: myminion + + +Running this using the default outputter would produce output which looks like +this: + +.. code-block:: text + + fa5944a73aa8_master: + ---------- + ID: good_state + Function: salt.state + Result: True + Comment: States ran successfully. Updating myminion. + Started: 21:08:02.681604 + Duration: 265.565 ms + Changes: + myminion: + ---------- + ID: test succeed with changes + Function: test.succeed_with_changes + Result: True + Comment: Success! + Started: 21:08:02.835893 + Duration: 0.375 ms + Changes: + ---------- + testing: + ---------- + new: + Something pretended to change + old: + Unchanged + + Summary for myminion + ------------ + Succeeded: 1 (changed=1) + Failed: 0 + ------------ + Total states run: 1 + Total run time: 0.375 ms + ---------- + ID: bad_state + Function: salt.state + Result: False + Comment: Run failed on minions: myminion + Started: 21:08:02.947702 + Duration: 177.01 ms + Changes: + myminion: + ---------- + ID: test fail with changes + Function: test.fail_with_changes + Result: False + Comment: Failure! + Started: 21:08:03.116634 + Duration: 0.502 ms + Changes: + ---------- + testing: + ---------- + new: + Something pretended to change + old: + Unchanged + + Summary for myminion + ------------ + Succeeded: 0 (changed=1) + Failed: 1 + ------------ + Total states run: 1 + Total run time: 0.502 ms + ---------- + ID: mymod.myfunc + Function: salt.function + Result: True + Comment: Function ran successfully. Function mymod.myfunc ran on myminion. + Started: 21:08:03.125011 + Duration: 159.488 ms + Changes: + myminion: + True + ---------- + ID: mymod.myfunc_false_result + Function: salt.function + Result: False + Comment: Running function mymod.myfunc_false_result failed on minions: myminion. Function mymod.myfunc_false_result ran on myminion. + Started: 21:08:03.285148 + Duration: 176.787 ms + Changes: + myminion: + False + + Summary for fa5944a73aa8_master + ------------ + Succeeded: 2 (changed=4) + Failed: 2 + ------------ + Total states run: 4 + Total run time: 778.850 ms + + +However, using the ``json`` outputter, you can get the output in an easily +loadable and parsable format: + +.. code-block:: bash + + salt-run state.orchestrate test --out=json + +.. code-block:: json + + { + "outputter": "highstate", + "data": { + "fa5944a73aa8_master": { + "salt_|-good_state_|-good_state_|-state": { + "comment": "States ran successfully. Updating myminion.", + "name": "good_state", + "start_time": "21:35:16.868345", + "result": true, + "duration": 267.299, + "__run_num__": 0, + "__jid__": "20171130213516897392", + "__sls__": "test", + "changes": { + "ret": { + "myminion": { + "test_|-test succeed with changes_|-test succeed with changes_|-succeed_with_changes": { + "comment": "Success!", + "name": "test succeed with changes", + "start_time": "21:35:17.022592", + "result": true, + "duration": 0.362, + "__run_num__": 0, + "__sls__": "succeed_with_changes", + "changes": { + "testing": { + "new": "Something pretended to change", + "old": "Unchanged" + } + }, + "__id__": "test succeed with changes" + } + } + }, + "out": "highstate" + }, + "__id__": "good_state" + }, + "salt_|-bad_state_|-bad_state_|-state": { + "comment": "Run failed on minions: test", + "name": "bad_state", + "start_time": "21:35:17.136511", + "result": false, + "duration": 197.635, + "__run_num__": 1, + "__jid__": "20171130213517202203", + "__sls__": "test", + "changes": { + "ret": { + "myminion": { + "test_|-test fail with changes_|-test fail with changes_|-fail_with_changes": { + "comment": "Failure!", + "name": "test fail with changes", + "start_time": "21:35:17.326268", + "result": false, + "duration": 0.509, + "__run_num__": 0, + "__sls__": "fail_with_changes", + "changes": { + "testing": { + "new": "Something pretended to change", + "old": "Unchanged" + } + }, + "__id__": "test fail with changes" + } + } + }, + "out": "highstate" + }, + "__id__": "bad_state" + }, + "salt_|-mymod.myfunc_|-mymod.myfunc_|-function": { + "comment": "Function ran successfully. Function mymod.myfunc ran on myminion.", + "name": "mymod.myfunc", + "start_time": "21:35:17.334373", + "result": true, + "duration": 151.716, + "__run_num__": 2, + "__jid__": "20171130213517361706", + "__sls__": "test", + "changes": { + "ret": { + "myminion": true + }, + "out": "highstate" + }, + "__id__": "mymod.myfunc" + }, + "salt_|-mymod.myfunc_false_result-mymod.myfunc_false_result-function": { + "comment": "Running function mymod.myfunc_false_result failed on minions: myminion. Function mymod.myfunc_false_result ran on myminion.", + "name": "mymod.myfunc_false_result", + "start_time": "21:35:17.486625", + "result": false, + "duration": 174.241, + "__run_num__": 3, + "__jid__": "20171130213517536270", + "__sls__": "test", + "changes": { + "ret": { + "myminion": false + }, + "out": "highstate" + }, + "__id__": "mymod.myfunc_false_result" + } + } + }, + "retcode": 1 + } + + +The Oxygen release includes a couple fixes to make parsing this data easier and +more accurate. The first is the ability to set a return code in a custom runner +or wheel function, as noted above. The second is a change to how failures are +included in the return data. Prior to the Oxygen release, minions that failed a +``salt.state`` orchestration job would show up in the ``comment`` field of the +return data, in a human-readable string that was not easily parsed. They are +now included in the ``changes`` dictionary alongside the minions that +succeeded. In addition, ``salt.function`` jobs which failed because the +:ref:`fail function ` returned ``False`` +used to handle their failures in the same way ``salt.state`` jobs did, and this +has likewise been corrected. diff --git a/doc/topics/releases/oxygen.rst b/doc/topics/releases/oxygen.rst index ea569ae8e5..003e56eb3d 100644 --- a/doc/topics/releases/oxygen.rst +++ b/doc/topics/releases/oxygen.rst @@ -84,6 +84,18 @@ If set to ``True``, this option will prevent a minion from allowing the ``saltenv`` argument to override the value set in :conf_minion:`saltenv` when running states. +Failed Minions for State/Function Orchestration Jobs Added to Changes Dictionary +-------------------------------------------------------------------------------- + +For orchestration jobs which run states (or run remote execution functions and +also use a :ref:`fail function ` to indicate +success or failure), minions which have ``False`` results were previously +included as a formatted string in the comment field of the return for that +orchestration job. This made the failed returns difficult to :ref:`parse +programatically `. The +failed returns in these cases are now included in the changes dictionary, +making for much easier parsing. + New Grains ---------- From ecd0a6a0164ba1f55d8e9c778a46872cfbe43927 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Thu, 30 Nov 2017 23:52:11 -0600 Subject: [PATCH 34/34] Add link to newly-merged docs section --- doc/topics/orchestrate/orchestrate_runner.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/doc/topics/orchestrate/orchestrate_runner.rst b/doc/topics/orchestrate/orchestrate_runner.rst index 9a651db571..cbf2ced7d7 100644 --- a/doc/topics/orchestrate/orchestrate_runner.rst +++ b/doc/topics/orchestrate/orchestrate_runner.rst @@ -258,6 +258,7 @@ To execute with pillar data. salt-run state.orch orch.deploy pillar='{"servers": "newsystem1", "master": "mymaster"}' +.. _orchestrate-runner-return-codes-runner-wheel: Return Codes in Runner/Wheel Jobs ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -591,9 +592,10 @@ loadable and parsable format: The Oxygen release includes a couple fixes to make parsing this data easier and -more accurate. The first is the ability to set a return code in a custom runner -or wheel function, as noted above. The second is a change to how failures are -included in the return data. Prior to the Oxygen release, minions that failed a +more accurate. The first is the ability to set a :ref:`return code +` in a custom runner or wheel +function, as noted above. The second is a change to how failures are included +in the return data. Prior to the Oxygen release, minions that failed a ``salt.state`` orchestration job would show up in the ``comment`` field of the return data, in a human-readable string that was not easily parsed. They are now included in the ``changes`` dictionary alongside the minions that