From f2c045520d4c1ada6bca80fa3d15ac24519e7594 Mon Sep 17 00:00:00 2001 From: rallytime Date: Thu, 1 Dec 2016 16:52:03 -0700 Subject: [PATCH 1/8] Write an integration test demonstrating the issue --- tests/integration/states/file.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/integration/states/file.py b/tests/integration/states/file.py index fe2c5dd5d7..3a29170066 100644 --- a/tests/integration/states/file.py +++ b/tests/integration/states/file.py @@ -1233,6 +1233,11 @@ class FileTest(integration.ModuleCase, integration.SaltReturnAssertsMixIn): # write a line to file with salt.utils.fopen(name, 'w+') as fp_: fp_.write('comment_me') + + # Look for changes with test=True: return should be "None" at the first run + ret = self.run_state('file.comment', test=True, name=name, regex='^comment') + self.assertSaltNoneReturn(ret) + # comment once ret = self.run_state('file.comment', name=name, regex='^comment') # result is positive @@ -1248,6 +1253,11 @@ class FileTest(integration.ModuleCase, integration.SaltReturnAssertsMixIn): # line is still commented with salt.utils.fopen(name, 'r') as fp_: self.assertTrue(fp_.read().startswith('#comment')) + + # Test previously commented file returns "True" now and not "None" with test=True + ret = self.run_state('file.comment', test=True, name=name, regex='^comment') + self.assertSaltTrueReturn(ret) + finally: os.remove(name) From 8a685b1820f586966f11c38909844eded94dc147 Mon Sep 17 00:00:00 2001 From: rallytime Date: Thu, 1 Dec 2016 16:52:56 -0700 Subject: [PATCH 2/8] Check to see if a line is already commented before moving on Fixes #37939 --- salt/states/file.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/salt/states/file.py b/salt/states/file.py index 2d9234f732..f262db85a3 100644 --- a/salt/states/file.py +++ b/salt/states/file.py @@ -3340,9 +3340,16 @@ def comment(name, regex, char='#', backup='.bak'): return _error(ret, check_msg) unanchor_regex = regex.lstrip('^').rstrip('$') + comment_regex = char + unanchor_regex + + # Check if the line is already commented + if __salt__['file.search'](name, comment_regex, multiline=True): + commented = True + else: + commented = False # Make sure the pattern appears in the file before continuing - if not __salt__['file.search'](name, regex, multiline=True): + if commented or not __salt__['file.search'](name, regex, multiline=True): if __salt__['file.search'](name, unanchor_regex, multiline=True): ret['comment'] = 'Pattern already commented' ret['result'] = True From 3babbcda94959a9f1e4995e1e72a2366ed3db283 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Fri, 2 Dec 2016 00:31:17 -0600 Subject: [PATCH 3/8] yumpkg.py: don't include non-upgrade versions found by "yum list available" --- salt/modules/yumpkg.py | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/salt/modules/yumpkg.py b/salt/modules/yumpkg.py index be008cd45b..ae51e10438 100644 --- a/salt/modules/yumpkg.py +++ b/salt/modules/yumpkg.py @@ -441,6 +441,8 @@ def latest_version(*names, **kwargs): if refresh: refresh_db(**kwargs) + cur_pkgs = list_pkgs(versions_as_list=True) + # Get available versions for specified package(s) cmd = [_yum(), '--quiet'] cmd.extend(repo_arg) @@ -455,7 +457,6 @@ def latest_version(*names, **kwargs): if out['stderr']: # Check first if this is just a matter of the packages being # up-to-date. - cur_pkgs = list_pkgs() if not all([x in cur_pkgs for x in names]): log.error( 'Problem encountered getting latest version for the ' @@ -472,13 +473,30 @@ def latest_version(*names, **kwargs): reverse=True ) + def _check_cur(pkg): + if pkg.name in cur_pkgs: + for installed_version in cur_pkgs[pkg.name]: + # If any installed version is greater than the one found by + # yum/dnf list available, then it is not an upgrade. + if salt.utils.compare_versions(ver1=installed_version, + oper='>', + ver2=pkg.version, + cmp_func=version_cmp): + return False + # pkg.version is greater than all installed versions + return True + else: + # Package is not installed + return True + for name in names: for pkg in (x for x in updates if x.name == name): if pkg.arch == 'noarch' or pkg.arch == namearch_map[name] \ or salt.utils.pkg.rpm.check_32(pkg.arch): - ret[name] = pkg.version - # no need to check another match, if there was one - break + if _check_cur(pkg): + ret[name] = pkg.version + # no need to check another match, if there was one + break else: ret[name] = '' From 65289503d9ad5c41c1b0775d7c6c29fceb891170 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Fri, 2 Dec 2016 11:51:00 -0600 Subject: [PATCH 4/8] Simplify logic for matching desired pkg arch with actual pkg arch --- salt/modules/yumpkg.py | 48 ++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/salt/modules/yumpkg.py b/salt/modules/yumpkg.py index ae51e10438..a7dcdc0b87 100644 --- a/salt/modules/yumpkg.py +++ b/salt/modules/yumpkg.py @@ -412,28 +412,6 @@ def latest_version(*names, **kwargs): if len(names) == 0: return '' - # Initialize the return dict with empty strings, and populate namearch_map. - # namearch_map will provide a means of distinguishing between multiple - # matches for the same package name, for example a target of 'glibc' on an - # x86_64 arch would return both x86_64 and i686 versions. - # - # Note that the logic in the for loop below would place the osarch into the - # map for noarch packages, but those cases are accounted for when iterating - # through the 'yum list' results later on. If the match for that package is - # a noarch, then the package is assumed to be noarch, and the namearch_map - # is ignored. - ret = {} - namearch_map = {} - for name in names: - ret[name] = '' - try: - arch = name.rsplit('.', 1)[-1] - if arch not in salt.utils.pkg.rpm.ARCHES: - arch = __grains__['osarch'] - except ValueError: - arch = __grains__['osarch'] - namearch_map[name] = arch - repo_arg = _get_repo_options(**kwargs) exclude_arg = _get_excludes_option(**kwargs) @@ -489,9 +467,33 @@ def latest_version(*names, **kwargs): # Package is not installed return True + ret = {} for name in names: + # Derive desired pkg arch (for arch-specific packages) based on the + # package name(s) passed to the function. On a 64-bit OS, "pkgame" + # would be assumed to match the osarch, while "pkgname.i686" would + # have an arch of "i686". This desired arch is then compared against + # the updates derived from _yum_pkginfo() above, so that we can + # distinguish an update for a 32-bit version of a package from its + # 64-bit counterpart. + try: + arch = name.rsplit('.', 1)[-1] + if arch not in salt.utils.pkg.rpm.ARCHES: + arch = __grains__['osarch'] + except ValueError: + arch = __grains__['osarch'] + + # This loop will iterate over the updates derived by _yum_pkginfo() + # above, which have been sorted descendingly by version number, + # ensuring that the latest available version for the named package is + # examined first. The call to _check_cur() will ensure that a package + # seen by yum as "available" will only be detected as an upgrade if it + # has a version higher than all currently-installed versions of the + # package. for pkg in (x for x in updates if x.name == name): - if pkg.arch == 'noarch' or pkg.arch == namearch_map[name] \ + # This if/or statement makes sure that we account for noarch + # packages as well as arch-specific packages. + if pkg.arch == 'noarch' or pkg.arch == arch \ or salt.utils.pkg.rpm.check_32(pkg.arch): if _check_cur(pkg): ret[name] = pkg.version From 1da7aacfbef0203dde2e62d582fa21f47541508f Mon Sep 17 00:00:00 2001 From: rallytime Date: Fri, 2 Dec 2016 11:22:10 -0700 Subject: [PATCH 5/8] Update unit tests to account for additional file.search call Also removed some mocks that were not needed anymore for the calls to ``file.contains_regex_multiline`` because that function is no longer called in the state file. --- tests/unit/states/file_test.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/unit/states/file_test.py b/tests/unit/states/file_test.py index 38fbea6708..00787a4439 100644 --- a/tests/unit/states/file_test.py +++ b/tests/unit/states/file_test.py @@ -989,7 +989,6 @@ class FileTestCase(TestCase): mock_t = MagicMock(return_value=True) mock_f = MagicMock(return_value=False) - mock = MagicMock(side_effect=[False, True, False, False]) with patch.object(os.path, 'isabs', mock_f): comt = ('Specified file {0} is not an absolute path'.format(name)) ret.update({'comment': comt, 'name': name}) @@ -997,8 +996,7 @@ class FileTestCase(TestCase): with patch.object(os.path, 'isabs', mock_t): with patch.dict(filestate.__salt__, - {'file.contains_regex_multiline': mock, - 'file.search': mock}): + {'file.search': MagicMock(side_effect=[True, True, True, False, False])}): comt = ('Pattern already commented') ret.update({'comment': comt, 'result': True}) self.assertDictEqual(filestate.comment(name, regex), ret) @@ -1008,8 +1006,7 @@ class FileTestCase(TestCase): self.assertDictEqual(filestate.comment(name, regex), ret) with patch.dict(filestate.__salt__, - {'file.contains_regex_multiline': mock_t, - 'file.search': mock_t, + {'file.search': MagicMock(side_effect=[False, True, False, True, True]), 'file.comment': mock_t, 'file.comment_line': mock_t}): with patch.dict(filestate.__opts__, {'test': True}): From 4e10f8e018c72fc1db62837bf807c5d8df17ccdd Mon Sep 17 00:00:00 2001 From: rallytime Date: Fri, 2 Dec 2016 13:08:41 -0700 Subject: [PATCH 6/8] Call exec_test for the Syndic daemon in tests.unit.daemons_test.py --- tests/unit/daemons_test.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/daemons_test.py b/tests/unit/daemons_test.py index 12aa627ba9..7965d3c4f7 100644 --- a/tests/unit/daemons_test.py +++ b/tests/unit/daemons_test.py @@ -232,6 +232,8 @@ class DaemonsStarterTestCase(TestCase, integration.SaltClientTestCaseMixIn): child_pipe.send(ret) child_pipe.close() + self._multiproc_exec_test(exec_test) + if __name__ == '__main__': from integration import run_tests run_tests(DaemonsStarterTestCase, needs_daemon=False) From eb372b27d820fb6a9bebed08617fae331d38e1e1 Mon Sep 17 00:00:00 2001 From: rallytime Date: Fri, 2 Dec 2016 16:53:39 -0700 Subject: [PATCH 7/8] Add missing "not" statement: The last syndic test should assertFalse() --- tests/unit/daemons_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/daemons_test.py b/tests/unit/daemons_test.py index 7965d3c4f7..f7dd2ecf1d 100644 --- a/tests/unit/daemons_test.py +++ b/tests/unit/daemons_test.py @@ -227,7 +227,7 @@ class DaemonsStarterTestCase(TestCase, integration.SaltClientTestCaseMixIn): for alg in ['sha224', 'sha256', 'sha384', 'sha512']: _create_syndic().start() ret = ret and _logger.last_type is None \ - and _logger.last_message + and not _logger.last_message child_pipe.send(ret) child_pipe.close() From 978af6d83c149fc2b9871f6b1ed190a45aedc863 Mon Sep 17 00:00:00 2001 From: twangboy Date: Mon, 5 Dec 2016 12:24:16 -0700 Subject: [PATCH 8/8] Remove only .sls files from the cached winrepo-ng --- salt/modules/win_pkg.py | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/salt/modules/win_pkg.py b/salt/modules/win_pkg.py index f5a46e6eb5..db74a1d92f 100644 --- a/salt/modules/win_pkg.py +++ b/salt/modules/win_pkg.py @@ -15,6 +15,7 @@ from __future__ import absolute_import import os import locale import logging +import re from distutils.version import LooseVersion # pylint: disable=import-error,no-name-in-module # Import third party libs @@ -27,7 +28,7 @@ except ImportError: # pylint: enable=import-error # Import salt libs -from salt.exceptions import SaltRenderError +from salt.exceptions import SaltRenderError, CommandExecutionError import salt.utils import salt.syspaths from salt.exceptions import MinionError @@ -361,10 +362,36 @@ def refresh_db(saltenv='base'): winrepo_source_dir = __opts__['winrepo_source_dir'] # Clear minion repo-ng cache - repo_path = '{0}\\files\\{1}\\win\\repo-ng\\salt-winrepo-ng'\ - .format(__opts__['cachedir'], saltenv) - if not __salt__['file.remove'](repo_path): - log.error('pkg.refresh_db: failed to clear existing cache') + repo_path = _get_local_repo_dir(saltenv=saltenv) + + # Do some safety checks on the repo_path before removing its contents + for pathchecks in [ + '[a-z]\\:\\\\$', + '\\\\', + re.escape(os.environ.get('SystemRoot', 'C:\\Windows')), + ]: + if re.match(pathchecks, repo_path, flags=re.IGNORECASE) is not None: + log.error( + 'Local cache dir seems a bad choice "%s"', + repo_path + ) + raise CommandExecutionError( + 'Error local cache dir seems a bad choice', + info=repo_path + ) + # Clear minion repo-ng cache see #35342 discussion + log.info('Removing all *.sls files of "%s" tree', repo_path) + for root, _, files in os.walk(repo_path): + for name in files: + if name.endswith('.sls'): + full_filename = os.path.join(root, name) + try: + os.remove(full_filename) + except (OSError, IOError) as exc: + raise CommandExecutionError( + 'Could not remove \'{0}\': {1}'. + format(full_filename, exc) + ) # Cache repo-ng locally cached_files = __salt__['cp.cache_dir'](