From af386a2d97722b266babaaf2df3e5ec7d27e676d Mon Sep 17 00:00:00 2001 From: Proskurin Kirill Date: Wed, 21 Nov 2018 15:23:15 +0000 Subject: [PATCH 01/22] Fixed the supervisord.dead state idempotency Fixes #50601 --- salt/states/supervisord.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/states/supervisord.py b/salt/states/supervisord.py index ad16de44059..9c16935f48b 100644 --- a/salt/states/supervisord.py +++ b/salt/states/supervisord.py @@ -338,6 +338,7 @@ def dead(name, else: # process name doesn't exist ret['comment'] = "Service {0} doesn't exist".format(name) + return ret if is_stopped is True: ret['comment'] = "Service {0} is not running".format(name) From 2d1f51ce84e8ca55bbbbc61bfebe8c138afd49a6 Mon Sep 17 00:00:00 2001 From: Damon Atkins Date: Thu, 22 Nov 2018 20:15:08 +1100 Subject: [PATCH 02/22] Fix lint only changes, full lint on merge forwards - lint only changes previous diff picked up out of data files, when the branch was out of date. - full limit on merge forward to pick up changes in the lint checks between versions. - update CODEOWNERS for .ci/* --- .ci/lint | 109 ++++++++++++++++++++++++++++++++++++--------- .github/CODEOWNERS | 4 ++ 2 files changed, 92 insertions(+), 21 deletions(-) diff --git a/.ci/lint b/.ci/lint index 8adbbece69d..25efc251d02 100644 --- a/.ci/lint +++ b/.ci/lint @@ -3,7 +3,7 @@ pipeline { options { timestamps() ansiColor('xterm') - timeout(time: 1, unit: 'HOURS') + timeout(time: 3, unit: 'HOURS') } environment { PYENV_ROOT = "/usr/local/pyenv" @@ -14,7 +14,7 @@ pipeline { stage('github-pending') { steps { githubNotify credentialsId: 'test-jenkins-credentials', - description: 'Testing lint...', + description: 'Python lint on changes...', status: 'PENDING', context: "jenkins/pr/lint" } @@ -24,12 +24,14 @@ pipeline { sh ''' # Need -M to detect renames otherwise they are reported as Delete and Add, need -C to detect copies, -C includes -M # -M is on by default in git 2.9+ - git diff --name-status -l99999 -C "origin/$CHANGE_TARGET" "origin/$BRANCH_NAME" > file-list-status.log + git diff --name-status -l99999 -C "origin/$CHANGE_TARGET" > file-list-status.log # the -l increase the search limit, lets use awk so we do not need to repeat the search above. gawk 'BEGIN {FS="\\t"} {if ($1 != "D") {print $NF}}' file-list-status.log > file-list-changed.log gawk 'BEGIN {FS="\\t"} {if ($1 == "D") {print $NF}}' file-list-status.log > file-list-deleted.log - (git diff --name-status -l99999 -C "origin/$CHANGE_TARGET";echo "---";git diff --name-status -l99999 -C "origin/$BRANCH_NAME";printenv|grep -E '=[0-9a-z]{40,}+$|COMMIT=|BRANCH') > file-list-experiment.log + (git diff --name-status -l99999 -C "origin/$CHANGE_TARGET" "origin/$BRANCH_NAME";echo "---";git diff --name-status -l99999 -C "origin/$BRANCH_NAME";printenv|grep -E '=[0-9a-z]{40,}+$|COMMIT=|BRANCH') > file-list-experiment.log touch pylint-report-salt.log pylint-report-tests.log + echo 254 > pylint-salt-chg.exit # assume failure + echo 254 > pylint-tests-chg.exit # assume failure eval "$(pyenv init -)" pyenv --version pyenv install --skip-existing 2.7.14 @@ -41,52 +43,117 @@ pipeline { archiveArtifacts artifacts: 'file-list-status.log,file-list-changed.log,file-list-deleted.log,file-list-experiment.log' } } - stage('linting') { - failFast false + stage('linting chg') { parallel { - stage('salt linting') { + stage('lint salt') { when { expression { return readFile('file-list-changed.log') =~ /(?i)(^|\n)(salt\/.*\.py|setup\.py)\n/ } } steps { sh ''' eval "$(pyenv init - --no-rehash)" - grep -Ei '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-salt | tee pylint-report-salt.log + # tee makes the exit/return code always 0 + grep -Ei '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | (xargs -r '--delimiter=\\n' tox -e pylint-salt;echo "$?" > pylint-salt-chg.exit) | tee pylint-report-salt.log # remove color escape coding sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-salt.log + read rc_exit < pylint-salt-chg.exit + exit "$rc_exit" ''' archiveArtifacts artifacts: 'pylint-report-salt.log' } } - stage('test linting') { + stage('lint test') { when { expression { return readFile('file-list-changed.log') =~ /(?i)(^|\n)tests\/.*\.py\n/ } } steps { sh ''' eval "$(pyenv init - --no-rehash)" - grep -Ei '^tests/.*\\.py$' file-list-changed.log | xargs -r '--delimiter=\\n' tox -e pylint-tests | tee pylint-report-tests.log + # tee makes the exit/return code always 0 + grep -Ei '^tests/.*\\.py$' file-list-changed.log | (xargs -r '--delimiter=\\n' tox -e pylint-tests;echo "$?" > pylint-tests-chg.exit) | tee pylint-report-tests.log # remove color escape coding sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-tests.log + read rc_exit < pylint-tests-chg.exit + exit "$rc_exit" ''' archiveArtifacts artifacts: 'pylint-report-tests.log' } } } + post { + always { + step([$class: 'WarningsPublisher', + parserConfigurations: [[ + parserName: 'PyLint', + pattern: 'pylint-report*chg.log' + ]], + failedTotalAll: '0', + useDeltaValues: false, + canRunOnFailed: true, + usePreviousBuildAsReference: true + ]) + } + } + } + stage('lint all') { + // perform a full linit if this is a merge forward and the change only lint passed. + when { + expression { return params.BRANCH_NAME =~ /(?i)^merge-/ && readFile('file-list-changed.log') =~ /^0/ } + } + parallel { + stage('begin') { + steps { + githubNotify credentialsId: 'test-jenkins-credentials', + description: 'Python lint on everything...', + status: 'PENDING', + context: "jenkins/pr/lint" + } + } + stage('lint salt') { + steps { + sh ''' + eval "$(pyenv init - --no-rehash)" + (tox -e pylint-salt ; echo "$?" > pylint-salt-full.exit) | tee pylint-report-salt-full.log + # remove color escape coding + sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-salt-full.log + read rc_exit < pylint-salt-full.exit + exit "$rc_exit" + ''' + archiveArtifacts artifacts: 'pylint-report-salt-full.log' + } + } + stage('lint test') { + steps { + sh ''' + eval "$(pyenv init - --no-rehash)" + (tox -e pylint-tests ; echo "$?" > pylint-tests-full.exit) | tee pylint-report-tests-full.log + # remove color escape coding + sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-tests-full.log + read rc_exit < pylint-tests-full.exit + exit "$rc_exit" + ''' + archiveArtifacts artifacts: 'pylint-report-tests-full.log' + } + } + } + post { + always { + step([$class: 'WarningsPublisher', + parserConfigurations: [[ + parserName: 'PyLint', + pattern: 'pylint-report*full.log' + ]], + failedTotalAll: '0', + useDeltaValues: false, + canRunOnFailed: true, + usePreviousBuildAsReference: true + ]) + } + } } } post { always { - step([$class: 'WarningsPublisher', - parserConfigurations: [[ - parserName: 'PyLint', - pattern: 'pylint-report*.log' - ]], - failedTotalAll: '0', - useDeltaValues: false, - canRunOnFailed: true, - usePreviousBuildAsReference: true - ]) cleanWs() } success { @@ -97,7 +164,7 @@ pipeline { } failure { githubNotify credentialsId: 'test-jenkins-credentials', - description: 'The lint job has failed', + description: 'The lint test has failed', status: 'FAILURE', context: "jenkins/pr/lint" slackSend channel: "#jenkins-prod-pr", diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 0a8f989d833..b8f19f3b21e 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -13,6 +13,7 @@ salt/*/*boto* @saltstack/team-boto # Team Core requirements/* @saltstack/team-core +rfcs/* @saltstack/team-core salt/auth/* @saltstack/team-core salt/cache/* @saltstack/team-core salt/cli/* @saltstack/team-core @@ -73,3 +74,6 @@ salt/modules/reg.py @saltstack/team-windows salt/states/reg.py @saltstack/team-windows tests/*/*win* @saltstack/team-windows tests/*/test_reg.py @saltstack/team-windows + +# Jenkins Integration +.ci/* @saltstack/saltstack-release-engineering @saltstack/team-core @saltstack/team-windows From fb05bd534b7263326d6689ec6080d0e7828e6d30 Mon Sep 17 00:00:00 2001 From: Jamie Bliss Date: Sun, 25 Nov 2018 23:19:55 -0500 Subject: [PATCH 03/22] Eat an exception from an entry point instead of letting it kill the system. --- salt/loader.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/salt/loader.py b/salt/loader.py index b4e7dbb70c0..c3cf963e6a6 100644 --- a/salt/loader.py +++ b/salt/loader.py @@ -160,9 +160,12 @@ def _module_dirs( ext_type_types.extend(opts[ext_type_dirs]) if HAS_PKG_RESOURCES and ext_type_dirs: for entry_point in pkg_resources.iter_entry_points('salt.loader', ext_type_dirs): - loaded_entry_point = entry_point.load() - for path in loaded_entry_point(): - ext_type_types.append(path) + try: + loaded_entry_point = entry_point.load() + for path in loaded_entry_point(): + ext_type_types.append(path) + except Exception: + log.exception("Error running entry point %r", entry_point) cli_module_dirs = [] # The dirs can be any module dir, or a in-tree _{ext_type} dir From 9f547205a01e91aa1461e686f534a97bdddaf26f Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 27 Nov 2018 11:45:01 -0600 Subject: [PATCH 04/22] Add ignore ability to process_read_exception --- salt/utils/files.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/salt/utils/files.py b/salt/utils/files.py index 65b4783c031..ec223c16856 100644 --- a/salt/utils/files.py +++ b/salt/utils/files.py @@ -205,10 +205,22 @@ def rename(src, dst): os.rename(src, dst) -def process_read_exception(exc, path): +def process_read_exception(exc, path, ignore=None): ''' Common code for raising exceptions when reading a file fails + + The ignore argument can be an iterable of integer error codes (or a single + integer error code) that should be ignored. ''' + if ignore is not None: + if isinstance(ignore, six.integer_types): + ignore = (ignore,) + else: + ignore = () + + if exc.errno in ignore: + return + if exc.errno == errno.ENOENT: raise CommandExecutionError('{0} does not exist'.format(path)) elif exc.errno == errno.EACCES: From d1c2038081fd99a1296c9c53e9a63aa0b368d58e Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 27 Nov 2018 11:55:54 -0600 Subject: [PATCH 05/22] Add generic ip address validation function --- salt/utils/validate/net.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/salt/utils/validate/net.py b/salt/utils/validate/net.py index 86c3632e152..f07506dd0a5 100644 --- a/salt/utils/validate/net.py +++ b/salt/utils/validate/net.py @@ -81,6 +81,14 @@ def ipv6_addr(addr): return __ip_addr(addr, socket.AF_INET6) +def ip_addr(addr): + ''' + Returns True if the IPv4 or IPv6 address (and optional subnet) are valid, + otherwise returns False. + ''' + return ipv4_addr(addr) or ipv6_addr(addr) + + def netmask(mask): ''' Returns True if the value passed is a valid netmask, otherwise return False From ec297a094426dd8830ea69aab07548c71302d4fc Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 27 Nov 2018 12:05:50 -0600 Subject: [PATCH 06/22] Performance improvements in hosts module Prevent additional unnecessary reads, etc. --- salt/modules/hosts.py | 69 ++++++++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/salt/modules/hosts.py b/salt/modules/hosts.py index 5627509c771..e4dd1ae479f 100644 --- a/salt/modules/hosts.py +++ b/salt/modules/hosts.py @@ -5,6 +5,7 @@ Manage the information in the hosts file # Import Python libs from __future__ import absolute_import, print_function, unicode_literals +import errno import os # Import salt libs @@ -22,7 +23,12 @@ def __get_hosts_filename(): ''' Return the path to the appropriate hosts file ''' - return __salt__['config.option']('hosts.file') + try: + return __context__['hosts.__get_hosts_filename'] + except KeyError: + __context__['hosts.__get_hosts_filename'] = \ + __salt__['config.option']('hosts.file') + return __context__['hosts.__get_hosts_filename'] def _get_or_create_hostfile(): @@ -43,26 +49,35 @@ def _list_hosts(): ''' Return the hosts found in the hosts file in as an OrderedDict ''' - count = 0 - hfn = __get_hosts_filename() - ret = odict.OrderedDict() - if not os.path.isfile(hfn): + try: + return __context__['hosts._list_hosts'] + except KeyError: + count = 0 + hfn = __get_hosts_filename() + ret = odict.OrderedDict() + try: + with salt.utils.files.fopen(hfn) as ifile: + for line in ifile: + line = salt.utils.stringutils.to_unicode(line).strip() + if not line: + continue + if line.startswith('#'): + ret.setdefault('comment-{0}'.format(count), []).append(line) + count += 1 + continue + if '#' in line: + line = line[:line.index('#')].strip() + comps = line.split() + ip = comps.pop(0) + ret.setdefault(ip, []).extend(comps) + except (IOError, OSError) as exc: + salt.utils.files.process_read_exception(exc, hfn, ignore=errno.ENOENT) + # Don't set __context__ since we weren't able to read from the + # hosts file. + return ret + + __context__['hosts._list_hosts'] = ret return ret - with salt.utils.files.fopen(hfn) as ifile: - for line in ifile: - line = salt.utils.stringutils.to_unicode(line).strip() - if not line: - continue - if line.startswith('#'): - ret.setdefault('comment-{0}'.format(count), []).append(line) - count += 1 - continue - if '#' in line: - line = line[:line.index('#')].strip() - comps = line.split() - ip = comps.pop(0) - ret.setdefault(ip, []).extend(comps) - return ret def list_hosts(): @@ -133,7 +148,10 @@ def has_pair(ip, alias): salt '*' hosts.has_pair ''' hosts = _list_hosts() - return ip in hosts and alias in hosts[ip] + try: + return alias in hosts[ip] + except KeyError: + return False def set_host(ip, alias): @@ -157,6 +175,9 @@ def set_host(ip, alias): if not os.path.isfile(hfn): return False + # Make sure future calls to _list_hosts() will re-read the file + __context__.pop('hosts._list_hosts', None) + line_to_add = salt.utils.stringutils.to_bytes( ip + '\t\t' + alias + os.linesep ) @@ -203,6 +224,8 @@ def rm_host(ip, alias): ''' if not has_pair(ip, alias): return True + # Make sure future calls to _list_hosts() will re-read the file + __context__.pop('hosts._list_hosts', None) hfn = _get_or_create_hostfile() with salt.utils.files.fopen(hfn, 'rb') as fp_: lines = fp_.readlines() @@ -251,6 +274,10 @@ def add_host(ip, alias): return True hosts = _list_hosts() + + # Make sure future calls to _list_hosts() will re-read the file + __context__.pop('hosts._list_hosts', None) + inserted = False for i, h in six.iteritems(hosts): for j in range(len(h)): From 30f1b85c098eece23cec3ace8f329f3718a59c35 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 27 Nov 2018 12:07:01 -0600 Subject: [PATCH 07/22] Remove non-matching IPs from hosts file --- salt/states/host.py | 76 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 59 insertions(+), 17 deletions(-) diff --git a/salt/states/host.py b/salt/states/host.py index 50bcb644d07..6acab55d8e4 100644 --- a/salt/states/host.py +++ b/salt/states/host.py @@ -75,36 +75,78 @@ def present(name, ip): # pylint: disable=C0103 The host to assign an ip to ip - The ip addr(s) to apply to the host + The ip addr(s) to apply to the host. Can be a single IP or a list of IP + addresses. ''' ret = {'name': name, 'changes': {}, - 'result': None, + 'result': None if __opts__['test'] else True, 'comment': ''} if not isinstance(ip, list): ip = [ip] + all_hosts = __salt__['hosts.list_hosts']() comments = [] - for _ip in ip: - if __salt__['hosts.has_pair'](_ip, name): - ret['result'] = True - comments.append('Host {0} ({1}) already present'.format(name, _ip)) + to_add = set() + to_remove = set() + + # First check for IPs not currently in the hosts file + to_add.update([(addr, name) for addr in ip if addr not in all_hosts]) + + # Now sweep through the hosts file and look for entries matching either the + # IP address(es) or hostname. + for addr, aliases in six.iteritems(all_hosts): + if addr not in ip: + if name in aliases: + # Found match for hostname, but the corresponding IP is not in + # our list, so we need to remove it. + to_remove.add((addr, name)) else: - if __opts__['test']: - comments.append('Host {0} ({1}) needs to be added/updated'.format(name, _ip)) + if name in aliases: + # No changes needed for this IP address and hostname + comments.append( + 'Host {0} ({1}) already present'.format(name, addr) + ) else: - if salt.utils.validate.net.ipv4_addr(_ip) or salt.utils.validate.net.ipv6_addr(_ip): - if __salt__['hosts.add_host'](_ip, name): - ret['changes'] = {'host': name} - ret['result'] = True - comments.append('Added host {0} ({1})'.format(name, _ip)) - else: - ret['result'] = False - comments.append('Failed to set host') + # IP address listed in hosts file, but hostname is not present. + # We will need to add it. + if salt.utils.validate.net.ip_addr(addr): + to_add.add((addr, name)) else: ret['result'] = False - comments.append('Invalid IP Address for {0} ({1})'.format(name, _ip)) + comments.append( + 'Invalid IP Address for {0} ({1})'.format(name, addr) + ) + + for addr, name in to_add: + if __opts__['test']: + comments.append( + 'Host {0} ({1}) would be added'.format(name, addr) + ) + else: + if __salt__['hosts.add_host'](addr, name): + comments.append('Added host {0} ({1})'.format(name, addr)) + else: + ret['result'] = False + comments.append('Failed to add host {0} ({1})'.format(name, addr)) + continue + ret['changes'].setdefault('added', {}).setdefault(addr, []).append(name) + + for addr, name in to_remove: + if __opts__['test']: + comments.append( + 'Host {0} ({1}) would be removed'.format(name, addr) + ) + else: + if __salt__['hosts.rm_host'](addr, name): + comments.append('Removed host {0} ({1})'.format(name, addr)) + else: + ret['result'] = False + comments.append('Failed to remove host {0} ({1})'.format(name, addr)) + continue + ret['changes'].setdefault('removed', {}).setdefault(addr, []).append(name) + ret['comment'] = '\n'.join(comments) return ret From 2671a3067137a9c06d1dac49e42ecd84a59f90d6 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 27 Nov 2018 12:08:21 -0600 Subject: [PATCH 08/22] Add unit tests for host.present state --- tests/unit/states/test_host.py | 198 +++++++++++++++++++++++++++++++-- 1 file changed, 190 insertions(+), 8 deletions(-) diff --git a/tests/unit/states/test_host.py b/tests/unit/states/test_host.py index 5781e94f726..a4e6fa285e8 100644 --- a/tests/unit/states/test_host.py +++ b/tests/unit/states/test_host.py @@ -15,7 +15,8 @@ from tests.support.mock import ( NO_MOCK, NO_MOCK_REASON, MagicMock, - patch + call, + patch, ) @@ -25,19 +26,200 @@ class HostTestCase(TestCase, LoaderModuleMockMixin): Validate the host state ''' def setup_loader_modules(self): - return {host: {}} + return { + host: { + '__opts__': { + 'test': False, + }, + }, + } def test_present(self): ''' Test to ensures that the named host is present with the given ip ''' - ret = {'changes': {}, - 'comment': 'Host salt (127.0.0.1) already present', - 'name': 'salt', 'result': True} + add_host = MagicMock(return_value=True) + rm_host = MagicMock(return_value=True) + hostname = 'salt' + ip_str = '127.0.0.1' + ip_list = ['10.1.2.3', '10.4.5.6'] - mock = MagicMock(return_value=True) - with patch.dict(host.__salt__, {'hosts.has_pair': mock}): - self.assertDictEqual(host.present("salt", "127.0.0.1"), ret) + # Case 1: No match for hostname. Single IP address passed to the state. + list_hosts = MagicMock(return_value={ + '127.0.0.1': ['localhost'], + }) + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_str) + assert ret['result'] is True + assert ret['comment'] == 'Added host {0} ({1})'.format(hostname, ip_str), ret['comment'] + assert ret['changes'] == { + 'added': { + ip_str: [hostname], + } + }, ret['changes'] + expected = [call(ip_str, hostname)] + assert add_host.mock_calls == expected, add_host.mock_calls + assert rm_host.mock_calls == [], rm_host.mock_calls + + # Case 2: No match for hostname. Multiple IP addresses passed to the + # state. + list_hosts = MagicMock(return_value={ + '127.0.0.1': ['localhost'], + }) + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_list) + assert ret['result'] is True + assert 'Added host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] + assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] + assert ret['changes'] == { + 'added': { + ip_list[0]: [hostname], + ip_list[1]: [hostname], + } + }, ret['changes'] + expected = sorted([call(x, hostname) for x in ip_list]) + assert sorted(add_host.mock_calls) == expected, add_host.mock_calls + assert rm_host.mock_calls == [], rm_host.mock_calls + + # Case 3: Match for hostname, but no matching IP. Single IP address + # passed to the state. + list_hosts = MagicMock(return_value={ + '127.0.0.1': ['localhost'], + ip_list[0]: [hostname], + }) + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_str) + assert ret['result'] is True + assert 'Added host {0} ({1})'.format(hostname, ip_str) in ret['comment'] + assert 'Removed host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] + assert ret['changes'] == { + 'added': { + ip_str: [hostname], + }, + 'removed': { + ip_list[0]: [hostname], + } + }, ret['changes'] + expected = [call(ip_str, hostname)] + assert add_host.mock_calls == expected, add_host.mock_calls + expected = [call(ip_list[0], hostname)] + assert rm_host.mock_calls == expected, rm_host.mock_calls + + # Case 4: Match for hostname, but no matching IP. Multiple IP addresses + # passed to the state. + cur_ip = '1.2.3.4' + list_hosts = MagicMock(return_value={ + '127.0.0.1': ['localhost'], + cur_ip: [hostname], + }) + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_list) + assert ret['result'] is True + assert 'Added host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] + assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] + assert 'Removed host {0} ({1})'.format(hostname, cur_ip) in ret['comment'] + assert ret['changes'] == { + 'added': { + ip_list[0]: [hostname], + ip_list[1]: [hostname], + }, + 'removed': { + cur_ip: [hostname], + } + }, ret['changes'] + expected = sorted([call(x, hostname) for x in ip_list]) + assert sorted(add_host.mock_calls) == expected, add_host.mock_calls + expected = [call(cur_ip, hostname)] + assert rm_host.mock_calls == expected, rm_host.mock_calls + + # Case 5: Multiple IP addresses passed to the state. One of them + # matches, the other does not. There is also a non-matching IP that + # must be removed. + cur_ip = '1.2.3.4' + list_hosts = MagicMock(return_value={ + '127.0.0.1': ['localhost'], + cur_ip: [hostname], + ip_list[0]: [hostname], + }) + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_list) + assert ret['result'] is True + assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] + assert 'Removed host {0} ({1})'.format(hostname, cur_ip) in ret['comment'] + assert ret['changes'] == { + 'added': { + ip_list[1]: [hostname], + }, + 'removed': { + cur_ip: [hostname], + } + }, ret['changes'] + expected = [call(ip_list[1], hostname)] + assert add_host.mock_calls == expected, add_host.mock_calls + expected = [call(cur_ip, hostname)] + assert rm_host.mock_calls == expected, rm_host.mock_calls + + # Case 6: Single IP address passed to the state, which matches the + # current configuration for that hostname. No changes should be made. + list_hosts = MagicMock(return_value={ + ip_str: [hostname], + }) + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_str) + assert ret['result'] is True + assert ret['comment'] == 'Host {0} ({1}) already present'.format(hostname, ip_str) in ret['comment'] + assert ret['changes'] == {}, ret['changes'] + assert add_host.mock_calls == [], add_host.mock_calls + assert rm_host.mock_calls == [], rm_host.mock_calls + + # Case 7: Multiple IP addresses passed to the state, which both match + # the current configuration for that hostname. No changes should be + # made. + list_hosts = MagicMock(return_value={ + ip_list[0]: [hostname], + ip_list[1]: [hostname], + }) + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_list) + assert ret['result'] is True + assert 'Host {0} ({1}) already present'.format(hostname, ip_list[0]) in ret['comment'] + assert 'Host {0} ({1}) already present'.format(hostname, ip_list[1]) in ret['comment'] + assert ret['changes'] == {}, ret['changes'] + assert add_host.mock_calls == [], add_host.mock_calls + assert rm_host.mock_calls == [], rm_host.mock_calls def test_absent(self): ''' From 7fd3bcea47dafbf10ff71091e881000618becfd7 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 27 Nov 2018 14:58:59 -0600 Subject: [PATCH 09/22] Add remove parameter to host.present state --- salt/states/host.py | 18 ++++++++-- tests/unit/states/test_host.py | 60 ++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/salt/states/host.py b/salt/states/host.py index 6acab55d8e4..fbedefdd656 100644 --- a/salt/states/host.py +++ b/salt/states/host.py @@ -67,7 +67,7 @@ from salt.ext import six import salt.utils.validate.net -def present(name, ip): # pylint: disable=C0103 +def present(name, ip, remove=False): # pylint: disable=C0103 ''' Ensures that the named host is present with the given ip @@ -77,6 +77,12 @@ def present(name, ip): # pylint: disable=C0103 ip The ip addr(s) to apply to the host. Can be a single IP or a list of IP addresses. + + remove : False + Remove any entries which don't match those configured in the ``ip`` + option. + + .. versionadded:: 2018.3.4 ''' ret = {'name': name, 'changes': {}, @@ -101,7 +107,15 @@ def present(name, ip): # pylint: disable=C0103 if name in aliases: # Found match for hostname, but the corresponding IP is not in # our list, so we need to remove it. - to_remove.add((addr, name)) + if remove: + to_remove.add((addr, name)) + else: + ret.setdefault('warnings', []).append( + 'Host {0} present for IP address {1}. To get rid of ' + 'this warning, either run this state with \'remove\' ' + 'set to True to remove {0} from {1}, or add {1} to ' + 'the \'ip\' argument.'.format(name, addr) + ) else: if name in aliases: # No changes needed for this IP address and hostname diff --git a/tests/unit/states/test_host.py b/tests/unit/states/test_host.py index a4e6fa285e8..7da308b118d 100644 --- a/tests/unit/states/test_host.py +++ b/tests/unit/states/test_host.py @@ -104,6 +104,26 @@ class HostTestCase(TestCase, LoaderModuleMockMixin): ret = host.present(hostname, ip_str) assert ret['result'] is True assert 'Added host {0} ({1})'.format(hostname, ip_str) in ret['comment'] + assert 'Host {0} present for IP address {1}'.format(hostname, ip_list[0]) in ret['warnings'][0] + assert ret['changes'] == { + 'added': { + ip_str: [hostname], + }, + }, ret['changes'] + expected = [call(ip_str, hostname)] + assert add_host.mock_calls == expected, add_host.mock_calls + assert rm_host.mock_calls == [], rm_host.mock_calls + + # Case 3a: Repeat the above with remove=True + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_str, remove=True) + assert ret['result'] is True + assert 'Added host {0} ({1})'.format(hostname, ip_str) in ret['comment'] assert 'Removed host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] assert ret['changes'] == { 'added': { @@ -135,6 +155,27 @@ class HostTestCase(TestCase, LoaderModuleMockMixin): assert ret['result'] is True assert 'Added host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] + assert ret['changes'] == { + 'added': { + ip_list[0]: [hostname], + ip_list[1]: [hostname], + }, + }, ret['changes'] + expected = sorted([call(x, hostname) for x in ip_list]) + assert sorted(add_host.mock_calls) == expected, add_host.mock_calls + assert rm_host.mock_calls == [], rm_host.mock_calls + + # Case 4a: Repeat the above with remove=True + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_list, remove=True) + assert ret['result'] is True + assert 'Added host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] + assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] assert 'Removed host {0} ({1})'.format(hostname, cur_ip) in ret['comment'] assert ret['changes'] == { 'added': { @@ -168,6 +209,25 @@ class HostTestCase(TestCase, LoaderModuleMockMixin): ret = host.present(hostname, ip_list) assert ret['result'] is True assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] + assert ret['changes'] == { + 'added': { + ip_list[1]: [hostname], + }, + }, ret['changes'] + expected = [call(ip_list[1], hostname)] + assert add_host.mock_calls == expected, add_host.mock_calls + assert rm_host.mock_calls == [], rm_host.mock_calls + + # Case 5a: Repeat the above with remove=True + add_host.reset_mock() + rm_host.reset_mock() + with patch.dict(host.__salt__, + {'hosts.list_hosts': list_hosts, + 'hosts.add_host': add_host, + 'hosts.rm_host': rm_host}): + ret = host.present(hostname, ip_list, remove=True) + assert ret['result'] is True + assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] assert 'Removed host {0} ({1})'.format(hostname, cur_ip) in ret['comment'] assert ret['changes'] == { 'added': { From 0ec8bcfd2f1bd28b703a3e962159e63a8010ec8b Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Tue, 27 Nov 2018 13:56:13 -0800 Subject: [PATCH 10/22] When using the gem installed state, when passing a version that includes greater than or less than symbols, ensure that the installed versions meets that requirement. --- salt/states/gem.py | 30 ++++++++++++++++++++++++++---- tests/unit/states/test_gem.py | 13 +++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/salt/states/gem.py b/salt/states/gem.py index 63bf359ef61..3134e8534b3 100644 --- a/salt/states/gem.py +++ b/salt/states/gem.py @@ -16,6 +16,9 @@ you can specify what ruby version and gemset to target. ''' from __future__ import absolute_import +import salt.utils + +import re import logging log = logging.getLogger(__name__) @@ -84,10 +87,29 @@ def installed(name, # pylint: disable=C0103 'Use of argument ruby found, but neither rvm or rbenv is installed' ) gems = __salt__['gem.list'](name, ruby, gem_bin=gem_bin, runas=user) - if name in gems and version is not None and str(version) in gems[name]: - ret['result'] = True - ret['comment'] = 'Gem is already installed.' - return ret + if name in gems and version is not None: + match = re.match(r'(>=|>|<|<=)', version) + if match: + # Grab the comparison + cmpr = match.group() + + # Clear out 'default:' and any whitespace + installed_version = re.sub('default: ', '', gems[name][0]).strip() + + # Clear out comparison from version and whitespace + desired_version = re.sub(cmpr, '', version).strip() + + if salt.utils.compare_versions(installed_version, + cmpr, + desired_version): + ret['result'] = True + ret['comment'] = 'Installed Gem meets version requirements.' + return ret + else: + if str(version) in gems[name]: + ret['result'] = True + ret['comment'] = 'Gem is already installed.' + return ret elif name in gems and version is None: ret['result'] = True ret['comment'] = 'Gem is already installed.' diff --git a/tests/unit/states/test_gem.py b/tests/unit/states/test_gem.py index 3ffd40bec54..a347e234751 100644 --- a/tests/unit/states/test_gem.py +++ b/tests/unit/states/test_gem.py @@ -47,6 +47,19 @@ class TestGemState(TestCase, LoaderModuleMockMixin): ri=False, gem_bin=None ) + def test_installed_version(self): + gems = {'foo': ['1.0'], 'bar': ['2.0']} + gem_list = MagicMock(return_value=gems) + gem_install_succeeds = MagicMock(return_value=True) + + with patch.dict(gem.__salt__, {'gem.list': gem_list}): + with patch.dict(gem.__salt__, + {'gem.install': gem_install_succeeds}): + ret = gem.installed('foo', version='>= 1.0') + self.assertEqual(True, ret['result']) + self.assertEqual('Installed Gem meets version requirements.', + ret['comment']) + def test_removed(self): gems = ['foo', 'bar'] gem_list = MagicMock(return_value=gems) From e4946f9cecc480127da4cf9b226b0395298ec090 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Wed, 28 Nov 2018 11:41:48 -0600 Subject: [PATCH 11/22] Rename "remove" argument to "clean" --- salt/states/host.py | 8 ++++---- tests/unit/states/test_host.py | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/salt/states/host.py b/salt/states/host.py index fbedefdd656..8005ca71480 100644 --- a/salt/states/host.py +++ b/salt/states/host.py @@ -67,7 +67,7 @@ from salt.ext import six import salt.utils.validate.net -def present(name, ip, remove=False): # pylint: disable=C0103 +def present(name, ip, clean=False): # pylint: disable=C0103 ''' Ensures that the named host is present with the given ip @@ -78,7 +78,7 @@ def present(name, ip, remove=False): # pylint: disable=C0103 The ip addr(s) to apply to the host. Can be a single IP or a list of IP addresses. - remove : False + clean : False Remove any entries which don't match those configured in the ``ip`` option. @@ -107,12 +107,12 @@ def present(name, ip, remove=False): # pylint: disable=C0103 if name in aliases: # Found match for hostname, but the corresponding IP is not in # our list, so we need to remove it. - if remove: + if clean: to_remove.add((addr, name)) else: ret.setdefault('warnings', []).append( 'Host {0} present for IP address {1}. To get rid of ' - 'this warning, either run this state with \'remove\' ' + 'this warning, either run this state with \'clean\' ' 'set to True to remove {0} from {1}, or add {1} to ' 'the \'ip\' argument.'.format(name, addr) ) diff --git a/tests/unit/states/test_host.py b/tests/unit/states/test_host.py index 7da308b118d..954b2749066 100644 --- a/tests/unit/states/test_host.py +++ b/tests/unit/states/test_host.py @@ -114,14 +114,14 @@ class HostTestCase(TestCase, LoaderModuleMockMixin): assert add_host.mock_calls == expected, add_host.mock_calls assert rm_host.mock_calls == [], rm_host.mock_calls - # Case 3a: Repeat the above with remove=True + # Case 3a: Repeat the above with clean=True add_host.reset_mock() rm_host.reset_mock() with patch.dict(host.__salt__, {'hosts.list_hosts': list_hosts, 'hosts.add_host': add_host, 'hosts.rm_host': rm_host}): - ret = host.present(hostname, ip_str, remove=True) + ret = host.present(hostname, ip_str, clean=True) assert ret['result'] is True assert 'Added host {0} ({1})'.format(hostname, ip_str) in ret['comment'] assert 'Removed host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] @@ -165,14 +165,14 @@ class HostTestCase(TestCase, LoaderModuleMockMixin): assert sorted(add_host.mock_calls) == expected, add_host.mock_calls assert rm_host.mock_calls == [], rm_host.mock_calls - # Case 4a: Repeat the above with remove=True + # Case 4a: Repeat the above with clean=True add_host.reset_mock() rm_host.reset_mock() with patch.dict(host.__salt__, {'hosts.list_hosts': list_hosts, 'hosts.add_host': add_host, 'hosts.rm_host': rm_host}): - ret = host.present(hostname, ip_list, remove=True) + ret = host.present(hostname, ip_list, clean=True) assert ret['result'] is True assert 'Added host {0} ({1})'.format(hostname, ip_list[0]) in ret['comment'] assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] @@ -218,14 +218,14 @@ class HostTestCase(TestCase, LoaderModuleMockMixin): assert add_host.mock_calls == expected, add_host.mock_calls assert rm_host.mock_calls == [], rm_host.mock_calls - # Case 5a: Repeat the above with remove=True + # Case 5a: Repeat the above with clean=True add_host.reset_mock() rm_host.reset_mock() with patch.dict(host.__salt__, {'hosts.list_hosts': list_hosts, 'hosts.add_host': add_host, 'hosts.rm_host': rm_host}): - ret = host.present(hostname, ip_list, remove=True) + ret = host.present(hostname, ip_list, clean=True) assert ret['result'] is True assert 'Added host {0} ({1})'.format(hostname, ip_list[1]) in ret['comment'] assert 'Removed host {0} ({1})'.format(hostname, cur_ip) in ret['comment'] From 2ddb587e992ceacaa72b31ad004922e9fd62fbcb Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Wed, 28 Nov 2018 11:45:25 -0600 Subject: [PATCH 12/22] Add release notes for new "clean" option --- doc/topics/releases/2018.3.4.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/topics/releases/2018.3.4.rst b/doc/topics/releases/2018.3.4.rst index 97dc3d143d3..b55f9f45ffb 100644 --- a/doc/topics/releases/2018.3.4.rst +++ b/doc/topics/releases/2018.3.4.rst @@ -4,3 +4,11 @@ In Progress: Salt 2018.3.4 Release Notes Version 2018.3.4 is an **unreleased** bugfix release for :ref:`2018.3.0 `. This release is still in progress and has not been released yet. + + +State Changes +============= + +- The :py:func:`host.present ` state can now remove + the specified hostname from IPs not specified in the state. This can be done + by setting the newly-added ``clean`` argument to ``True``. From 6b73c7637f889ae88bdcd4f5bacbafa959010cf2 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Wed, 28 Nov 2018 09:03:35 -0800 Subject: [PATCH 13/22] When using file.replace, with the search_only option, if the pattern does not exist in the file then we should return False. --- salt/modules/file.py | 2 ++ tests/unit/modules/test_file.py | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/salt/modules/file.py b/salt/modules/file.py index 1555276dd4f..46c37678203 100644 --- a/salt/modules/file.py +++ b/salt/modules/file.py @@ -2096,6 +2096,8 @@ def replace(path, # Just search; bail as early as a match is found if re.search(cpattern, r_data): return True # `with` block handles file closure + else: + return False else: result, nrepl = re.subn(cpattern, repl.replace('\\', '\\\\') if backslash_literal else repl, diff --git a/tests/unit/modules/test_file.py b/tests/unit/modules/test_file.py index 1b4c46a8118..161986a584d 100644 --- a/tests/unit/modules/test_file.py +++ b/tests/unit/modules/test_file.py @@ -198,6 +198,22 @@ class FileReplaceTestCase(TestCase, LoaderModuleMockMixin): ''' filemod.replace(self.tfile.name, r'Etiam', 123) + def test_search_only_return_true(self): + ret = filemod.replace(self.tfile.name, + r'Etiam', 'Salticus', + search_only=True) + + self.assertIsInstance(ret, bool) + self.assertEqual(ret, True) + + def test_search_only_return_false(self): + ret = filemod.replace(self.tfile.name, + r'Etian', 'Salticus', + search_only=True) + + self.assertIsInstance(ret, bool) + self.assertEqual(ret, False) + class FileBlockReplaceTestCase(TestCase, LoaderModuleMockMixin): def setup_loader_modules(self): From 7e7df06063541e3c7b92bbb05fa7a6e181b8c2b5 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Wed, 28 Nov 2018 09:37:27 -0800 Subject: [PATCH 14/22] lint --- salt/modules/file.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/salt/modules/file.py b/salt/modules/file.py index 46c37678203..12f000766dd 100644 --- a/salt/modules/file.py +++ b/salt/modules/file.py @@ -5655,6 +5655,7 @@ def list_backups(path, limit=None): [files[x] for x in sorted(files, reverse=True)[:limit]] ))) + list_backup = salt.utils.alias_function(list_backups, 'list_backup') @@ -5827,6 +5828,7 @@ def delete_backup(path, backup_id): return ret + remove_backup = salt.utils.alias_function(delete_backup, 'remove_backup') From 07868f1dfd19b2d3a095c626233a1dfd90527d22 Mon Sep 17 00:00:00 2001 From: Jamie Bliss Date: Wed, 28 Nov 2018 15:33:57 -0500 Subject: [PATCH 15/22] Improve logging output --- salt/loader.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/salt/loader.py b/salt/loader.py index c3cf963e6a6..5cfda4dc0ff 100644 --- a/salt/loader.py +++ b/salt/loader.py @@ -138,6 +138,18 @@ def static_loader( return ret +def _format_entrypoint_target(ep): + """ + Makes a string describing the target of an EntryPoint object. + + Base strongly on EntryPoint.__str__(). + """ + s = ep.module_name + if ep.attrs: + s += ':' + '.'.join(ep.attrs) + return s + + def _module_dirs( opts, ext_type, @@ -164,8 +176,9 @@ def _module_dirs( loaded_entry_point = entry_point.load() for path in loaded_entry_point(): ext_type_types.append(path) - except Exception: - log.exception("Error running entry point %r", entry_point) + except Exception as exc: + log.error("Error getting module directories from %s: %s", _format_entrypoint_target(entry_point), exc) + log.debug("Full backtrace for module directories error", exc_info=True) cli_module_dirs = [] # The dirs can be any module dir, or a in-tree _{ext_type} dir From b6d1605f9941e1707768b3c48500940283dde531 Mon Sep 17 00:00:00 2001 From: Damon Atkins Date: Thu, 29 Nov 2018 23:44:11 +1100 Subject: [PATCH 16/22] ci/lint corrected filenames and test condition on full lint Also fix CODEOWNER team name --- .ci/lint | 37 +++++++++++++++++++------------------ .github/CODEOWNERS | 2 +- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/.ci/lint b/.ci/lint index 25efc251d02..41206071efb 100644 --- a/.ci/lint +++ b/.ci/lint @@ -14,7 +14,7 @@ pipeline { stage('github-pending') { steps { githubNotify credentialsId: 'test-jenkins-credentials', - description: 'Python lint on changes...', + description: 'Python lint on changes begins...', status: 'PENDING', context: "jenkins/pr/lint" } @@ -29,9 +29,10 @@ pipeline { gawk 'BEGIN {FS="\\t"} {if ($1 != "D") {print $NF}}' file-list-status.log > file-list-changed.log gawk 'BEGIN {FS="\\t"} {if ($1 == "D") {print $NF}}' file-list-status.log > file-list-deleted.log (git diff --name-status -l99999 -C "origin/$CHANGE_TARGET" "origin/$BRANCH_NAME";echo "---";git diff --name-status -l99999 -C "origin/$BRANCH_NAME";printenv|grep -E '=[0-9a-z]{40,}+$|COMMIT=|BRANCH') > file-list-experiment.log - touch pylint-report-salt.log pylint-report-tests.log echo 254 > pylint-salt-chg.exit # assume failure + echo 254 > pylint-salt-full.exit # assume failure echo 254 > pylint-tests-chg.exit # assume failure + echo 254 > pylint-tests-full.exit # assume failure eval "$(pyenv init -)" pyenv --version pyenv install --skip-existing 2.7.14 @@ -45,7 +46,7 @@ pipeline { } stage('linting chg') { parallel { - stage('lint salt') { + stage('lint salt chg') { when { expression { return readFile('file-list-changed.log') =~ /(?i)(^|\n)(salt\/.*\.py|setup\.py)\n/ } } @@ -53,16 +54,16 @@ pipeline { sh ''' eval "$(pyenv init - --no-rehash)" # tee makes the exit/return code always 0 - grep -Ei '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | (xargs -r '--delimiter=\\n' tox -e pylint-salt;echo "$?" > pylint-salt-chg.exit) | tee pylint-report-salt.log + grep -Ei '^salt/.*\\.py$|^setup\\.py$' file-list-changed.log | (xargs -r '--delimiter=\\n' tox -e pylint-salt ; echo "$?" > pylint-salt-chg.exit) | tee pylint-report-salt-chg.log # remove color escape coding - sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-salt.log + sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-salt-chg.log read rc_exit < pylint-salt-chg.exit exit "$rc_exit" ''' - archiveArtifacts artifacts: 'pylint-report-salt.log' + archiveArtifacts artifacts: 'pylint-report-salt-chg.log' } } - stage('lint test') { + stage('lint test chg') { when { expression { return readFile('file-list-changed.log') =~ /(?i)(^|\n)tests\/.*\.py\n/ } } @@ -70,13 +71,13 @@ pipeline { sh ''' eval "$(pyenv init - --no-rehash)" # tee makes the exit/return code always 0 - grep -Ei '^tests/.*\\.py$' file-list-changed.log | (xargs -r '--delimiter=\\n' tox -e pylint-tests;echo "$?" > pylint-tests-chg.exit) | tee pylint-report-tests.log + grep -Ei '^tests/.*\\.py$' file-list-changed.log | (xargs -r '--delimiter=\\n' tox -e pylint-tests ; echo "$?" > pylint-tests-chg.exit) | tee pylint-report-tests-chg.log # remove color escape coding - sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-tests.log + sed -ri 's/\\x1B\\[([0-9]{1,2}(;[0-9]{1,2})?)?[m|K]//g' pylint-report-tests-chg.log read rc_exit < pylint-tests-chg.exit exit "$rc_exit" ''' - archiveArtifacts artifacts: 'pylint-report-tests.log' + archiveArtifacts artifacts: 'pylint-report-tests-chg.log' } } } @@ -95,21 +96,21 @@ pipeline { } } } - stage('lint all') { + stage('linting all') { // perform a full linit if this is a merge forward and the change only lint passed. when { - expression { return params.BRANCH_NAME =~ /(?i)^merge-/ && readFile('file-list-changed.log') =~ /^0/ } + expression { return params.BRANCH_NAME =~ /(?i)^merge-/ } } parallel { - stage('begin') { + stage('setup full') { steps { githubNotify credentialsId: 'test-jenkins-credentials', - description: 'Python lint on everything...', + description: 'Python lint on everything begins...', status: 'PENDING', context: "jenkins/pr/lint" } } - stage('lint salt') { + stage('lint salt full') { steps { sh ''' eval "$(pyenv init - --no-rehash)" @@ -122,7 +123,7 @@ pipeline { archiveArtifacts artifacts: 'pylint-report-salt-full.log' } } - stage('lint test') { + stage('lint test full') { steps { sh ''' eval "$(pyenv init - --no-rehash)" @@ -158,13 +159,13 @@ pipeline { } success { githubNotify credentialsId: 'test-jenkins-credentials', - description: 'The lint job has passed', + description: 'Python lint test has passed', status: 'SUCCESS', context: "jenkins/pr/lint" } failure { githubNotify credentialsId: 'test-jenkins-credentials', - description: 'The lint test has failed', + description: 'Python lint test has failed', status: 'FAILURE', context: "jenkins/pr/lint" slackSend channel: "#jenkins-prod-pr", diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b8f19f3b21e..33d58a2f77a 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -76,4 +76,4 @@ tests/*/*win* @saltstack/team-windows tests/*/test_reg.py @saltstack/team-windows # Jenkins Integration -.ci/* @saltstack/saltstack-release-engineering @saltstack/team-core @saltstack/team-windows +.ci/* @saltstack/saltstack-sre-team @saltstack/team-core From 3940a0fa68bd9ad22c2927d1d39fdbea07b65c5f Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Thu, 29 Nov 2018 09:24:28 -0800 Subject: [PATCH 17/22] When adding alises to an existing Certbot certificate, if we see a message about expanding in the stderr returned from cmd.run_all we should rerun the cmd with --expand included. --- salt/modules/acme.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/salt/modules/acme.py b/salt/modules/acme.py index e15ab86ded6..70e7a0529bd 100644 --- a/salt/modules/acme.py +++ b/salt/modules/acme.py @@ -169,7 +169,13 @@ def cert(name, res = __salt__['cmd.run_all'](' '.join(cmd)) if res['retcode'] != 0: - return {'result': False, 'comment': 'Certificate {0} renewal failed with:\n{1}'.format(name, res['stderr'])} + if 'expand' in res['stderr']: + cmd.append('--expand') + res = __salt__['cmd.run_all'](' '.join(cmd)) + if res['retcode'] != 0: + return {'result': False, 'comment': 'Certificate {0} renewal failed with:\n{1}'.format(name, res['stderr'])} + else: + return {'result': False, 'comment': 'Certificate {0} renewal failed with:\n{1}'.format(name, res['stderr'])} if 'no action taken' in res['stdout']: comment = 'Certificate {0} unchanged'.format(cert_file) From cec7cb4dc3e04354e1838f89034ea2a48377e9f4 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Thu, 29 Nov 2018 11:50:45 -0800 Subject: [PATCH 18/22] Older versions of python-augeas need the path passed to aug.match to be a string. --- salt/modules/augeas_cfg.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/salt/modules/augeas_cfg.py b/salt/modules/augeas_cfg.py index 580ff9579ac..dae81c8ca07 100644 --- a/salt/modules/augeas_cfg.py +++ b/salt/modules/augeas_cfg.py @@ -43,6 +43,7 @@ except ImportError: # Import salt libs import salt.utils.args import salt.utils.data +import salt.utils.stringutils from salt.exceptions import SaltInvocationError log = logging.getLogger(__name__) @@ -494,7 +495,7 @@ def ls(path, load_path=None): # pylint: disable=C0103 def _match(path): ''' Internal match function ''' try: - matches = aug.match(path) + matches = aug.match(salt.utils.stringutils.to_str(path)) except RuntimeError: return {} From 869774480d7cfb04e654cf82f0526ac0a8b0e862 Mon Sep 17 00:00:00 2001 From: Jamie Bliss Date: Thu, 29 Nov 2018 22:51:29 -0500 Subject: [PATCH 19/22] Use single quotes per style guide --- salt/loader.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/loader.py b/salt/loader.py index 5cfda4dc0ff..3b776119454 100644 --- a/salt/loader.py +++ b/salt/loader.py @@ -139,11 +139,11 @@ def static_loader( def _format_entrypoint_target(ep): - """ + ''' Makes a string describing the target of an EntryPoint object. Base strongly on EntryPoint.__str__(). - """ + ''' s = ep.module_name if ep.attrs: s += ':' + '.'.join(ep.attrs) From 5936066e622d41d2c5d674a66e2b00dec13c3c12 Mon Sep 17 00:00:00 2001 From: Damon Atkins Date: Fri, 30 Nov 2018 23:36:07 +1100 Subject: [PATCH 20/22] ensure archiveArtifacts are always collected. Use CHANGE_BRANCH to detect merge-forward --- .ci/lint | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/.ci/lint b/.ci/lint index 41206071efb..2ee375f1954 100644 --- a/.ci/lint +++ b/.ci/lint @@ -60,7 +60,6 @@ pipeline { read rc_exit < pylint-salt-chg.exit exit "$rc_exit" ''' - archiveArtifacts artifacts: 'pylint-report-salt-chg.log' } } stage('lint test chg') { @@ -77,16 +76,16 @@ pipeline { read rc_exit < pylint-tests-chg.exit exit "$rc_exit" ''' - archiveArtifacts artifacts: 'pylint-report-tests-chg.log' } } } post { always { + archiveArtifacts artifacts: 'pylint-report-*-chg.log', allowEmptyArchive: true step([$class: 'WarningsPublisher', parserConfigurations: [[ parserName: 'PyLint', - pattern: 'pylint-report*chg.log' + pattern: 'pylint-report-*-chg.log' ]], failedTotalAll: '0', useDeltaValues: false, @@ -99,7 +98,7 @@ pipeline { stage('linting all') { // perform a full linit if this is a merge forward and the change only lint passed. when { - expression { return params.BRANCH_NAME =~ /(?i)^merge-/ } + expression { return params.CHANGE_BRANCH =~ /(?i)^merge[._-]/ } } parallel { stage('setup full') { @@ -120,7 +119,6 @@ pipeline { read rc_exit < pylint-salt-full.exit exit "$rc_exit" ''' - archiveArtifacts artifacts: 'pylint-report-salt-full.log' } } stage('lint test full') { @@ -133,16 +131,16 @@ pipeline { read rc_exit < pylint-tests-full.exit exit "$rc_exit" ''' - archiveArtifacts artifacts: 'pylint-report-tests-full.log' } } } post { always { + archiveArtifacts artifacts: 'pylint-report-*-full.log', allowEmptyArchive: true step([$class: 'WarningsPublisher', parserConfigurations: [[ parserName: 'PyLint', - pattern: 'pylint-report*full.log' + pattern: 'pylint-report-*-full.log' ]], failedTotalAll: '0', useDeltaValues: false, From ecd84863cd849677babf4f5036da0c9c93862ab8 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Sun, 2 Dec 2018 15:21:17 -0700 Subject: [PATCH 21/22] Honor run_run timeout for shell tests --- tests/support/case.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/support/case.py b/tests/support/case.py index af5e292852e..0e159eef40c 100644 --- a/tests/support/case.py +++ b/tests/support/case.py @@ -502,7 +502,7 @@ class ShellCase(ShellTestCase, AdaptedConfigurationTestCaseMixin, ScriptPathMixi arg_str, with_retcode=with_retcode, catch_stderr=catch_stderr, - timeout=60) + timeout=timeout + 10) def run_run_plus(self, fun, *arg, **kwargs): ''' From 7019d47ce9a150459df39cea00d295045e5607d1 Mon Sep 17 00:00:00 2001 From: Bruno Binet Date: Tue, 4 Dec 2018 18:19:51 +0100 Subject: [PATCH 22/22] Fix grafana dashboard updating when nothing has changed The new field "uid" (from grafana5) must be ignored: it will always differ as it is generated server side by grafana when creating the dashboard. --- salt/states/grafana4_dashboard.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/states/grafana4_dashboard.py b/salt/states/grafana4_dashboard.py index 5fb3e892182..25997ded376 100644 --- a/salt/states/grafana4_dashboard.py +++ b/salt/states/grafana4_dashboard.py @@ -247,6 +247,7 @@ def absent(name, orgname=None, profile='grafana'): _IGNORED_DASHBOARD_FIELDS = [ 'id', + 'uid', 'originalTitle', 'version', ]