Merge pull request #46746 from rallytime/merge-2018.3

[2018.3] Merge forward from 2017.7 to 2018.3

.kitchen.yml

@@ -31,7 +31,7 @@ provisioner:
   salt_version: latest
   salt_bootstrap_url: https://bootstrap.saltstack.com
   salt_bootstrap_options: -X -p rsync stable <%= version %>
-  log_level: info
+  log_level: debug
   sudo: true
   require_chef: false
   retry_on_exit_code:
@@ -189,7 +189,6 @@ suites:
 verifier:
   name: runtests
   sudo: true
-  verbose: true
   run_destructive: true
   transport: zeromq
   types:

doc/faq.rst

@@ -254,17 +254,19 @@ specifying the pillar variable is the same one used for :py:func:`pillar.get
     <salt.states.file.managed>` state is only supported in Salt 2015.8.4 and
     newer.
 
+.. _faq-restart-salt-minion:
+
 What is the best way to restart a Salt Minion daemon using Salt after upgrade?
 ------------------------------------------------------------------------------
 
 Updating the ``salt-minion`` package requires a restart of the ``salt-minion``
 service. But restarting the service while in the middle of a state run
 interrupts the process of the Minion running states and sending results back to
-the Master. A common way to workaround that is to schedule restarting of the
-Minion service using :ref:`masterless mode <masterless-quickstart>` after all
-other states have been applied. This allows the minion to keep Minion to Master
-connection alive for the Minion to report the final results to the Master, while
-the service is restarting in the background.
+the Master. A common way to workaround that is to schedule restarting the
+Minion service in the background by issuing a ``salt-call`` command calling
+``service.restart`` function. This prevents the Minion being disconnected from
+the Master immediately. Otherwise you would get
+``Minion did not return. [Not connected]`` message as the result of a state run.
 
 Upgrade without automatic restart
 *********************************
@@ -328,7 +330,7 @@ The following example works on UNIX-like operating systems:
     {%- if grains['os'] != 'Windows' %}
     Restart Salt Minion:
       cmd.run:
-        - name: 'salt-call --local service.restart salt-minion'
+        - name: 'salt-call service.restart salt-minion'
         - bg: True
         - onchanges:
           - pkg: Upgrade Salt Minion
@@ -348,9 +350,9 @@ as follows:
     Restart Salt Minion:
       cmd.run:
     {%- if grains['kernel'] == 'Windows' %}
-        - name: 'C:\salt\salt-call.bat --local service.restart salt-minion'
+        - name: 'C:\salt\salt-call.bat service.restart salt-minion'
     {%- else %}
-        - name: 'salt-call --local service.restart salt-minion'
+        - name: 'salt-call service.restart salt-minion'
     {%- endif %}
         - bg: True
         - onchanges:
@@ -358,7 +360,13 @@ as follows:
 
 However, it requires more advanced tricks to upgrade from legacy version of
 Salt (before ``2016.3.0``) on UNIX-like operating systems, where executing
-commands in the background is not supported:
+commands in the background is not supported. You also may need to schedule
+restarting the Minion service using :ref:`masterless mode
+<masterless-quickstart>` after all other states have been applied for Salt
+versions earlier than ``2016.11.0``. This allows the Minion to keep the
+connection to the Master alive for being able to report the final results back
+to the Master, while the service is restarting in the background. This state
+should run last or watch for the ``pkg`` state changes:
 
 .. code-block:: jinja
 
@@ -382,8 +390,8 @@ Restart the Minion from the command line:
 
 .. code-block:: bash
 
-    salt -G kernel:Windows cmd.run_bg 'C:\salt\salt-call.bat --local service.restart salt-minion'
-    salt -C 'not G@kernel:Windows' cmd.run_bg 'salt-call --local service.restart salt-minion'
+    salt -G kernel:Windows cmd.run_bg 'C:\salt\salt-call.bat service.restart salt-minion'
+    salt -C 'not G@kernel:Windows' cmd.run_bg 'salt-call service.restart salt-minion'
 
 Salting the Salt Master
 -----------------------
@@ -409,6 +417,10 @@ for salt itself:
 
 https://github.com/saltstack-formulas/salt-formula
 
+Restarting the ``salt-master`` service using execution module or application of
+state could be done the same way as for the Salt minion described :ref:`above
+<faq-restart-salt-minion>`.
+
 .. _faq-grain-security:
 
 Is Targeting using Grain Data Secure?
@@ -443,4 +455,3 @@ the grain and values that you want to change / set.)
 
 You should also `file an issue <https://github.com/saltstack/salt/issues>`_
 describing the change so it can be fixed in Salt.
-

doc/ref/clouds/all/index.rst

@@ -1,8 +1,8 @@
 .. _all-salt.clouds:
 
-===============================
-Full list of Salt Cloud modules
-===============================
+=============
+cloud modules
+=============
 
 .. currentmodule:: salt.cloud.clouds
 

doc/ref/configuration/master.rst

@@ -4480,6 +4480,25 @@ Recursively merge lists by aggregating them instead of replacing them.
 
     pillar_merge_lists: False
 
+.. conf_master:: pillar_includes_override_sls
+
+``pillar_includes_override_sls``
+********************************
+
+.. versionadded:: 2017.7.6,2018.3.1
+
+Default: ``False``
+
+Prior to version 2017.7.3, keys from :ref:`pillar includes <pillar-include>`
+would be merged on top of the pillar SLS. Since 2017.7.3, the includes are
+merged together and then the pillar SLS is merged on top of that.
+
+Set this option to ``True`` to return to the old behavior.
+
+.. code-block:: yaml
+
+    pillar_includes_override_sls: True
+
 .. _pillar-cache-opts:
 
 Pillar Cache Options

doc/topics/installation/rhel.rst

@@ -49,17 +49,13 @@ the SaltStack Repository.
 Installation from the Community-Maintained Repository
 =====================================================
 
-Beginning with version 0.9.4, Salt has been available in `EPEL`_. For
-RHEL/CentOS 5, `Fedora COPR`_ is a single community repository that provides
-Salt packages due to the removal from EPEL5.
+Beginning with version 0.9.4, Salt has been available in `EPEL`_.
 
 .. note::
-   Packages in these repositories are built by community, and it can
-   take a little while until the latest stable SaltStack release become
-   available.
+   Packages in this repository are built by community, and it can take a little
+   while until the latest stable SaltStack release become available.
 
 .. _`EPEL`: http://fedoraproject.org/wiki/EPEL
-.. _`Fedora COPR`: https://copr.fedorainfracloud.org/coprs/saltstack/salt-el5/
 
 RHEL/CentOS 6 and 7, Scientific Linux, etc.
 -------------------------------------------
@@ -146,26 +142,13 @@ ZeroMQ 4
 ========
 
 We recommend using ZeroMQ 4 where available. SaltStack provides ZeroMQ 4.0.5
-and pyzmq 14.5.0 in the :ref:`SaltStack Repository <installation-rhel-repo>`
-as well as a separate `zeromq4 COPR`_ repository.
-
-.. _`zeromq4 COPR`: http://copr.fedorainfracloud.org/coprs/saltstack/zeromq4/
+and ``pyzmq`` 14.5.0 in the :ref:`SaltStack Repository
+<installation-rhel-repo>`.
 
 If this repository is added *before* Salt is installed, then installing either
 ``salt-master`` or ``salt-minion`` will automatically pull in ZeroMQ 4.0.5, and
 additional steps to upgrade ZeroMQ and pyzmq are unnecessary.
 
