From 13cf46fc875b10c457aaa83587738d76b42cde4e Mon Sep 17 00:00:00 2001 From: Akmod Date: Tue, 26 Mar 2019 12:11:49 -0600 Subject: [PATCH 1/5] Make it possible to pass arbitrary arguments to pip --- salt/modules/pip.py | 47 ++++++++++++++++++++++++++++++++++ tests/unit/modules/test_pip.py | 22 ++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/salt/modules/pip.py b/salt/modules/pip.py index 64b2f7f9bbc0..45b2c445aef9 100644 --- a/salt/modules/pip.py +++ b/salt/modules/pip.py @@ -441,6 +441,7 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 no_cache_dir=False, cache_dir=None, no_binary=None, + pip_future=None, **kwargs): ''' Install packages with pip @@ -612,6 +613,26 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 no_cache_dir Disable the cache. + pip_future + pip keyword and positional arguments not yet implemented in salt + + .. code-block:: yaml + + pandas: + pip.installed: + - name: pandas + - pip_future: + - --latest-pip-kwarg: + - param1 + - param2 + - --latest-pip-arg + + Will be translated into the following pip command: + + .. code-block:: bash + + pip install pandas --latest-pip-kwarg param1 --latest-pip-kwarg parm2 --latest-pip-arg + CLI Example: .. code-block:: bash @@ -895,6 +916,32 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 if trusted_host: cmd.extend(['--trusted-host', trusted_host]) + if pip_future: + # These are arguments from the latest version of pip that + # have not yet been implemented in salt + for arg in pip_future: + # It is a keyword argument + if isinstance(arg, dict): + # There will only ever be one item in this dictionary + key, val = arg.popitem() + # There are multiple definitions for this keyword. I.E. + # - --keyword: + # - param1 + # - param2 + # Break it into "--keyword param1 --keyword param2" + if isinstance(val, list): + for list_arg in val: + cmd.append((key, list_arg)) + # Don't allow any recursion into keyword arg definitions + elif isinstance(val, dict): + raise RecursionError("Too many levels in arg: {}".format(arg)) + # This is a a normal one-to-one keyword argument + else: + cmd.extend(arg.items()) + # It is a positional argument, append it to the list + else: + cmd.append(arg) + cmd_kwargs = dict(saltenv=saltenv, use_vt=use_vt, runas=user) if kwargs: diff --git a/tests/unit/modules/test_pip.py b/tests/unit/modules/test_pip.py index 23e5f03e8e5d..da686d582f7c 100644 --- a/tests/unit/modules/test_pip.py +++ b/tests/unit/modules/test_pip.py @@ -803,6 +803,28 @@ def test_install_multiple_requirements_arguments_in_resulting_command(self): python_shell=False, ) + def test_install_pip_future_arguments_in_resulting_command(self): + pkg = 'pep8' + mock = MagicMock(return_value={'retcode': 0, 'stdout': ''}) + with patch.dict(pip.__salt__, {'cmd.run_all': mock}): + pip.install(pkg, pip_future=[ + {"--latest-pip-kwarg": ["param1", "param2"]}, + "--latest-pip-arg" + ]) + expected = [ + sys.executable, '-m', 'pip', 'install', pkg, + ("--latest-pip-kwarg", "param1"), + ("--latest-pip-kwarg", "param2"), + "--latest-pip-arg" + ] + mock.assert_called_with( + expected, + saltenv='base', + runas=None, + use_vt=False, + python_shell=False, + ) + def test_uninstall_multiple_requirements_arguments_in_resulting_command(self): with patch('salt.modules.pip._get_cached_requirements') as get_cached_requirements: cached_reqs = [ From 8143a4d67f2ea6a05fec1b695d988dca501fa456 Mon Sep 17 00:00:00 2001 From: Akmod Date: Tue, 26 Mar 2019 13:24:33 -0600 Subject: [PATCH 2/5] Flattened nested logic --- salt/modules/pip.py | 22 ++++++---------------- tests/unit/modules/test_pip.py | 23 +++++++++++++++++++---- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/salt/modules/pip.py b/salt/modules/pip.py index 45b2c445aef9..42a2613626d2 100644 --- a/salt/modules/pip.py +++ b/salt/modules/pip.py @@ -622,16 +622,14 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 pip.installed: - name: pandas - pip_future: - - --latest-pip-kwarg: - - param1 - - param2 + - --latest-pip-kwarg: param - --latest-pip-arg Will be translated into the following pip command: .. code-block:: bash - pip install pandas --latest-pip-kwarg param1 --latest-pip-kwarg parm2 --latest-pip-arg + pip install pandas --latest-pip-kwarg param --latest-pip-arg CLI Example: @@ -924,20 +922,12 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 if isinstance(arg, dict): # There will only ever be one item in this dictionary key, val = arg.popitem() - # There are multiple definitions for this keyword. I.E. - # - --keyword: - # - param1 - # - param2 - # Break it into "--keyword param1 --keyword param2" - if isinstance(val, list): - for list_arg in val: - cmd.append((key, list_arg)) # Don't allow any recursion into keyword arg definitions - elif isinstance(val, dict): - raise RecursionError("Too many levels in arg: {}".format(arg)) + # Don't allow multiple definitions of a keyword + if isinstance(val, (dict, list)): + raise TypeError("Too many levels in: {}".format(key)) # This is a a normal one-to-one keyword argument - else: - cmd.extend(arg.items()) + cmd.extend([key, val]) # It is a positional argument, append it to the list else: cmd.append(arg) diff --git a/tests/unit/modules/test_pip.py b/tests/unit/modules/test_pip.py index da686d582f7c..6eaa19342c34 100644 --- a/tests/unit/modules/test_pip.py +++ b/tests/unit/modules/test_pip.py @@ -808,14 +808,12 @@ def test_install_pip_future_arguments_in_resulting_command(self): mock = MagicMock(return_value={'retcode': 0, 'stdout': ''}) with patch.dict(pip.__salt__, {'cmd.run_all': mock}): pip.install(pkg, pip_future=[ - {"--latest-pip-kwarg": ["param1", "param2"]}, + {"--latest-pip-kwarg": "param"}, "--latest-pip-arg" ]) expected = [ sys.executable, '-m', 'pip', 'install', pkg, - ("--latest-pip-kwarg", "param1"), - ("--latest-pip-kwarg", "param2"), - "--latest-pip-arg" + "--latest-pip-kwarg", "param", "--latest-pip-arg" ] mock.assert_called_with( expected, @@ -825,6 +823,23 @@ def test_install_pip_future_arguments_in_resulting_command(self): python_shell=False, ) + def test_install_pip_future_arguments_recursion_error(self): + pkg = 'pep8' + mock = MagicMock(return_value={'retcode': 0, 'stdout': ''}) + with patch.dict(pip.__salt__, {'cmd.run_all': mock}): + + with self.subTest(params="Multiple arg Definition"): + self.assertRaises(TypeError, lambda: pip.install( + pkg, pip_future=[ + {"--latest-pip-kwarg": ["param1", "param2"]}, + ])) + + with self.subTest(params="Arg Recursion too deep"): + self.assertRaises(TypeError, lambda: pip.install( + pkg, pip_future=[ + {"--latest-pip-kwarg": [{"--too-deep": dict()}]}, + ])) + def test_uninstall_multiple_requirements_arguments_in_resulting_command(self): with patch('salt.modules.pip._get_cached_requirements') as get_cached_requirements: cached_reqs = [ From 63e7407af4e1d84fe89e51aa359b6c2f3484ab80 Mon Sep 17 00:00:00 2001 From: Akmod Date: Tue, 26 Mar 2019 13:55:58 -0600 Subject: [PATCH 3/5] Removed unittest call non-child class --- tests/unit/modules/test_pip.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/tests/unit/modules/test_pip.py b/tests/unit/modules/test_pip.py index 6eaa19342c34..5ac5bbf88f57 100644 --- a/tests/unit/modules/test_pip.py +++ b/tests/unit/modules/test_pip.py @@ -828,17 +828,15 @@ def test_install_pip_future_arguments_recursion_error(self): mock = MagicMock(return_value={'retcode': 0, 'stdout': ''}) with patch.dict(pip.__salt__, {'cmd.run_all': mock}): - with self.subTest(params="Multiple arg Definition"): - self.assertRaises(TypeError, lambda: pip.install( - pkg, pip_future=[ - {"--latest-pip-kwarg": ["param1", "param2"]}, - ])) - - with self.subTest(params="Arg Recursion too deep"): - self.assertRaises(TypeError, lambda: pip.install( - pkg, pip_future=[ - {"--latest-pip-kwarg": [{"--too-deep": dict()}]}, - ])) + self.assertRaises(TypeError, lambda: pip.install( + pkg, pip_future=[ + {"--latest-pip-kwarg": ["param1", "param2"]}, + ])) + + self.assertRaises(TypeError, lambda: pip.install( + pkg, pip_future=[ + {"--latest-pip-kwarg": [{"--too-deep": dict()}]}, + ])) def test_uninstall_multiple_requirements_arguments_in_resulting_command(self): with patch('salt.modules.pip._get_cached_requirements') as get_cached_requirements: From 0bb495d0198054938e9c1074abb6eba20cc84935 Mon Sep 17 00:00:00 2001 From: Akmod Date: Thu, 18 Apr 2019 13:53:33 -0600 Subject: [PATCH 4/5] Renamed pip_future and added documentation to pip_state - Changed `pip_future` to `extra_args` - Changed the execution module code block to a cli example - Moved the state.sls example to pip_state.py --- salt/modules/pip.py | 23 ++++++++++++----------- salt/states/pip_state.py | 23 ++++++++++++++++++++++- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/salt/modules/pip.py b/salt/modules/pip.py index 42a2613626d2..7d0616502bdd 100644 --- a/salt/modules/pip.py +++ b/salt/modules/pip.py @@ -79,6 +79,7 @@ # Import python libs import logging import os + try: import pkg_resources except ImportError: @@ -441,7 +442,7 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 no_cache_dir=False, cache_dir=None, no_binary=None, - pip_future=None, + extra_args=None, **kwargs): ''' Install packages with pip @@ -613,17 +614,17 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 no_cache_dir Disable the cache. - pip_future + extra_args pip keyword and positional arguments not yet implemented in salt - .. code-block:: yaml + .. code-block:: yaml + + salt '*' pip.install pandas extra_args="[{'--latest-pip-kwarg':'param'}, '--latest-pip-arg']" + + .. warning:: - pandas: - pip.installed: - - name: pandas - - pip_future: - - --latest-pip-kwarg: param - - --latest-pip-arg + If unsupported options are passed here that are not supported in a + minion's version of pip, a `No such option error` will be thrown. Will be translated into the following pip command: @@ -914,10 +915,10 @@ def install(pkgs=None, # pylint: disable=R0912,R0913,R0914 if trusted_host: cmd.extend(['--trusted-host', trusted_host]) - if pip_future: + if extra_args: # These are arguments from the latest version of pip that # have not yet been implemented in salt - for arg in pip_future: + for arg in extra_args: # It is a keyword argument if isinstance(arg, dict): # There will only ever be one item in this dictionary diff --git a/salt/states/pip_state.py b/salt/states/pip_state.py index edc79762f31b..6d80e4a9d5b0 100644 --- a/salt/states/pip_state.py +++ b/salt/states/pip_state.py @@ -21,8 +21,10 @@ # Import python libs from __future__ import absolute_import, print_function, unicode_literals -import re + import logging +import re + try: import pkg_resources HAS_PKG_RESOURCES = True @@ -341,6 +343,7 @@ def installed(name, no_cache_dir=False, cache_dir=None, no_binary=None, + extra_args=None, **kwargs): ''' Make sure the package is installed @@ -602,6 +605,23 @@ def installed(name, - reload_modules: True - exists_action: i + extra_args + pip keyword and positional arguments not yet implemented in salt + + .. code-block:: yaml + + pandas: + pip.installed: + - name: pandas + - extra_args: + - --latest-pip-kwarg: param + - --latest-pip-arg + + .. warning:: + + If unsupported options are passed here that are not supported in a + minion's version of pip, a `No such option error` will be thrown. + .. _`virtualenv`: http://www.virtualenv.org/en/latest/ ''' @@ -838,6 +858,7 @@ def installed(name, use_vt=use_vt, trusted_host=trusted_host, no_cache_dir=no_cache_dir, + extra_args=extra_args, **kwargs ) From 8886406b8ff5ccad2114c9ee84a7ca8b5d49fa57 Mon Sep 17 00:00:00 2001 From: Akmod Date: Thu, 18 Apr 2019 14:33:53 -0600 Subject: [PATCH 5/5] Changed `pip_future` to `extra_args` in test cases - solves https://github.com/saltstack/salt/issues/24751 --- tests/unit/modules/test_pip.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/unit/modules/test_pip.py b/tests/unit/modules/test_pip.py index 5ac5bbf88f57..394a98cd2161 100644 --- a/tests/unit/modules/test_pip.py +++ b/tests/unit/modules/test_pip.py @@ -2,18 +2,18 @@ # Import python libs from __future__ import absolute_import + import os import sys -# Import Salt Testing libs -from tests.support.mixins import LoaderModuleMockMixin -from tests.support.unit import skipIf, TestCase -from tests.support.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch - +import salt.modules.pip as pip # Import salt libs import salt.utils.platform -import salt.modules.pip as pip from salt.exceptions import CommandExecutionError +# Import Salt Testing libs +from tests.support.mixins import LoaderModuleMockMixin +from tests.support.mock import NO_MOCK, NO_MOCK_REASON, MagicMock, patch +from tests.support.unit import skipIf, TestCase @skipIf(NO_MOCK, NO_MOCK_REASON) @@ -803,11 +803,11 @@ def test_install_multiple_requirements_arguments_in_resulting_command(self): python_shell=False, ) - def test_install_pip_future_arguments_in_resulting_command(self): + def test_install_extra_args_arguments_in_resulting_command(self): pkg = 'pep8' mock = MagicMock(return_value={'retcode': 0, 'stdout': ''}) with patch.dict(pip.__salt__, {'cmd.run_all': mock}): - pip.install(pkg, pip_future=[ + pip.install(pkg, extra_args=[ {"--latest-pip-kwarg": "param"}, "--latest-pip-arg" ]) @@ -823,18 +823,18 @@ def test_install_pip_future_arguments_in_resulting_command(self): python_shell=False, ) - def test_install_pip_future_arguments_recursion_error(self): + def test_install_extra_args_arguments_recursion_error(self): pkg = 'pep8' mock = MagicMock(return_value={'retcode': 0, 'stdout': ''}) with patch.dict(pip.__salt__, {'cmd.run_all': mock}): self.assertRaises(TypeError, lambda: pip.install( - pkg, pip_future=[ + pkg, extra_args=[ {"--latest-pip-kwarg": ["param1", "param2"]}, ])) self.assertRaises(TypeError, lambda: pip.install( - pkg, pip_future=[ + pkg, extra_args=[ {"--latest-pip-kwarg": [{"--too-deep": dict()}]}, ]))