From 3461c682c3696b8cd751f54370e2793ab1dcafb8 Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Tue, 28 Jan 2020 12:12:14 -0500 Subject: [PATCH] Add mechanism for storing warnings and deprecations outside of AnsibleModule (#58993) * Move warn() and deprecate() methods out of basic.py * Use _global_warnings and _global_deprications and create accessor functions - This lays the foundation for future functions being moved outside of AnsibleModule that need an interface to warnings and deprecations without modifying them. * Add unit tests for new warn and deprecate functions --- .../add-global-warnings-container.yaml | 2 + lib/ansible/module_utils/basic.py | 46 ++++++------- lib/ansible/module_utils/common/warnings.py | 35 ++++++++++ .../module_common/test_recursive_finder.py | 2 + .../module_utils/basic/test_argument_spec.py | 11 ++-- .../common/warnings/test_deprecate.py | 66 +++++++++++++++++++ .../module_utils/common/warnings/test_warn.py | 61 +++++++++++++++++ 7 files changed, 193 insertions(+), 30 deletions(-) create mode 100644 changelogs/fragments/add-global-warnings-container.yaml create mode 100644 lib/ansible/module_utils/common/warnings.py create mode 100644 test/units/module_utils/common/warnings/test_deprecate.py create mode 100644 test/units/module_utils/common/warnings/test_warn.py diff --git a/changelogs/fragments/add-global-warnings-container.yaml b/changelogs/fragments/add-global-warnings-container.yaml new file mode 100644 index 00000000000..53182311823 --- /dev/null +++ b/changelogs/fragments/add-global-warnings-container.yaml @@ -0,0 +1,2 @@ +minor_changes: + - add mechanism for storing warnings and deprecations globally and not attached to an ``AnsibleModule`` object (https://github.com/ansible/ansible/pull/58993) diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 2913e6aa802..94a1d3c08dc 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -193,6 +193,12 @@ from ansible.module_utils.common.validation import ( ) from ansible.module_utils.common._utils import get_all_subclasses as _get_all_subclasses from ansible.module_utils.parsing.convert_bool import BOOLEANS, BOOLEANS_FALSE, BOOLEANS_TRUE, boolean +from ansible.module_utils.common.warnings import ( + deprecate, + get_deprecation_messages, + get_warning_messages, + warn, +) # Note: When getting Sequence from collections, it matches with strings. If # this matters, make sure to check for strings before checking for sequencetype @@ -612,8 +618,6 @@ class AnsibleModule(object): # May be used to set modifications to the environment for any # run_command invocation self.run_command_environ_update = {} - self._warnings = [] - self._deprecations = [] self._clean = {} self._string_conversion_action = '' @@ -728,22 +732,12 @@ class AnsibleModule(object): return self._tmpdir def warn(self, warning): - - if isinstance(warning, string_types): - self._warnings.append(warning) - self.log('[WARNING] %s' % warning) - else: - raise TypeError("warn requires a string not a %s" % type(warning)) + warn(warning) + self.log('[WARNING] %s' % warning) def deprecate(self, msg, version=None): - if isinstance(msg, string_types): - self._deprecations.append({ - 'msg': msg, - 'version': version - }) - self.log('[DEPRECATION WARNING] %s %s' % (msg, version)) - else: - raise TypeError("deprecate requires a string not a %s" % type(msg)) + deprecate(msg, version) + self.log('[DEPRECATION WARNING] %s %s' % (msg, version)) def load_file_common_arguments(self, params): ''' @@ -1409,7 +1403,7 @@ class AnsibleModule(object): alias_warnings = [] alias_results, self._legal_inputs = handle_aliases(spec, param, alias_warnings=alias_warnings) for option, alias in alias_warnings: - self._warnings.append('Both option %s and its alias %s are set.' % (option_prefix + option, option_prefix + alias)) + warn('Both option %s and its alias %s are set.' % (option_prefix + option, option_prefix + alias)) deprecated_aliases = [] for i in spec.keys(): @@ -1419,9 +1413,7 @@ class AnsibleModule(object): for deprecation in deprecated_aliases: if deprecation['name'] in param.keys(): - self._deprecations.append( - {'msg': "Alias '%s' is deprecated. See the module docs for more information" % deprecation['name'], - 'version': deprecation['version']}) + deprecate("Alias '%s' is deprecated. See the module docs for more information" % deprecation['name'], deprecation['version']) return alias_results def _handle_no_log_values(self, spec=None, param=None): @@ -1435,7 +1427,9 @@ class AnsibleModule(object): except TypeError as te: self.fail_json(msg="Failure when processing no_log parameters. Module invocation will be hidden. " "%s" % to_native(te), invocation={'module_args': 'HIDDEN DUE TO FAILURE'}) - self._deprecations.extend(list_deprecations(spec, param)) + + for msg, version in list_deprecations(spec, param): + deprecate(msg, version) def _check_arguments(self, spec=None, param=None, legal_inputs=None): self._syslog_facility = 'LOG_USER' @@ -2026,8 +2020,9 @@ class AnsibleModule(object): else: self.warn(kwargs['warnings']) - if self._warnings: - kwargs['warnings'] = self._warnings + warnings = get_warning_messages() + if warnings: + kwargs['warnings'] = warnings if 'deprecations' in kwargs: if isinstance(kwargs['deprecations'], list): @@ -2041,8 +2036,9 @@ class AnsibleModule(object): else: self.deprecate(kwargs['deprecations']) # pylint: disable=ansible-deprecated-no-version - if self._deprecations: - kwargs['deprecations'] = self._deprecations + deprecations = get_deprecation_messages() + if deprecations: + kwargs['deprecations'] = deprecations kwargs = remove_values(kwargs, self.no_log_values) print('\n%s' % self.jsonify(kwargs)) diff --git a/lib/ansible/module_utils/common/warnings.py b/lib/ansible/module_utils/common/warnings.py new file mode 100644 index 00000000000..18a04709da8 --- /dev/null +++ b/lib/ansible/module_utils/common/warnings.py @@ -0,0 +1,35 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2019 Ansible Project +# Simplified BSD License (see licenses/simplified_bsd.txt or https://opensource.org/licenses/BSD-2-Clause) + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +from ansible.module_utils.six import string_types + +_global_warnings = [] +_global_deprecations = [] + + +def warn(warning): + if isinstance(warning, string_types): + _global_warnings.append(warning) + else: + raise TypeError("warn requires a string not a %s" % type(warning)) + + +def deprecate(msg, version=None): + if isinstance(msg, string_types): + _global_deprecations.append({'msg': msg, 'version': version}) + else: + raise TypeError("deprecate requires a string not a %s" % type(msg)) + + +def get_warning_messages(): + """Return a tuple of warning messages accumulated over this run""" + return tuple(_global_warnings) + + +def get_deprecation_messages(): + """Return a tuple of deprecations accumulated over this run""" + return tuple(_global_deprecations) diff --git a/test/units/executor/module_common/test_recursive_finder.py b/test/units/executor/module_common/test_recursive_finder.py index 4a6d43b3536..b57de77acbc 100644 --- a/test/units/executor/module_common/test_recursive_finder.py +++ b/test/units/executor/module_common/test_recursive_finder.py @@ -48,6 +48,7 @@ MODULE_UTILS_BASIC_IMPORTS = frozenset((('ansible', '__init__'), ('ansible', 'module_utils', 'common', 'parameters'), ('ansible', 'module_utils', 'common', 'process'), ('ansible', 'module_utils', 'common', 'sys_info'), + ('ansible', 'module_utils', 'common', 'warnings'), ('ansible', 'module_utils', 'common', 'text', '__init__'), ('ansible', 'module_utils', 'common', 'text', 'converters'), ('ansible', 'module_utils', 'common', 'text', 'formatters'), @@ -69,6 +70,7 @@ MODULE_UTILS_BASIC_FILES = frozenset(('ansible/module_utils/_text.py', 'ansible/module_utils/common/_json_compat.py', 'ansible/module_utils/common/collections.py', 'ansible/module_utils/common/parameters.py', + 'ansible/module_utils/common/warnings.py', 'ansible/module_utils/parsing/convert_bool.py', 'ansible/module_utils/common/__init__.py', 'ansible/module_utils/common/file.py', diff --git a/test/units/module_utils/basic/test_argument_spec.py b/test/units/module_utils/basic/test_argument_spec.py index 744df3083d9..4caf7f62f85 100644 --- a/test/units/module_utils/basic/test_argument_spec.py +++ b/test/units/module_utils/basic/test_argument_spec.py @@ -14,6 +14,7 @@ import pytest from units.compat.mock import MagicMock from ansible.module_utils import basic +from ansible.module_utils.common.warnings import get_deprecation_messages, get_warning_messages from ansible.module_utils.six import integer_types from ansible.module_utils.six.moves import builtins @@ -238,7 +239,7 @@ class TestComplexArgSpecs: """Test that the complex argspec issues a warning if we specify an option both with its canonical name and its alias""" am = basic.AnsibleModule(**complex_argspec) assert isinstance(am.params['foo'], str) - assert 'Both option foo and its alias dup are set.' in am._warnings + assert 'Both option foo and its alias dup are set.' in get_warning_messages() assert am.params['foo'] == 'hello2' @pytest.mark.parametrize('stdin', [{'foo': 'hello', 'bam': 'test'}], indirect=['stdin']) @@ -338,8 +339,8 @@ class TestComplexArgSpecs: """Test a deprecated alias""" am = basic.AnsibleModule(**complex_argspec) - assert "Alias 'zodraz' is deprecated." in am._deprecations[0]['msg'] - assert am._deprecations[0]['version'] == '9.99' + assert "Alias 'zodraz' is deprecated." in get_deprecation_messages()[0]['msg'] + assert get_deprecation_messages()[0]['version'] == '9.99' class TestComplexOptions: @@ -615,7 +616,7 @@ def test_no_log_false(stdin, capfd): "arg_pass": {"no_log": False} } am = basic.AnsibleModule(arg_spec) - assert "testing" not in am.no_log_values and not am._warnings + assert "testing" not in am.no_log_values and not get_warning_messages() @pytest.mark.parametrize("stdin", [{"arg_pass": "testing"}], indirect=["stdin"]) @@ -629,4 +630,4 @@ def test_no_log_none(stdin, capfd): # Omitting no_log is only picked up by _log_invocation, so the value never # makes it into am.no_log_values. Instead we can check for the warning # emitted by am._log_invocation. - assert len(am._warnings) > 0 + assert len(get_warning_messages()) > 0 diff --git a/test/units/module_utils/common/warnings/test_deprecate.py b/test/units/module_utils/common/warnings/test_deprecate.py new file mode 100644 index 00000000000..b85264d0eba --- /dev/null +++ b/test/units/module_utils/common/warnings/test_deprecate.py @@ -0,0 +1,66 @@ +# -*- coding: utf-8 -*- +# (c) 2019 Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +import pytest + +import ansible.module_utils.common.warnings as warnings + +from ansible.module_utils.common.warnings import deprecate, get_deprecation_messages +from ansible.module_utils.six import PY3 + + +@pytest.fixture +def deprecation_messages(): + return [ + {'msg': 'First deprecation', 'version': None}, + {'msg': 'Second deprecation', 'version': '2.14'}, + {'msg': 'Third deprecation', 'version': '2.9'}, + ] + + +def test_deprecate_message_only(): + deprecate('Deprecation message') + assert warnings._global_deprecations == [{'msg': 'Deprecation message', 'version': None}] + + +def test_deprecate_with_version(): + deprecate(msg='Deprecation message', version='2.14') + assert warnings._global_deprecations == [{'msg': 'Deprecation message', 'version': '2.14'}] + + +def test_multiple_deprecations(deprecation_messages): + for d in deprecation_messages: + deprecate(**d) + + assert deprecation_messages == warnings._global_deprecations + + +def test_get_deprecation_messages(deprecation_messages): + for d in deprecation_messages: + deprecate(**d) + + accessor_deprecations = get_deprecation_messages() + assert isinstance(accessor_deprecations, tuple) + assert len(accessor_deprecations) == 3 + + +@pytest.mark.parametrize( + 'test_case', + ( + 1, + True, + [1], + {'k1': 'v1'}, + (1, 2), + 6.62607004, + b'bytestr' if PY3 else None, + None, + ) +) +def test_deprecate_failure(test_case): + with pytest.raises(TypeError, match='deprecate requires a string not a %s' % type(test_case)): + deprecate(test_case) diff --git a/test/units/module_utils/common/warnings/test_warn.py b/test/units/module_utils/common/warnings/test_warn.py new file mode 100644 index 00000000000..020b0625218 --- /dev/null +++ b/test/units/module_utils/common/warnings/test_warn.py @@ -0,0 +1,61 @@ +# -*- coding: utf-8 -*- +# (c) 2019 Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +import pytest + +import ansible.module_utils.common.warnings as warnings + +from ansible.module_utils.common.warnings import warn, get_warning_messages +from ansible.module_utils.six import PY3 + + +@pytest.fixture +def warning_messages(): + return [ + 'First warning', + 'Second warning', + 'Third warning', + ] + + +def test_warn(): + warn('Warning message') + assert warnings._global_warnings == ['Warning message'] + + +def test_multiple_warningss(warning_messages): + for w in warning_messages: + warn(w) + + assert warning_messages == warnings._global_warnings + + +def test_get_warning_messages(warning_messages): + for w in warning_messages: + warn(w) + + accessor_warnings = get_warning_messages() + assert isinstance(accessor_warnings, tuple) + assert len(accessor_warnings) == 3 + + +@pytest.mark.parametrize( + 'test_case', + ( + 1, + True, + [1], + {'k1': 'v1'}, + (1, 2), + 6.62607004, + b'bytestr' if PY3 else None, + None, + ) +) +def test_warn_failure(test_case): + with pytest.raises(TypeError, match='warn requires a string not a %s' % type(test_case)): + warn(test_case)