-.. warning:: RHEL/CentOS 5 Users
-    Using COPR repos on RHEL/CentOS 5 requires that the ``python-hashlib``
-    package be installed. Not having it present will result in checksum errors
-    because YUM will not be able to process the SHA256 checksums used by COPR.
-
-.. note::
-    For RHEL/CentOS 5 installations, if using the SaltStack repo or Fedora COPR
-    to install Salt (as described :ref:`above <installation-rhel-repo>`),
-    then it is not necessary to enable the `zeromq4 COPR`_, because those
-    repositories already include ZeroMQ 4.
-
 Package Management
 ==================
 

doc/topics/pillar/index.rst

@@ -285,6 +285,8 @@ Since both pillar SLS files contained a ``bind`` key which contained a nested
 dictionary, the pillar dictionary's ``bind`` key contains the combined contents
 of both SLS files' ``bind`` keys.
 
+.. _pillar-include:
+
 Including Other Pillars
 =======================
 

doc/topics/releases/2017.7.6.rst

@@ -0,0 +1,15 @@
+===========================
+Salt 2017.7.6 Release Notes
+===========================
+
+Version 2017.7.6 is a bugfix release for :ref:`2017.7.0 <release-2017-7-0>`.
+
+Option to Return to Previous Pillar Include Behavior
+----------------------------------------------------
+
+Prior to version 2017.7.3, keys from :ref:`pillar includes <pillar-include>`
+would be merged on top of the pillar SLS. Since 2017.7.3, the includes are
+merged together and then the pillar SLS is merged on top of that.
+
+The :conf_master:`pillar_includes_override_sls` option has been added allow
+users to switch back to the pre-2017.7.3 behavior.

pkg/windows/installer/Salt-Minion-Setup.nsi

@@ -1056,7 +1056,7 @@ Function AddToPath
 
     # Make sure the new length isn't over the NSIS_MAX_STRLEN
     IntCmp $2 ${NSIS_MAX_STRLEN} +4 +4 0
-        DetailPrint "AddToPath: new length $2 > ${NSIS_MAX_STRLEN}"
+        DetailPrint "AddToPath Failed: new length $2 > ${NSIS_MAX_STRLEN}"
         MessageBox MB_OK \
             "You may add C:\salt to the %PATH% for convenience when issuing local salt commands from the command line." \
             /SD IDOK

salt/client/ssh/__init__.py

@@ -57,7 +57,7 @@ from salt.ext import six
 from salt.ext.six.moves import input  # pylint: disable=import-error,redefined-builtin
 try:
     import saltwinshell
-    HAS_WINSHELL = False
+    HAS_WINSHELL = True
 except ImportError:
     HAS_WINSHELL = False
 from salt.utils.zeromq import zmq
@@ -560,6 +560,19 @@ class SSH(object):
                         self.targets[host][default] = self.defaults[default]
                 if 'host' not in self.targets[host]:
                     self.targets[host]['host'] = host
