Show a deprecation warning if no_site_packages is used. Fixes #6027.

If both `no_site_packages` and `system_site_packages` are used, throw an error.
Added the appropriate tests.

salt/modules/virtualenv_mod.py

@@ -4,9 +4,12 @@ Create virtualenv environments
 
 # Import python libs
 import logging
+import warnings
 
 # Import salt libs
-from salt import utils
+import salt.utils
+import salt.exceptions
+
 
 # Import 3rd party libs
 try:
@@ -55,7 +58,7 @@ def create(path,
         The name (and optionally path) of the virtualenv command. This can also
         be set globally in the minion config file as ``virtualenv.venv_bin``.
     no_site_packages : False
-        Passthrough argument given to virtualenv
+        Passthrough argument given to virtualenv (Deprecated since 0.17.0)
     system_site_packages : False
         Passthrough argument given to virtualenv or pyvenv
     distribute : False
@@ -84,7 +87,28 @@ def create(path,
     if venv_bin is None:
         venv_bin = __opts__.get('venv_bin') or __pillar__.get('venv_bin')
     # raise CommandNotFoundError if venv_bin is missing
-    utils.check_or_die(venv_bin)
+    salt.utils.check_or_die(venv_bin)
+
+    if no_site_packages:
+        # Show a deprecation warning
+        # XXX: Remove deprecation warning message on 0.18.0
+        warnings.filterwarnings(
+            'once', '', DeprecationWarning, __name__
+        )
+        warnings.warn(
+            '\'no_site_packages\' has been deprecated. Please start using '
+            '\'system_site_packages=False\' which means exactly the same '
+            'as \'no_site_packages=True\'',
+            DeprecationWarning
+        )
+
+    if no_site_packages and system_site_packages:
+        raise salt.exceptions.CommandExecutionError(
+            '\'no_site_packages\' and \'system_site_packages\' are mutually '
+            'exclusive options. Please use only one, and prefer '
+            '\'system_site_packages\' since \'no_site_packages\' has been '
+            'deprecated.'
+        )
 
     cmd = [venv_bin]
 
@@ -92,8 +116,6 @@ def create(path,
         # Virtualenv package
         if no_site_packages:
             cmd.append('--no-site-packages')
-        if system_site_packages:
-            cmd.append('--system-site-packages')
         if distribute:
             if VIRTUALENV_VERSION_INFO >= (1, 10):
                 log.info(

tests/unit/modules/virtualenv_test.py

@@ -8,7 +8,9 @@
     :license: Apache 2.0, see LICENSE for more details.
 '''
 
+# Import python libraries
 import sys
+import warnings
 
 # Import Salt Testing libs
 from salttesting import skipIf, TestCase
@@ -35,6 +37,7 @@ except ImportError:
 
 # Import salt libs
 from salt.modules import virtualenv_mod
+from salt.exceptions import CommandExecutionError
 
 virtualenv_mod.__salt__ = {'cmd.which_bin': lambda _: 'virtualenv'}
 
@@ -50,10 +53,10 @@ class VirtualenvTestCase(TestCase):
 
         with patch.dict(virtualenv_mod.__salt__, {'cmd.run_all': mock}):
             virtualenv_mod.create(
-                '/tmp/foo', no_site_packages=True, distribute=True
+                '/tmp/foo', system_site_packages=True, distribute=True
             )
             mock.assert_called_once_with(
-                'virtualenv --no-site-packages --distribute /tmp/foo',
+                'virtualenv --distribute --system-site-packages /tmp/foo',
                 runas=None
             )
 
@@ -62,10 +65,10 @@ class VirtualenvTestCase(TestCase):
             mock = MagicMock(return_value={'retcode': 0, 'stdout': ''})
             with patch.dict(virtualenv_mod.__salt__, {'cmd.run_all': mock}):
                 virtualenv_mod.create(
-                    '/tmp/foo', no_site_packages=True, distribute=True
+                    '/tmp/foo', system_site_packages=True, distribute=True
                 )
                 mock.assert_called_once_with(
-                    'virtualenv --no-site-packages /tmp/foo', runas=None
+                    'virtualenv --system-site-packages /tmp/foo', runas=None
                 )
 
             # Are we logging the deprecation information?
@@ -150,6 +153,35 @@ class VirtualenvTestCase(TestCase):
                 runas=None
             )
 
+    def test_system_site_packages_and_no_site_packages_mutual_exclusion(self):
+        mock = MagicMock(return_value={'retcode': 0, 'stdout': ''})
+        with patch.dict(virtualenv_mod.__salt__, {'cmd.run_all': mock}):
+            self.assertRaises(
+                CommandExecutionError,
+                virtualenv_mod.create,
+                '/tmp/foo',
+                no_site_packages=True,
+                system_site_packages=True
+            )
+
+    def test_no_site_packages_deprecation(self):
+        # NOTE: If this test starts failing it might be because the deprecation
+        # warning was removed, or because some other test in this module is
+        # passing 'no_site_packages' to 'virtualenv_mod.create'. The
+        # deprecation warning is shown only once.
+
+        mock = MagicMock(return_value={'retcode': 0, 'stdout': ''})
+        with patch.dict(virtualenv_mod.__salt__, {'cmd.run_all': mock}):
+            with warnings.catch_warnings(record=True) as w:
+                virtualenv_mod.create(
+                    '/tmp/foo', no_site_packages=True
+                )
+                self.assertEqual(
+                    '\'no_site_packages\' has been deprecated. Please start '
+                    'using \'system_site_packages=False\' which means exactly '
+                    'the same as \'no_site_packages=True\'', str(w[-1].message)
+                )
+
 
 if __name__ == '__main__':
     from integration import run_tests