+                if self.targets[host].get('winrm') and not HAS_WINSHELL:
+                    returned.add(host)
+                    rets.add(host)
+                    log_msg = 'Please contact sales@saltstack.com for access to the enterprise saltwinshell module.'
+                    log.debug(log_msg)
+                    no_ret = {'fun_args': [],
+                              'jid': None,
+                              'return': log_msg,
+                              'retcode': 1,
+                              'fun': '',
+                              'id': host}
+                    yield {host: no_ret}
+                    continue
                 args = (
                         que,
                         self.opts,

salt/config/__init__.py

@@ -736,6 +736,9 @@ VALID_OPTS = {
     # Recursively merge lists by aggregating them instead of replacing them.
     'pillar_merge_lists': bool,
 
+    # If True, values from included pillar SLS targets will override
+    'pillar_includes_override_sls': bool,
+
     # How to merge multiple top files from multiple salt environments
     # (saltenvs); can be 'merge' or 'same'
     'top_file_merging_strategy': six.string_types,
@@ -1231,6 +1234,9 @@ DEFAULT_MINION_OPTS = {
     'pillarenv': None,
     'pillarenv_from_saltenv': False,
     'pillar_opts': False,
+    'pillar_source_merging_strategy': 'smart',
+    'pillar_merge_lists': False,
+    'pillar_includes_override_sls': False,
     # ``pillar_cache``, ``pillar_cache_ttl`` and ``pillar_cache_backend``
     # are not used on the minion but are unavoidably in the code path
     'pillar_cache': False,
@@ -1607,6 +1613,7 @@ DEFAULT_MASTER_OPTS = {
     'pillar_safe_render_error': True,
     'pillar_source_merging_strategy': 'smart',
     'pillar_merge_lists': False,
+    'pillar_includes_override_sls': False,
     'pillar_cache': False,
     'pillar_cache_ttl': 3600,
     'pillar_cache_backend': 'disk',

salt/grains/core.py

@@ -2396,7 +2396,30 @@ def get_server_id():
 
     if salt.utils.platform.is_proxy():
         return {}
-    return {'server_id': abs(hash(__opts__.get('id', '')) % (2 ** 31))}
+    id_ = __opts__.get('id', '')
+    id_hash = None
+    py_ver = sys.version_info[:2]
+    if py_ver >= (3, 3):
+        # Python 3.3 enabled hash randomization, so we need to shell out to get
+        # a reliable hash.
+        py_bin = 'python{0}.{1}'.format(*py_ver)
+        id_hash = __salt__['cmd.run'](
+            [py_bin, '-c', 'print(hash("{0}"))'.format(id_)],
+            env={'PYTHONHASHSEED': '0'}
+        )
+        try:
+            id_hash = int(id_hash)
+        except (TypeError, ValueError):
+            log.debug(
+                'Failed to hash the ID to get the server_id grain. Result of '
+                'hash command: %s', id_hash
+            )
+            id_hash = None
+    if id_hash is None:
+        # Python < 3.3 or error encountered above
+        id_hash = hash(id_)
+
+    return {'server_id': abs(id_hash % (2 ** 31))}
 
 
 def get_master():

salt/grains/metadata.py

@@ -48,9 +48,11 @@ def _search(prefix="latest/"):
     Recursively look up all grains in the metadata server
     '''
     ret = {}
-    linedata = http.query(os.path.join(HOST, prefix))
+    linedata = http.query(os.path.join(HOST, prefix), headers=True)
     if 'body' not in linedata:
         return ret
+    if linedata['headers'].get('Content-Type', 'text/plain') == 'application/octet-stream':
+        return linedata['body']
     for line in linedata['body'].split('\n'):
         if line.endswith('/'):
             ret[line[:-1]] = _search(prefix=os.path.join(prefix, line))

salt/minion.py

@@ -2521,6 +2521,7 @@ class Minion(MinionBase):
                     self.opts,
                     self.functions,
                     self.returners,
+                    utils=self.utils,
                     cleanup=[master_event(type='alive')])
 
             try:

salt/modules/cmdmod.py

@@ -414,10 +414,16 @@ def _run(cmd,
             )
         try:
             # Getting the environment for the runas user
+            # Use markers to thwart any stdout noise
             # There must be a better way to do this.
+            import uuid
+            marker = '<<<' + str(uuid.uuid4()) + '>>>'
+            marker_b = marker.encode(__salt_system_encoding__)
             py_code = (
                 'import sys, os, itertools; '
-                'sys.stdout.write(\"\\0\".join(itertools.chain(*os.environ.items())))'
+                'sys.stdout.write(\"' + marker + '\"); '
+                'sys.stdout.write(\"\\0\".join(itertools.chain(*os.environ.items()))); '
+                'sys.stdout.write(\"' + marker + '\");'
             )
             if __grains__['os'] in ['MacOS', 'Darwin']:
                 env_cmd = ('sudo', '-i', '-u', runas, '--',
@@ -431,11 +437,34 @@ def _run(cmd,
                 env_cmd = ('su', '-', runas, '-c', sys.executable)
             else:
                 env_cmd = ('su', '-s', shell, '-', runas, '-c', sys.executable)
-            env_bytes = salt.utils.stringutils.to_bytes(subprocess.Popen(
+
+            env_bytes, env_encoded_err = subprocess.Popen(
                 env_cmd,
-                stdin=subprocess.PIPE,
-                stdout=subprocess.PIPE
-            ).communicate(salt.utils.stringutils.to_bytes(py_code))[0])
+                stderr=subprocess.PIPE,
+                stdout=subprocess.PIPE,
+                stdin=subprocess.PIPE
+            ).communicate(salt.utils.stringutils.to_bytes(py_code))[0]
+            marker_count = env_bytes.count(marker_b)
+            if marker_count == 0:
+                # Possibly PAM prevented the login
+                log.error(
+                    'Environment could not be retrieved for user \'%s\': '
+                    'stderr=%r stdout=%r',
+                    runas, env_encoded_err, env_bytes
+                )
+                # Ensure that we get an empty env_runas dict below since we
+                # were not able to get the environment.
+                env_bytes = b''
+            elif marker_count != 2:
+                raise CommandExecutionError(
+                    'Environment could not be retrieved for user \'{0}\'',
+                    info={'stderr': repr(env_encoded_err),
+                          'stdout': repr(env_bytes)}
+                )
+            else:
+                # Strip the marker
+                env_bytes = env_bytes.split(marker_b)[1]
+
             if six.PY2:
                 import itertools
                 env_runas = dict(itertools.izip(*[iter(env_bytes.split(b'\0'))]*2))

salt/modules/debbuild.py

@@ -426,6 +426,9 @@ def build(runas,
     # use default /var/cache/pbuilder/result
     results_dir = '/var/cache/pbuilder/result'
 
+    ## ensure clean
+    __salt__['cmd.run']('rm -fR {0}'.format(results_dir))
+
     # dscs should only contain salt orig and debian tarballs and dsc file
     for dsc in dscs:
         afile = os.path.basename(dsc)
@@ -436,10 +439,10 @@ def build(runas,
             try:
                 __salt__['cmd.run']('chown {0} -R {1}'.format(runas, dbase))
 
-                cmd = 'pbuilder --update --override-config'
+                cmd = 'pbuilder update --override-config'
                 __salt__['cmd.run'](cmd, runas=runas, python_shell=True)
 
-                cmd = 'pbuilder --build {0}'.format(dsc)
+                cmd = 'pbuilder build --debbuildopts "-sa" {0}'.format(dsc)
                 __salt__['cmd.run'](cmd, runas=runas, python_shell=True)
 
                 # ignore local deps generated package file

salt/modules/mysql.py

@@ -1790,8 +1790,10 @@ def grant_exists(grant,
             if not target_tokens:  # Avoid the overhead of re-calc in loop
                 target_tokens = _grant_to_tokens(target)
             grant_tokens = _grant_to_tokens(grant)
+            grant_tokens_database = grant_tokens['database'].replace('"', '').replace('\\', '').replace('`', '')
+            target_tokens_database = target_tokens['database'].replace('"', '').replace('\\', '').replace('`', '')
             if grant_tokens['user'] == target_tokens['user'] and \
-                    grant_tokens['database'] == target_tokens['database'] and \
+                    grant_tokens_database == target_tokens_database and \
                     grant_tokens['host'] == target_tokens['host'] and \
                     set(grant_tokens['grant']) >= set(target_tokens['grant']):
                 return True

salt/modules/npm.py

@@ -159,7 +159,7 @@ def install(pkg=None,
     if runas:
         uid = salt.utils.user.get_uid(runas)
         if uid:
-            env.update({'SUDO_UID': b'{0}'.format(uid), 'SUDO_USER': b''})
+            env.update({'SUDO_UID': uid, 'SUDO_USER': ''})
 
     cmd = ' '.join(cmd)
     result = __salt__['cmd.run_all'](cmd, python_shell=True, cwd=dir, runas=runas, env=env)
@@ -238,7 +238,7 @@ def uninstall(pkg, dir=None, runas=None, env=None):
     if runas:
         uid = salt.utils.user.get_uid(runas)
         if uid:
-            env.update({'SUDO_UID': b'{0}'.format(uid), 'SUDO_USER': b''})
+            env.update({'SUDO_UID': uid, 'SUDO_USER': ''})
 
     cmd = ['npm', 'uninstall', '"{0}"'.format(pkg)]
     if not dir:
@@ -297,7 +297,7 @@ def list_(pkg=None, dir=None, runas=None, env=None, depth=None):
     if runas:
         uid = salt.utils.user.get_uid(runas)
         if uid:
-            env.update({'SUDO_UID': b'{0}'.format(uid), 'SUDO_USER': b''})
+            env.update({'SUDO_UID': uid, 'SUDO_USER': ''})
 
     cmd = ['npm', 'list', '--json', '--silent']
 
@@ -360,7 +360,7 @@ def cache_clean(path=None, runas=None, env=None, force=False):
     if runas:
         uid = salt.utils.user.get_uid(runas)
         if uid:
-            env.update({'SUDO_UID': b'{0}'.format(uid), 'SUDO_USER': b''})
+            env.update({'SUDO_UID': uid, 'SUDO_USER': ''})
 
     cmd = ['npm', 'cache', 'clean']
     if path:
@@ -407,7 +407,7 @@ def cache_list(path=None, runas=None, env=None):
     if runas:
         uid = salt.utils.user.get_uid(runas)
         if uid:
-            env.update({'SUDO_UID': b'{0}'.format(uid), 'SUDO_USER': b''})
+            env.update({'SUDO_UID': uid, 'SUDO_USER': ''})
 
     cmd = ['npm', 'cache', 'ls']
     if path:
@@ -447,7 +447,7 @@ def cache_path(runas=None, env=None):
     if runas:
         uid = salt.utils.user.get_uid(runas)
         if uid:
-            env.update({'SUDO_UID': b'{0}'.format(uid), 'SUDO_USER': b''})
+            env.update({'SUDO_UID': uid, 'SUDO_USER': ''})
 
     cmd = 'npm config get cache'
 

salt/modules/pillar.py

@@ -366,6 +366,28 @@ def item(*args, **kwargs):
 
         .. versionadded:: 2015.8.0
 
+    pillarenv
+        If specified, this function will query the master to generate fresh
+        pillar data on the fly, specifically from the requested pillar
+        environment. Note that this can produce different pillar data than
+        executing this function without an environment, as its normal behavior
+        is just to return a value from minion's pillar data in memory (which
+        can be sourced from more than one pillar environment).
+
+        Using this argument will not affect the pillar data in memory. It will
+        however be slightly slower and use more resources on the master due to
+        the need for the master to generate and send the minion fresh pillar
+        data. This tradeoff in performance however allows for the use case
+        where pillar data is desired only from a single environment.
+
+        .. versionadded:: 2017.7.6,2018.3.1
+
+    saltenv
+        Included only for compatibility with
+        :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored.
+
+        .. versionadded:: 2017.7.6,2018.3.1
+
     CLI Examples:
 
     .. code-block:: bash
@@ -377,11 +399,17 @@ def item(*args, **kwargs):
     ret = {}
     default = kwargs.get('default', '')
     delimiter = kwargs.get('delimiter', DEFAULT_TARGET_DELIM)
+    pillarenv = kwargs.get('pillarenv', None)
+    saltenv = kwargs.get('saltenv', None)
+
+    pillar_dict = __pillar__ \
+        if all(x is None for x in (saltenv, pillarenv)) \
+        else items(saltenv=saltenv, pillarenv=pillarenv)
 
     try:
         for arg in args:
             ret[arg] = salt.utils.data.traverse_dict_and_list(
-                __pillar__,
+                pillar_dict,
                 arg,
                 default,
                 delimiter)

salt/modules/win_dism.py

@@ -430,16 +430,27 @@ def add_package(package,
     Install a package using DISM
 
     Args:
-        package (str): The package to install. Can be a .cab file, a .msu file,
-            or a folder
-        ignore_check (Optional[bool]): Skip installation of the package if the
-            applicability checks fail
-        prevent_pending (Optional[bool]): Skip the installation of the package
-            if there are pending online actions
-        image (Optional[str]): The path to the root directory of an offline
-            Windows image. If `None` is passed, the running operating system is
-            targeted. Default is None.
-        restart (Optional[bool]): Reboot the machine if required by the install
+        package (str):
+            The package to install. Can be a .cab file, a .msu file, or a folder
+
+            .. note::
+                An `.msu` package is supported only when the target image is
+                offline, either mounted or applied.
+
+        ignore_check (Optional[bool]):
+            Skip installation of the package if the applicability checks fail
+
+        prevent_pending (Optional[bool]):
+            Skip the installation of the package if there are pending online
+            actions
+
+        image (Optional[str]):
+            The path to the root directory of an offline Windows image. If
+            ``None`` is passed, the running operating system is targeted.
+            Default is None.
+
+        restart (Optional[bool]):
+            Reboot the machine if required by the install
 
     Returns:
         dict: A dictionary containing the results of the command

salt/modules/win_lgpo.py

@@ -3440,7 +3440,7 @@ def _processValueItem(element, reg_key, reg_valuename, policy, parent_element,
                 this_element_value = b''.join([this_element_value.encode('utf-16-le'),
                                                encoded_null])
         elif etree.QName(element).localname == 'multiText':
-            this_vtype = 'REG_MULTI_SZ'
+            this_vtype = 'REG_MULTI_SZ' if not check_deleted else 'REG_SZ'
             if this_element_value is not None:
                 this_element_value = '{0}{1}{1}'.format(chr(0).join(this_element_value), chr(0))
         elif etree.QName(element).localname == 'list':

salt/modules/yumpkg.py

@@ -2792,20 +2792,18 @@ def mod_repo(repo, basedir=None, **kwargs):
     filerepos[repo].update(repo_opts)
     content = header
     for stanza in six.iterkeys(filerepos):
-        comments = ''
-        if 'comments' in six.iterkeys(filerepos[stanza]):
         comments = salt.utils.pkg.rpm.combine_comments(
-                    filerepos[stanza]['comments'])
-            del filerepos[stanza]['comments']
-        content += '\n[{0}]'.format(stanza)
+            filerepos[stanza].pop('comments', [])
+        )
+        content += '[{0}]\n'.format(stanza)
         for line in six.iterkeys(filerepos[stanza]):
-            content += '\n{0}={1}'.format(
+            content += '{0}={1}\n'.format(
                 line,
                 filerepos[stanza][line]
                     if not isinstance(filerepos[stanza][line], bool)
                     else _bool_to_str(filerepos[stanza][line])
             )
-        content += '\n{0}\n'.format(comments)
+        content += comments + '\n'
 
     with salt.utils.files.fopen(repofile, 'w') as fileout:
         fileout.write(salt.utils.stringutils.to_str(content))
@@ -2834,15 +2832,30 @@ def _parse_repo_file(filename):
         section_dict.pop('__name__', None)
         config[section] = section_dict
 
-    # Try to extract leading comments
+    # Try to extract header comments, as well as comments for each repo. Read
+    # from the beginning of the file and assume any leading comments are
+    # header comments. Continue to read each section header and then find the
+    # comments for each repo.
     headers = ''
-    with salt.utils.files.fopen(filename, 'r') as rawfile:
-        for line in rawfile:
+    section = None
+    with salt.utils.files.fopen(filename, 'r') as repofile:
+        for line in repofile:
             line = salt.utils.stringutils.to_unicode(line)
-            if line.strip().startswith('#'):
-                headers += '{0}\n'.format(line.strip())
+            line = line.strip()
+            if line.startswith('#'):
+                if section is None:
+                    headers += line + '\n'
                 else:
-                break
+                    try:
+                        comments = config[section].setdefault('comments', [])
+                        comments.append(line[1:].lstrip())
+                    except KeyError:
+                        log.debug(
+                            'Found comment in %s which does not appear to '
+                            'belong to any repo section: %s', filename, line
+                        )
+            elif line.startswith('[') and line.endswith(']'):
+                section = line[1:-1]
 
     return (headers, salt.utils.data.decode(config))
 

salt/modules/zypper.py

@@ -726,6 +726,123 @@ def list_pkgs(versions_as_list=False, **kwargs):
         attr)
 
 
+def list_repo_pkgs(*args, **kwargs):
+    '''
+    .. versionadded:: 2017.7.5,2018.3.1
+
+    Returns all available packages. Optionally, package names (and name globs)
+    can be passed and the results will be filtered to packages matching those
+    names. This is recommended as it speeds up the function considerably.
+
+    This function can be helpful in discovering the version or repo to specify
+    in a :mod:`pkg.installed <salt.states.pkg.installed>` state.
+
+    The return data will be a dictionary mapping package names to a list of
+    version numbers, ordered from newest to oldest. If ``byrepo`` is set to
+    ``True``, then the return dictionary will contain repository names at the
+    top level, and each repository will map packages to lists of version
+    numbers. For example:
+
+    .. code-block:: python
+
+        # With byrepo=False (default)
+        {
+            'bash': ['4.3-83.3.1',
+                     '4.3-82.6'],
+            'vim': ['7.4.326-12.1']
+        }
+        {
+            'OSS': {
+                'bash': ['4.3-82.6'],
+                'vim': ['7.4.326-12.1']
+            },
+            'OSS Update': {
+                'bash': ['4.3-83.3.1']
+            }
+        }
+
+    fromrepo : None
+        Only include results from the specified repo(s). Multiple repos can be
+        specified, comma-separated.
+
+    byrepo : False
+        When ``True``, the return data for each package will be organized by
+        repository.
+
+    CLI Examples:
+
+    .. code-block:: bash
+
+        salt '*' pkg.list_repo_pkgs
+        salt '*' pkg.list_repo_pkgs foo bar baz
+        salt '*' pkg.list_repo_pkgs 'python2-*' byrepo=True
+        salt '*' pkg.list_repo_pkgs 'python2-*' fromrepo='OSS Updates'
+    '''
+    byrepo = kwargs.pop('byrepo', False)
+    fromrepo = kwargs.pop('fromrepo', '') or ''
+    ret = {}
+
+    targets = [
+        arg if isinstance(arg, six.string_types) else six.text_type(arg)
+        for arg in args
+    ]
+
+    def _is_match(pkgname):
+        '''
+        When package names are passed to a zypper search, they will be matched
+        anywhere in the package name. This makes sure that only exact or
+        fnmatch matches are identified.
+        '''
+        if not args:
+            # No package names passed, everyone's a winner!
+            return True
+        for target in targets:
+            if fnmatch.fnmatch(pkgname, target):
+                return True
+        return False
+
+    for node in __zypper__.xml.call('se', '-s', *targets).getElementsByTagName('solvable'):
+        pkginfo = dict(node.attributes.items())
+        try:
+            if pkginfo['kind'] != 'package':
+                continue
+            reponame = pkginfo['repository']
+            if fromrepo and reponame != fromrepo:
+                continue
+            pkgname = pkginfo['name']
+            pkgversion = pkginfo['edition']
+        except KeyError:
+            continue
+        else:
+            if _is_match(pkgname):
+                repo_dict = ret.setdefault(reponame, {})
+                version_list = repo_dict.setdefault(pkgname, set())
+                version_list.add(pkgversion)
+
+    if byrepo:
+        for reponame in ret:
+            # Sort versions newest to oldest
+            for pkgname in ret[reponame]:
+                sorted_versions = sorted(
+                    [LooseVersion(x) for x in ret[reponame][pkgname]],
+                    reverse=True
+                )
+                ret[reponame][pkgname] = [x.vstring for x in sorted_versions]
+        return ret
+    else:
+        byrepo_ret = {}
+        for reponame in ret:
+            for pkgname in ret[reponame]:
+                byrepo_ret.setdefault(pkgname, []).extend(ret[reponame][pkgname])
+        for pkgname in byrepo_ret:
+            sorted_versions = sorted(
+                [LooseVersion(x) for x in byrepo_ret[pkgname]],
+                reverse=True
+            )
+            byrepo_ret[pkgname] = [x.vstring for x in sorted_versions]
+        return byrepo_ret
+
+
 def _get_configured_repos():
     '''
     Get all the info about repositories from the configurations.
@@ -1144,6 +1261,15 @@ def install(name=None,
         return {}
 
     version_num = Wildcard(__zypper__)(name, version)
+
+    if version_num:
+        if pkgs is None and sources is None:
+            # Allow "version" to work for single package target
+            pkg_params = {name: version_num}
+        else:
+            log.warning('"version" parameter will be ignored for multiple '
+                        'package targets')
+
     if pkg_type == 'repository':
         targets = []
         for param, version_num in six.iteritems(pkg_params):

salt/pillar/__init__.py

@@ -784,11 +784,22 @@ class Pillar(object):
                                             nstate = {
                                                 key_fragment: nstate
                                             }
+                                    if not self.opts.get('pillar_includes_override_sls', False):
                                         include_states.append(nstate)
+                                    else:
+                                        state = merge(
+                                            state,
+                                            nstate,
+                                            self.merge_strategy,
+                                            self.opts.get('renderer', 'yaml'),
+                                            self.opts.get('pillar_merge_lists', False))
                                 if err:
                                     errors += err
-                        if include_states:
-                            # merge included state(s) with the current state merged last
+
+                        if not self.opts.get('pillar_includes_override_sls', False):
+                            # merge included state(s) with the current state
+                            # merged last to ensure that its values are
+                            # authoritative.
                             include_states.append(state)
                             state = None
                             for s in include_states:

salt/returners/local_cache.py

@@ -441,7 +441,7 @@ def clean_old_jobs():
                     shutil.rmtree(t_path)
                 elif os.path.isfile(jid_file):
                     jid_ctime = os.stat(jid_file).st_ctime
-                    hours_difference = (time.time()- jid_ctime) / 3600.0
+                    hours_difference = (time.time() - jid_ctime) / 3600.0
                     if hours_difference > __opts__['keep_jobs'] and os.path.exists(t_path):
                         # Remove the entire t_path from the original JID dir
                         shutil.rmtree(t_path)

salt/state.py

@@ -2342,6 +2342,7 @@ class State(object):
                     if not r_state.startswith('prerequired'):
                         req_stats.add('pre')
                 else:
+                    if run_dict[tag].get('__state_ran__', True):
                         req_stats.add('met')
             if r_state.endswith('_any'):
                 if 'met' in req_stats or 'change' in req_stats:
@@ -2620,6 +2621,7 @@ class State(object):
                             'duration': duration,
                             'start_time': start_time,
                             'comment': 'State was not run because onfail req did not change',
+                            '__state_ran__': False,
                             '__run_num__': self.__run_num,
                             '__sls__': low['__sls__']}
             self.__run_num += 1
@@ -2630,6 +2632,7 @@ class State(object):
                             'duration': duration,
                             'start_time': start_time,
                             'comment': 'State was not run because none of the onchanges reqs changed',
+                            '__state_ran__': False,
                             '__run_num__': self.__run_num,
                             '__sls__': low['__sls__']}
             self.__run_num += 1

salt/states/file.py

@@ -5333,9 +5333,14 @@ def copy(
         subdir=False,
         **kwargs):
     '''
-    If the source file exists on the system, copy it to the named file. The
-    named file will not be overwritten if it already exists unless the force
-    option is set to True.
+    If the file defined by the ``source`` option exists on the minion, copy it
+    to the named path. The file will not be overwritten if it already exists,
+    unless the ``force`` option is set to ``True``.
+
+    .. note::
+        This state only copies files from one location on a minion to another
+        location on the same minion. For copying files from the master, use a
+        :py:func:`file.managed <salt.states.file.managed>` state.
 
     name
         The location of the file to copy to

salt/states/pkg.py

@@ -1009,11 +1009,11 @@ def installed(
         **WILDCARD VERSIONS**
 
         As of the 2017.7.0 release, this state now supports wildcards in
-        package versions for SUSE SLES/Leap/Tumbleweed, Debian/Ubuntu, RHEL/CentOS,
-        Arch Linux, and their derivatives. Using wildcards can be useful for
-        packages where the release name is built into the version in some way,
-        such as for RHEL/CentOS which typically has version numbers like
-        ``1.2.34-5.el7``. An example of the usage for this would be:
+        package versions for SUSE SLES/Leap/Tumbleweed, Debian/Ubuntu,
+        RHEL/CentOS, Arch Linux, and their derivatives. Using wildcards can be
+        useful for packages where the release name is built into the version in
+        some way, such as for RHEL/CentOS which typically has version numbers
+        like ``1.2.34-5.el7``. An example of the usage for this would be:
 
         .. code-block:: yaml
 
@@ -1021,6 +1021,11 @@ def installed(
               pkg.installed:
                 - version: '1.2.34*'
 
+        Keep in mind that using wildcard versions will result in a slower state
+        run since Salt must gather the available versions of the specified
+        packages and figure out which of them match the specified wildcard
+        expression.
+
     :param bool refresh:
         This parameter controls whether or not the package repo database is
         updated prior to installing the requested package(s).
@@ -2668,7 +2673,7 @@ def removed(name,
             .. code-block:: yaml
 
                 vim-enhanced:
-                  pkg.installed:
+                  pkg.removed:
                     - version: 2:7.4.160-1.el7
 
             In version 2015.8.9, an **ignore_epoch** argument has been added to
@@ -2774,7 +2779,7 @@ def purged(name,
             .. code-block:: yaml
 
                 vim-enhanced:
-                  pkg.installed:
+                  pkg.purged:
                     - version: 2:7.4.160-1.el7
 
             In version 2015.8.9, an **ignore_epoch** argument has been added to

salt/utils/pkg/rpm.py

@@ -12,7 +12,6 @@ import subprocess
 
 # Import 3rd-party libs
 from salt.ext import six
-from salt.ext.six.moves import range  # pylint: disable=redefined-builtin
 
 log = logging.getLogger(__name__)
 
@@ -122,13 +121,13 @@ def combine_comments(comments):
     '''
     if not isinstance(comments, list):
         comments = [comments]
-    for idx in range(len(comments)):
-        if not isinstance(comments[idx], six.string_types):
-            comments[idx] = six.text_type(comments[idx])
-        comments[idx] = comments[idx].strip()
-        if not comments[idx].startswith('#'):
-            comments[idx] = '#' + comments[idx]
-    return '\n'.join(comments)
+    ret = []
+    for comment in comments:
+        if not isinstance(comment, six.string_types):
+            comment = str(comment)
+        # Normalize for any spaces (or lack thereof) after the #
+        ret.append('# {0}\n'.format(comment.lstrip('#').lstrip()))
+    return ''.join(ret)
 
 
 def version_to_evr(verstring):

salt/utils/schedule.py

@@ -75,7 +75,7 @@ class Schedule(object):
     '''
     instance = None
 
-    def __new__(cls, opts, functions, returners=None, intervals=None, cleanup=None, proxy=None, standalone=False):
+    def __new__(cls, opts, functions, returners=None, intervals=None, cleanup=None, proxy=None, utils=None, standalone=False):
         '''
         Only create one instance of Schedule
         '''
@@ -85,20 +85,21 @@ class Schedule(object):
             # it in a WeakValueDictionary-- which will remove the item if no one
             # references it-- this forces a reference while we return to the caller
             cls.instance = object.__new__(cls)
-            cls.instance.__singleton_init__(opts, functions, returners, intervals, cleanup, proxy, standalone)
+            cls.instance.__singleton_init__(opts, functions, returners, intervals, cleanup, proxy, utils, standalone)
         else:
             log.debug('Re-using Schedule')
         return cls.instance
 
     # has to remain empty for singletons, since __init__ will *always* be called
-    def __init__(self, opts, functions, returners=None, intervals=None, cleanup=None, proxy=None, standalone=False):
+    def __init__(self, opts, functions, returners=None, intervals=None, cleanup=None, proxy=None, utils=None, standalone=False):
         pass
 
     # an init for the singleton instance to call
-    def __singleton_init__(self, opts, functions, returners=None, intervals=None, cleanup=None, proxy=None, standalone=False):
+    def __singleton_init__(self, opts, functions, returners=None, intervals=None, cleanup=None, proxy=None, utils=None, standalone=False):
         self.opts = opts
         self.proxy = proxy
         self.functions = functions
+        self.utils = utils
         self.standalone = standalone
         self.skip_function = None
         self.skip_during_range = None
@@ -586,10 +587,11 @@ class Schedule(object):
             # This also needed for ZeroMQ transport to reset all functions
             # context data that could keep paretns connections. ZeroMQ will
             # hang on polling parents connections from the child process.
+            utils = self.utils or salt.loader.utils(self.opts)
             if self.opts['__role'] == 'master':
-                self.functions = salt.loader.runner(self.opts)
+                self.functions = salt.loader.runner(self.opts, utils=utils)
             else:
-                self.functions = salt.loader.minion_mods(self.opts, proxy=self.proxy)
+                self.functions = salt.loader.minion_mods(self.opts, proxy=self.proxy, utils=utils)
             self.returners = salt.loader.returners(self.opts, self.functions, proxy=self.proxy)
         ret = {'id': self.opts.get('id', 'master'),
                'fun': func,
@@ -1411,6 +1413,8 @@ class Schedule(object):
                 self.functions = {}
                 returners = self.returners
                 self.returners = {}
+                utils = self.utils
+                self.utils = {}
             try:
                 # Job is disabled, continue
                 if 'enabled' in data and not data['enabled']:
@@ -1448,6 +1452,7 @@ class Schedule(object):
                 # Restore our function references.
                 self.functions = functions
                 self.returners = returners
+                self.utils = utils
 
 
 def clean_proc_dir(opts):

salt/utils/templates.py

@@ -373,6 +373,13 @@ def render_jinja_tmpl(tmplstr, context, tmplpath=None):
             decoded_context[key] = value
             continue
 
+        try:
+            decoded_context[key] = salt.utils.stringutils.to_unicode(value, encoding=SLS_ENCODING)
+        except UnicodeDecodeError as ex:
+            log.debug(
+                "Failed to decode using default encoding (%s), trying system encoding",
+                SLS_ENCODING,
+            )
             decoded_context[key] = salt.utils.locales.sdecode(value)
 
     try:

tests/integration/files/file/base/mkimage-busybox-static

@@ -82,6 +82,9 @@ cp "$busybox" "$rootfsDir/bin/busybox"
 	unset IFS
 
 	for module in "${modules[@]}"; do
+		# Don't stomp on the busybox binary (newer busybox releases
+		# include busybox in the --list-modules output)
+		test "$module" == "bin/busybox" && continue
 		mkdir -p "$(dirname "$module")"
 		ln -sf /bin/busybox "$module"
 	done

tests/integration/files/file/base/requisites/onfail_multiple_required.sls

@@ -0,0 +1,25 @@
+a:
+  cmd.run:
+    - name: exit 1
+
+b:
+  cmd.run:
+    - name: echo b
+    - onfail:
+      - cmd: a
+
+c:
+  cmd.run:
+    - name: echo c
+    - onfail:
+      - cmd: a
+    - require:
+      - cmd: b
+
+d:
+  cmd.run:
+    - name: echo d
+    - onfail:
+      - cmd: a
+    - require:
+      - cmd: c

tests/integration/files/file/base/requisites/onfail_multiple_required_no_run.sls

@@ -0,0 +1,25 @@
+a:
+  cmd.run:
+    - name: exit 0
+
+b:
+  cmd.run:
+    - name: echo b
+    - onfail:
+      - cmd: a
+
+c:
+  cmd.run:
+    - name: echo c
+    - onfail:
+      - cmd: a
+    - require:
+      - cmd: b
+
+d:
+  cmd.run:
+    - name: echo d
+    - onfail:
+      - cmd: a
+    - require:
+      - cmd: c

tests/integration/modules/test_pkg.py

@@ -272,38 +272,53 @@ class PkgModuleTest(ModuleCase, SaltReturnAssertsMixin):
         self.run_function('pkg.refresh_db')
 
         if os_family == 'Suse':
-            # pkg.latest version returns empty if the latest version is already installed
-            vim_version_dict = self.run_function('pkg.latest_version', ['vim'])
-            vim_info = self.run_function('pkg.info_available', ['vim'])['vim']
-            if vim_version_dict == {}:
-                # Latest version is installed, get its version and construct
-                # a version selector so the immediately previous version is selected
-                vim_version = 'version=<'+vim_info['version']
-            else:
-                # Vim was not installed, so pkg.latest_version returns the latest one.
-                # Construct a version selector so immediately previous version is selected
-                vim_version = 'version=<'+vim_version_dict
+            # This test assumes that there are multiple possible versions of a
+            # package available. That makes it brittle if you pick just one
+            # target, as changes in the available packages will break the test.
+            # Therefore, we'll choose from several packages to make sure we get
+            # one that is suitable for this test.
+            packages = ('hwinfo', 'avrdude', 'diffoscope', 'vim')
 
-            # Only install a new version of vim if vim is up-to-date, otherwise we don't
-            # need this check. (And the test will fail when we check for the empty dict
-            # since vim gets upgraded in the install step.)
-            if 'out-of-date' not in vim_info['status']:
-                # Install a version of vim that should need upgrading
-                ret = self.run_function('pkg.install', ['vim', vim_version])
+            available = self.run_function('pkg.list_repo_pkgs', packages)
+            versions = self.run_function('pkg.version', packages)
+
+            for package in packages:
+                try:
+                    new, old = available[package][:2]
+                except (KeyError, ValueError):
+                    # Package not available, or less than 2 versions
+                    # available. This is not a suitable target.
+                    continue
+                else:
+                    target = package
+                    current = versions[target]
+                    break
+            else:
+                # None of the packages have more than one version available, so
+                # we need to find new package(s). pkg.list_repo_pkgs can be
+                # used to get an overview of the available packages. We should
+                # try to find packages with few dependencies and small download
+                # sizes, to keep this test from taking longer than necessary.
+                self.fail('No suitable package found for this test')
+
+            # Make sure we have the 2nd-oldest available version installed
+            ret = self.run_function('pkg.install', [target], version=old)
             if not isinstance(ret, dict):
                 if ret.startswith('ERROR'):
-                        self.skipTest('Could not install earlier vim to complete test.')
-                else:
-                    self.assertNotEqual(ret, {})
+                    self.skipTest(
+                        'Could not install older {0} to complete '
+                        'test.'.format(target)
+                    )
 
-            # Run a system upgrade, which should catch the fact that Vim needs upgrading, and upgrade it.
+            # Run a system upgrade, which should catch the fact that the
+            # targeted package needs upgrading, and upgrade it.
             ret = self.run_function(func)
 
             # The changes dictionary should not be empty.
             if 'changes' in ret:
-                self.assertIn('vim', ret['changes'])
+                self.assertIn(target, ret['changes'])
             else:
-                self.assertIn('vim', ret)
+                self.assertIn(target, ret)
         else:
             ret = self.run_function('pkg.list_upgrades')
             if ret == '' or ret == {}:

tests/integration/modules/test_state.py

@@ -1464,6 +1464,56 @@ class StateModuleTest(ModuleCase, SaltReturnAssertsMixin):
         test_data = state_run['cmd_|-test_non_failing_state_|-echo "Should not run"_|-run']
         self.assertIn('duration', test_data)
 
+    def test_multiple_onfail_requisite_with_required(self):
+        '''
+        test to ensure multiple states are run
+        when specified as onfails for a single state.
+        This is a test for the issue:
+        https://github.com/saltstack/salt/issues/46552
+        '''
+
+        state_run = self.run_function('state.sls', mods='requisites.onfail_multiple_required')
+
+        retcode = state_run['cmd_|-b_|-echo b_|-run']['changes']['retcode']
+        self.assertEqual(retcode, 0)
+
+        retcode = state_run['cmd_|-c_|-echo c_|-run']['changes']['retcode']
+        self.assertEqual(retcode, 0)
+
+        retcode = state_run['cmd_|-d_|-echo d_|-run']['changes']['retcode']
+        self.assertEqual(retcode, 0)
+
+        stdout = state_run['cmd_|-b_|-echo b_|-run']['changes']['stdout']
+        self.assertEqual(stdout, 'b')
+
+        stdout = state_run['cmd_|-c_|-echo c_|-run']['changes']['stdout']
+        self.assertEqual(stdout, 'c')
+
+        stdout = state_run['cmd_|-d_|-echo d_|-run']['changes']['stdout']
+        self.assertEqual(stdout, 'd')
+
+    def test_multiple_onfail_requisite_with_required_no_run(self):
+        '''
+        test to ensure multiple states are not run
+        when specified as onfails for a single state
+        which fails.
+        This is a test for the issue:
+        https://github.com/saltstack/salt/issues/46552
+        '''
+
+        state_run = self.run_function('state.sls', mods='requisites.onfail_multiple_required_no_run')
+
+        expected = 'State was not run because onfail req did not change'
+
+        stdout = state_run['cmd_|-b_|-echo b_|-run']['comment']
+        self.assertEqual(stdout, expected)
+
+        stdout = state_run['cmd_|-c_|-echo c_|-run']['comment']
+        self.assertEqual(stdout, expected)
+
+        stdout = state_run['cmd_|-d_|-echo d_|-run']['comment']
+        self.assertEqual(stdout, expected)
+
     # listen tests
 
     def test_listen_requisite(self):

tests/integration/shell/test_auth.py

@@ -59,12 +59,18 @@ class AuthTest(ShellCase):
     def setUp(self):
         for user in (self.userA, self.userB):
             try:
+                if salt.utils.is_darwin() and user not in str(self.run_call('user.list_users')):
+                    # workaround for https://github.com/saltstack/salt-jenkins/issues/504
+                    raise KeyError
                 pwd.getpwnam(user)
             except KeyError:
                 self.run_call('user.add {0} createhome=False'.format(user))
 
         # only put userB into the group for the group auth test
         try:
+            if salt.utils.is_darwin() and self.group not in str(self.run_call('group.info {0}'.format(self.group))):
+                # workaround for https://github.com/saltstack/salt-jenkins/issues/504
+                raise KeyError
             grp.getgrnam(self.group)
         except KeyError:
             self.run_call('group.add {0}'.format(self.group))

tests/integration/states/test_npm.py

@@ -6,12 +6,14 @@
 '''
 # Import Python libs
 from __future__ import absolute_import, unicode_literals, print_function
+import os
 
 # Import Salt Testing libs
 from tests.support.case import ModuleCase
 from tests.support.unit import skipIf
 from tests.support.helpers import destructiveTest, requires_network
 from tests.support.mixins import SaltReturnAssertsMixin
+from tests.support.runtests import RUNTIME_VARS
 
 # Import salt libs
 import salt.modules.cmdmod as cmd
@@ -42,10 +44,19 @@ class NpmStateTest(ModuleCase, SaltReturnAssertsMixin):
         '''
         Determine if URL-referenced NPM module can be successfully installed.
         '''
-        ret = self.run_state('npm.installed', name='request/request#v2.81.1')
+        if LooseVersion(cmd.run('npm -v')) >= LooseVersion(MAX_NPM_VERSION):
+            user = os.environ.get('SUDO_USER', 'root')
+            npm_dir = os.path.join(RUNTIME_VARS.TMP, 'git-install-npm')
+            self.run_state('file.directory', name=npm_dir, user=user, dir_mode='755')
+        else:
+            user = None
+            npm_dir = None
+        ret = self.run_state('npm.installed', name='request/request#v2.81.1', runas=user, dir=npm_dir)
         self.assertSaltTrueReturn(ret)
-        ret = self.run_state('npm.removed', name='git://github.com/request/request')
+        ret = self.run_state('npm.removed', name='git://github.com/request/request', runas=user, dir=npm_dir)
         self.assertSaltTrueReturn(ret)
+        if npm_dir is not None:
+            self.run_state('file.absent', name=npm_dir)
 
     @requires_network()
     @destructiveTest

tests/integration/states/test_pip.py

@@ -14,6 +14,7 @@ import os
 import pwd
 import glob
 import shutil
+import sys
 
 # Import Salt Testing libs
 from tests.support.mixins import SaltReturnAssertsMixin
@@ -525,6 +526,7 @@ class PipStateTest(ModuleCase, SaltReturnAssertsMixin):
             if os.path.isdir(venv_dir):
                 shutil.rmtree(venv_dir)
 
+    @skipIf(sys.version_info[:2] >= (3, 6), 'Old version of virtualenv too old for python3.6')
     def test_46127_pip_env_vars(self):
         '''
         Test that checks if env_vars passed to pip.installed are also passed

tests/integration/states/test_pkgrepo.py

@@ -22,16 +22,16 @@ import salt.utils.platform
 from salt.ext import six
 
 
+@destructiveTest
+@skipIf(salt.utils.platform.is_windows(), 'minion is windows')
 class PkgrepoTest(ModuleCase, SaltReturnAssertsMixin):
     '''
     pkgrepo state tests
     '''
-    @destructiveTest
-    @skipIf(salt.utils.platform.is_windows(), 'minion is windows')
     @requires_system_grains
     def test_pkgrepo_01_managed(self, grains):
         '''
-        This is a destructive test as it adds a repository.
+        Test adding a repo
         '''
         os_grain = self.run_function('grains.item', ['os'])['os']
         os_release_info = tuple(self.run_function('grains.item', ['osrelease_info'])['osrelease_info'])
@@ -56,12 +56,9 @@ class PkgrepoTest(ModuleCase, SaltReturnAssertsMixin):
         for state_id, state_result in six.iteritems(ret):
             self.assertSaltTrueReturn(dict([(state_id, state_result)]))
 
-    @destructiveTest
-    @skipIf(salt.utils.platform.is_windows(), 'minion is windows')
     def test_pkgrepo_02_absent(self):
         '''
-        This is a destructive test as it removes the repository added in the
-        above test.
+        Test removing the repo from the above test
         '''
         os_grain = self.run_function('grains.item', ['os'])['os']
         os_release_info = tuple(self.run_function('grains.item', ['osrelease_info'])['osrelease_info'])
@@ -78,3 +75,56 @@ class PkgrepoTest(ModuleCase, SaltReturnAssertsMixin):
         self.assertReturnNonEmptySaltType(ret)
         for state_id, state_result in six.iteritems(ret):
             self.assertSaltTrueReturn(dict([(state_id, state_result)]))
+
+    @requires_system_grains
+    def test_pkgrepo_03_with_comments(self, grains):
+        '''
+        Test adding a repo with comments
+        '''
+        os_family = grains['os_family'].lower()
+
+        if os_family in ('redhat', 'suse'):
+            kwargs = {
+                'name': 'examplerepo',
+                'baseurl': 'http://example.com/repo',
+                'enabled': False,
+                'comments': ['This is a comment']
+            }
+        elif os_family in ('debian',):
+            self.skipTest('Debian/Ubuntu test case needed')
+        else:
+            self.skipTest("No test case for os_family '{0}'".format(os_family))
+
+        try:
+            # Run the state to add the repo
+            ret = self.run_state('pkgrepo.managed', **kwargs)
+            self.assertSaltTrueReturn(ret)
+
+            # Run again with modified comments
+            kwargs['comments'].append('This is another comment')
+            ret = self.run_state('pkgrepo.managed', **kwargs)
+            self.assertSaltTrueReturn(ret)
+            ret = ret[next(iter(ret))]
+            self.assertEqual(
+                ret['changes'],
+                {
+                    'comments': {
+                        'old': ['This is a comment'],
+                        'new': ['This is a comment',
+                                'This is another comment']
+                    }
+                }
+            )
+
+            # Run a third time, no changes should be made
+            ret = self.run_state('pkgrepo.managed', **kwargs)
+            self.assertSaltTrueReturn(ret)
+            ret = ret[next(iter(ret))]
+            self.assertFalse(ret['changes'])
+            self.assertEqual(
+                ret['comment'],
+                "Package repo '{0}' already configured".format(kwargs['name'])
+            )
+        finally:
+            # Clean up
+            self.run_state('pkgrepo.absent', name=kwargs['name'])

tests/runtests.py

@@ -736,6 +736,7 @@ class SaltTestsuiteParser(SaltCoverageTestingParser):
                         continue
                     results = self.run_suite('', name, suffix='test_*.py', load_from_name=True)
                     status.append(results)
+                return status
             for suite in TEST_SUITES:
                 if suite != 'unit' and getattr(self.options, suite):
                     status.append(self.run_integration_suite(**TEST_SUITES[suite]))

tests/unit/modules/test_cmdmod.py

@@ -303,7 +303,7 @@ class CMDMODTestCase(TestCase, LoaderModuleMockMixin):
                 environment = os.environ.copy()
 
                 popen_mock.return_value = Mock(
-                    communicate=lambda *args, **kwags: ['{}', None],
+                    communicate=lambda *args, **kwags: [b'', None],
                     pid=lambda: 1,
                     retcode=0
                 )

tests/unit/returners/test_local_cache.py

@@ -10,6 +10,7 @@ Unit tests for the Default Job Cache (local_cache).
 from __future__ import absolute_import, print_function, unicode_literals
 import os
 import shutil
+import time
 import logging
 import tempfile
 import time
@@ -82,6 +83,11 @@ class LocalCacheCleanOldJobsTestCase(TestCase, LoaderModuleMockMixin):
         # Call clean_old_jobs function, patching the keep_jobs value with a
         # very small value to force the call to clean the job.
         with patch.dict(local_cache.__opts__, {'keep_jobs': 0.00000001}):
+            # Sleep on Windows because time.time is only precise to 3 decimal
+            # points, and therefore subtracting the jid_ctime from time.time
+            # will result in a negative number
+            if salt.utils.platform.is_windows():
+                time.sleep(0.25)
             local_cache.clean_old_jobs()
 
         # Assert that the JID dir was removed
@@ -149,6 +155,11 @@ class LocalCacheCleanOldJobsTestCase(TestCase, LoaderModuleMockMixin):
         # Call clean_old_jobs function, patching the keep_jobs value with a
         # very small value to force the call to clean the job.
         with patch.dict(local_cache.__opts__, {'keep_jobs': 0.00000001}):
+            # Sleep on Windows because time.time is only precise to 3 decimal
+            # points, and therefore subtracting the jid_ctime from time.time
+            # will result in a negative number
+            if salt.utils.platform.is_windows():
+                time.sleep(0.25)
             local_cache.clean_old_jobs()
 
         # Assert that the JID dir was removed

tests/unit/returners/test_smtp_return.py

@@ -66,8 +66,8 @@ class SMTPReturnerTestCase(TestCase, LoaderModuleMockMixin):
                                             'renderer': 'jinja|yaml',
                                             'renderer_blacklist': [],
                                             'renderer_whitelist': [],
-                                            'file_roots': [],
-                                            'pillar_roots': [],
+                                            'file_roots': {},
+                                            'pillar_roots': {},
                                             'cachedir': '/'}), \
                     patch('salt.returners.smtp_return.gnupg'), \
                     patch('salt.returners.smtp_return.smtplib.SMTP') as mocked_smtplib:
@@ -79,8 +79,8 @@ class SMTPReturnerTestCase(TestCase, LoaderModuleMockMixin):
                                             'renderer': 'jinja|yaml',
                                             'renderer_blacklist': [],
                                             'renderer_whitelist': [],
-                                            'file_roots': [],
-                                            'pillar_roots': [],
+                                            'file_roots': {},
+                                            'pillar_roots': {},
                                             'cachedir': '/'}), \
                     patch('salt.returners.smtp_return.smtplib.SMTP') as mocked_smtplib:
                 self._test_returner(mocked_smtplib)

tests/unit/test_pillar.py

@@ -433,6 +433,66 @@ class PillarTestCase(TestCase):
                 ({'foo': 'bar', 'nested': {'level': {'foo': 'bar2'}}}, [])
             )
 
+    def test_includes_override_sls(self):
+        opts = {
+            'renderer': 'json',
+            'renderer_blacklist': [],
+            'renderer_whitelist': [],
+            'state_top': '',
+            'pillar_roots': [],
+            'file_roots': [],
+            'extension_modules': ''
+        }
+        grains = {
+            'os': 'Ubuntu',
+            'os_family': 'Debian',
+            'oscodename': 'raring',
+            'osfullname': 'Ubuntu',
+            'osrelease': '13.04',
+            'kernel': 'Linux'
+        }
+        with patch('salt.pillar.compile_template') as compile_template:
+
+            # Test with option set to True
+            opts['pillar_includes_override_sls'] = True
+            pillar = salt.pillar.Pillar(opts, grains, 'mocked-minion', 'base')
+            # Mock getting the proper template files
+            pillar.client.get_state = MagicMock(
+                return_value={
+                    'dest': '/path/to/pillar/files/foo.sls',
+                    'source': 'salt://foo.sls'
+                }
+            )
+
+            compile_template.side_effect = [
+                {'foo': 'bar', 'include': ['blah']},
+                {'foo': 'bar2'}
+            ]
+            self.assertEqual(
+                pillar.render_pillar({'base': ['foo.sls']}),
+                ({'foo': 'bar2'}, [])
+            )
+
+            # Test with option set to False
+            opts['pillar_includes_override_sls'] = False
+            pillar = salt.pillar.Pillar(opts, grains, 'mocked-minion', 'base')
+            # Mock getting the proper template files
+            pillar.client.get_state = MagicMock(
+                return_value={
+                    'dest': '/path/to/pillar/files/foo.sls',
+                    'source': 'salt://foo.sls'
+                }
+            )
+
+            compile_template.side_effect = [
+                {'foo': 'bar', 'include': ['blah']},
+                {'foo': 'bar2'}
+            ]
+            self.assertEqual(
+                pillar.render_pillar({'base': ['foo.sls']}),
+                ({'foo': 'bar'}, [])
+            )
+
     def test_topfile_order(self):
         with patch('salt.pillar.salt.fileclient.get_file_client', autospec=True) as get_file_client, \
                 patch('salt.pillar.salt.minion.Matcher') as Matcher:  # autospec=True disabled due to py3 mock bug