From 00ddc27d69624f4c70cc7dacb2d0f311a84863e9 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Mon, 1 Jul 2024 19:56:19 -0400 Subject: [PATCH] actually show plugin config warnings/deprecations (#82593) previouslly we recorded but did not show to avoid spam since we could not dedup from forks, that was already fixed in another PR so now we can show/display them. Also: * funcitonalize deprecation msg construct from docs * reuse formatting func in cli * normalize alternatives: most of the code used intended plural but some and most data/tests used the singular * update schemas and tests Co-authored-by: Matt Davis <6775756+nitzmahone@users.noreply.github.com> Co-authored-by: Felix Fontein --- changelogs/fragments/getoffmylawn.yml | 2 + lib/ansible/cli/doc.py | 19 ++--- lib/ansible/config/base.yml | 33 +++++++- lib/ansible/config/manager.py | 14 ++++ lib/ansible/constants.py | 31 +++++-- lib/ansible/plugins/__init__.py | 2 + test/integration/targets/ansible-doc/test.yml | 2 +- test/integration/targets/deprecations/aliases | 2 + .../deprecations/cache_plugins/notjsonfile.py | 82 +++++++++++++++++++ .../deprecations/entry_key_deprecated.cfg | 2 + .../deprecations/entry_key_deprecated2.cfg | 3 + .../deprecations/entry_key_not_deprecated.cfg | 2 + .../deprecations/library/removeoption.py | 78 ++++++++++++++++++ .../deprecations/library/willremove.py | 79 ++++++++++++++++++ .../integration/targets/deprecations/runme.sh | 39 +++++++++ .../validate-modules/validate_modules/main.py | 2 +- .../validate_modules/schema.py | 32 ++++++-- 17 files changed, 401 insertions(+), 23 deletions(-) create mode 100644 changelogs/fragments/getoffmylawn.yml create mode 100644 test/integration/targets/deprecations/aliases create mode 100644 test/integration/targets/deprecations/cache_plugins/notjsonfile.py create mode 100644 test/integration/targets/deprecations/entry_key_deprecated.cfg create mode 100644 test/integration/targets/deprecations/entry_key_deprecated2.cfg create mode 100644 test/integration/targets/deprecations/entry_key_not_deprecated.cfg create mode 100644 test/integration/targets/deprecations/library/removeoption.py create mode 100644 test/integration/targets/deprecations/library/willremove.py create mode 100755 test/integration/targets/deprecations/runme.sh diff --git a/changelogs/fragments/getoffmylawn.yml b/changelogs/fragments/getoffmylawn.yml new file mode 100644 index 00000000000..1cc805c1798 --- /dev/null +++ b/changelogs/fragments/getoffmylawn.yml @@ -0,0 +1,2 @@ +minor_changes: + - plugins, deprecations and warnings concerning configuration are now displayed to the user, technical issue that prevented 'de-duplication' have been resolved. diff --git a/lib/ansible/cli/doc.py b/lib/ansible/cli/doc.py index 44fe39a597f..4d9dfbe57d2 100755 --- a/lib/ansible/cli/doc.py +++ b/lib/ansible/cli/doc.py @@ -1089,7 +1089,7 @@ class DocCLI(CLI, RoleMixin): text = DocCLI.get_man_text(doc, collection_name, plugin_type) except Exception as e: display.vvv(traceback.format_exc()) - raise AnsibleError("Unable to retrieve documentation from '%s' due to: %s" % (plugin, to_native(e)), orig_exc=e) + raise AnsibleError("Unable to retrieve documentation from '%s'" % (plugin), orig_exc=e) return text @@ -1387,16 +1387,15 @@ class DocCLI(CLI, RoleMixin): if doc.get('deprecated', False): text.append(_format("DEPRECATED: ", 'bold', 'DEP')) if isinstance(doc['deprecated'], dict): - if 'removed_at_date' in doc['deprecated']: - text.append( - "\tReason: %(why)s\n\tWill be removed in a release after %(removed_at_date)s\n\tAlternatives: %(alternative)s" % doc.pop('deprecated') - ) - else: - if 'version' in doc['deprecated'] and 'removed_in' not in doc['deprecated']: - doc['deprecated']['removed_in'] = doc['deprecated']['version'] - text.append("\tReason: %(why)s\n\tWill be removed in: Ansible %(removed_in)s\n\tAlternatives: %(alternative)s" % doc.pop('deprecated')) + if 'removed_at_date' not in doc['deprecated'] and 'version' in doc['deprecated'] and 'removed_in' not in doc['deprecated']: + doc['deprecated']['removed_in'] = doc['deprecated']['version'] + try: + text.append('\t' + C.config.get_deprecated_msg_from_config(doc['deprecated'], True)) + except KeyError as e: + raise AnsibleError("Invalid deprecation documentation structure", orig_exc=e) else: - text.append("%s" % doc.pop('deprecated')) + text.append("%s" % doc['deprecated']) + del doc['deprecated'] if doc.pop('has_action', False): text.append("") diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml index 5c1f36225eb..1c79bfa6c1f 100644 --- a/lib/ansible/config/base.yml +++ b/lib/ansible/config/base.yml @@ -2116,4 +2116,35 @@ VERBOSE_TO_STDERR: - section: defaults key: verbose_to_stderr type: bool -... +_Z_TEST_ENTRY: + name: testentry + description: for tests + env: + - name: ANSIBLE_TEST_ENTRY + - name: ANSIBLE_TEST_ENTRY_D + deprecated: + why: for testing + version: '3.30' + alternatives: nothing + ini: + - section: testing + key: valid + - section: testing + key: deprecated + deprecated: + why: for testing + version: '3.30' + alternatives: nothing +_Z_TEST_ENTRY_2: + version_added: '2.18' + name: testentry + description: for tests + deprecated: + why: for testing + version: '3.30' + alternatives: nothing + env: + - name: ANSIBLE_TEST_ENTRY2 + ini: + - section: testing + key: valid2 diff --git a/lib/ansible/config/manager.py b/lib/ansible/config/manager.py index cd674cfb32c..5f93820548a 100644 --- a/lib/ansible/config/manager.py +++ b/lib/ansible/config/manager.py @@ -672,3 +672,17 @@ class ConfigManager(object): self._plugins[plugin_type] = {} self._plugins[plugin_type][name] = defs + + @staticmethod + def get_deprecated_msg_from_config(dep_docs, include_removal=False): + + removal = '' + if include_removal: + if 'removed_at_date' in dep_docs: + removal = f"Will be removed in a release after {dep_docs['removed_at_date']}\n\t" + else: + removal = f"Will be removed in: Ansible {dep_docs['removed_in']}\n\t" + + # TODO: choose to deprecate either singular or plural + alt = dep_docs.get('alternatives', dep_docs.get('alternative', '')) + return f"Reason: {dep_docs['why']}\n\t{removal}Alternatives: {alt}" diff --git a/lib/ansible/constants.py b/lib/ansible/constants.py index 42b1b1c7bd7..5e5799c1326 100644 --- a/lib/ansible/constants.py +++ b/lib/ansible/constants.py @@ -15,6 +15,10 @@ from ansible.module_utils.parsing.convert_bool import BOOLEANS_TRUE from ansible.release import __version__ from ansible.utils.fqcn import add_internal_fqcns +# initialize config manager/config data to read/store global settings +# and generate 'pseudo constants' for app consumption. +config = ConfigManager() + def _warning(msg): ''' display is not guaranteed here, nor it being the full class, but try anyways, fallback to sys.stderr.write ''' @@ -36,6 +40,26 @@ def _deprecated(msg, version): sys.stderr.write(' [DEPRECATED] %s, to be removed in %s\n' % (msg, version)) +def handle_config_noise(display=None): + + if display is not None: + w = display.warning + d = display.deprecated + else: + w = _warning + d = _deprecated + + while config.WARNINGS: + warn = config.WARNINGS.pop(0) + w(warn) + + while config.DEPRECATED: + # tuple with name and options + dep = config.DEPRECATED.pop(0) + msg = config.get_deprecated_msg_from_config(dep[1]) + d(msg, version=dep[1]['version']) + + def set_constant(name, value, export=vars()): ''' sets constants and returns resolved options dict ''' export[name] = value @@ -218,11 +242,8 @@ MAGIC_VARIABLE_MAPPING = dict( ) # POPULATE SETTINGS FROM CONFIG ### -config = ConfigManager() - -# Generate constants from config for setting in config.get_configuration_definitions(): set_constant(setting, config.get_config_value(setting, variables=vars())) -for warn in config.WARNINGS: - _warning(warn) +# emit any warnings or deprecations +handle_config_noise() diff --git a/lib/ansible/plugins/__init__.py b/lib/ansible/plugins/__init__.py index c083dee93e8..63d087b0806 100644 --- a/lib/ansible/plugins/__init__.py +++ b/lib/ansible/plugins/__init__.py @@ -92,6 +92,7 @@ class AnsiblePlugin(ABC): def set_option(self, option, value): self._options[option] = C.config.get_config_value(option, plugin_type=self.plugin_type, plugin_name=self._load_name, direct={option: value}) + C.handle_config_noise(display) def set_options(self, task_keys=None, var_options=None, direct=None): ''' @@ -108,6 +109,7 @@ class AnsiblePlugin(ABC): if self.allow_extras and var_options and '_extras' in var_options: # these are largely unvalidated passthroughs, either plugin or underlying API will validate self._options['_extras'] = var_options['_extras'] + C.handle_config_noise(display) def has_option(self, option): if not self._options: diff --git a/test/integration/targets/ansible-doc/test.yml b/test/integration/targets/ansible-doc/test.yml index f981401d652..0c3dcc0c22b 100644 --- a/test/integration/targets/ansible-doc/test.yml +++ b/test/integration/targets/ansible-doc/test.yml @@ -18,7 +18,7 @@ that: - result is failed - | - "ERROR! Unable to retrieve documentation from 'test_docs_missing_description' due to: All (sub-)options and return values must have a 'description' field" + "ERROR! Unable to retrieve documentation from 'test_docs_missing_description'. All (sub-)options and return values must have a 'description' field" in result.stderr - name: module with suboptions (avoid first line as it has full path) diff --git a/test/integration/targets/deprecations/aliases b/test/integration/targets/deprecations/aliases new file mode 100644 index 00000000000..8278ec8bcc7 --- /dev/null +++ b/test/integration/targets/deprecations/aliases @@ -0,0 +1,2 @@ +shippable/posix/group3 +context/controller diff --git a/test/integration/targets/deprecations/cache_plugins/notjsonfile.py b/test/integration/targets/deprecations/cache_plugins/notjsonfile.py new file mode 100644 index 00000000000..dfa20158f71 --- /dev/null +++ b/test/integration/targets/deprecations/cache_plugins/notjsonfile.py @@ -0,0 +1,82 @@ +# (c) 2020 Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import annotations + +DOCUMENTATION = ''' + cache: notjsonfile + short_description: NotJSON cache plugin + description: This cache uses is NOT JSON + author: Ansible Core (@ansible-core) + version_added: 0.7.0 + options: + _uri: + required: True + description: + - Path in which the cache plugin will save the JSON files + env: + - name: ANSIBLE_CACHE_PLUGIN_CONNECTION + version_added: 1.2.0 + ini: + - key: fact_caching_connection + section: notjsonfile_cache + - key: fact_caching_connection + section: defaults + _prefix: + description: User defined prefix to use when creating the JSON files + env: + - name: ANSIBLE_CACHE_PLUGIN_PREFIX + version_added: 1.1.0 + ini: + - key: fact_caching_prefix + section: defaults + - key: fact_caching_prefix + section: notjson_cache + deprecated: + alternative: section is notjsonfile_cache + why: Another test deprecation + removed_at_date: '2050-01-01' + - key: fact_caching_prefix + section: notjsonfile_cache + _timeout: + default: 86400 + description: Expiration timeout for the cache plugin data + env: + - name: ANSIBLE_CACHE_PLUGIN_TIMEOUT + - name: ANSIBLE_NOTJSON_CACHE_PLUGIN_TIMEOUT + deprecated: + alternative: do not use a variable + why: Test deprecation + version: '3.0.0' + ini: + - key: fact_caching_timeout + section: defaults + - key: fact_caching_timeout + section: notjsonfile_cache + vars: + - name: notsjonfile_fact_caching_timeout + version_added: 1.5.0 + type: integer + removeme: + default: 86400 + description: Expiration timeout for the cache plugin data + deprecated: + alternative: cause i need to test it + why: Test deprecation + version: '2.0.0' + env: + - name: ANSIBLE_NOTJSON_CACHE_PLUGIN_REMOVEME +''' + +from ansible.plugins.cache import BaseFileCacheModule + + +class CacheModule(BaseFileCacheModule): + """ + A caching module backed by json files. + """ + def _dump(self): + pass + + def _load(self): + pass diff --git a/test/integration/targets/deprecations/entry_key_deprecated.cfg b/test/integration/targets/deprecations/entry_key_deprecated.cfg new file mode 100644 index 00000000000..2a49bb8d720 --- /dev/null +++ b/test/integration/targets/deprecations/entry_key_deprecated.cfg @@ -0,0 +1,2 @@ +[testing] +deprecated=false diff --git a/test/integration/targets/deprecations/entry_key_deprecated2.cfg b/test/integration/targets/deprecations/entry_key_deprecated2.cfg new file mode 100644 index 00000000000..02798c90565 --- /dev/null +++ b/test/integration/targets/deprecations/entry_key_deprecated2.cfg @@ -0,0 +1,3 @@ +[testing] +# ini key not deprecated, but parent setting is +valid2=true diff --git a/test/integration/targets/deprecations/entry_key_not_deprecated.cfg b/test/integration/targets/deprecations/entry_key_not_deprecated.cfg new file mode 100644 index 00000000000..53f2b1369c9 --- /dev/null +++ b/test/integration/targets/deprecations/entry_key_not_deprecated.cfg @@ -0,0 +1,2 @@ +[testing] +valid=false diff --git a/test/integration/targets/deprecations/library/removeoption.py b/test/integration/targets/deprecations/library/removeoption.py new file mode 100644 index 00000000000..9f08792fcd8 --- /dev/null +++ b/test/integration/targets/deprecations/library/removeoption.py @@ -0,0 +1,78 @@ +# -*- coding: utf-8 -*- +# Copyright: Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import annotations + + +DOCUMENTATION = r''' +--- +module: removeoption +short_description: noop +description: does nothing, test for removal of option +options: + one: + description: + - first option + type: bool + default: no + two: + description: + - second option + deprecated: + removed_in: '3.30' + why: cause i wanna test this! + alternatives: none needed +notes: +- Just noop to test module deprecation +seealso: +- module: willremove +author: +- Ansible Core Team +attributes: + action: + support: full + async: + support: full + bypass_host_loop: + support: none + check_mode: + support: full + diff_mode: + support: none + platform: + platforms: all +''' + +EXAMPLES = r''' +- name: useless + remove_option: + one: true + two: /etc/file.conf +''' + +RETURN = r''' +''' + +from ansible.module_utils.basic import AnsibleModule + + +def main(): + module = AnsibleModule( + argument_spec=dict( + one=dict(type='bool', default='no'), + two=dict(type='str', removed_in_version='3.30'), + ), + supports_check_mode=True + ) + + one = module.params['one'] + two = module.params['two'] + + result = {'yolo': 'lola'} + + module.exit_json(**result) + + +if __name__ == '__main__': + main() diff --git a/test/integration/targets/deprecations/library/willremove.py b/test/integration/targets/deprecations/library/willremove.py new file mode 100644 index 00000000000..0c5810d8501 --- /dev/null +++ b/test/integration/targets/deprecations/library/willremove.py @@ -0,0 +1,79 @@ +# -*- coding: utf-8 -*- +# Copyright: Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import annotations + + +DOCUMENTATION = r''' +--- +module: willremove +version_added: histerical +short_description: does nothing +description: does nothing, this is deprecation test +deprecated: + removed_in: '3.30' + why: cause i wanna! + alternatives: no soup for you! +options: + one: + description: + - first option + type: bool + default: no + two: + description: + - second option +notes: +- Just noop to test module deprecation +seealso: +- module: removeoption +author: +- Ansible Core Team +attributes: + action: + support: full + async: + support: full + bypass_host_loop: + support: none + check_mode: + support: full + diff_mode: + support: none + platform: + platforms: all +''' + +EXAMPLES = r''' +- name: useless + willremove: + one: true + two: /etc/file.conf +''' + +RETURN = r''' +''' + +from ansible.module_utils.basic import AnsibleModule + + +def main(): + module = AnsibleModule( + argument_spec=dict( + one=dict(type='bool', default='no'), + two=dict(type='str'), + ), + supports_check_mode=True + ) + + one = module.params['one'] + two = module.params['two'] + + result = {'yolo': 'lola'} + + module.exit_json(**result) + + +if __name__ == '__main__': + main() diff --git a/test/integration/targets/deprecations/runme.sh b/test/integration/targets/deprecations/runme.sh new file mode 100755 index 00000000000..f16d4937a7d --- /dev/null +++ b/test/integration/targets/deprecations/runme.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash + +set -eux -o pipefail + +export ANSIBLE_DEPRECATION_WARNINGS=True + +### check general config + +# check for entry key valid, no deprecation +[ "$(ANSIBLE_CONFIG='entry_key_not_deprecated.cfg' ansible -m meta -a 'noop' localhost 2>&1 | grep -c 'DEPRECATION')" -eq "0" ] + +# check for entry key deprecation, must be defined to trigger +[ "$(ANSIBLE_CONFIG='entry_key_deprecated.cfg' ansible -m meta -a 'noop' localhost 2>&1 | grep -c 'DEPRECATION')" -eq "1" ] + +# check for deprecation of entry itself, must be consumed to trigger +[ "$(ANSIBLE_TEST_ENTRY2=1 ansible -m debug -a 'msg={{q("config", "_Z_TEST_ENTRY_2")}}' localhost 2>&1 | grep -c 'DEPRECATION')" -eq "1" ] + +# check for entry deprecation, just need key defined to trigger +[ "$(ANSIBLE_CONFIG='entry_key_deprecated2.cfg' ansible -m meta -a 'noop' localhost 2>&1 | grep -c 'DEPRECATION')" -eq "1" ] + + +### check plugin config + +# force use of the test plugin +export ANSIBLE_CACHE_PLUGIN_CONNECTION=/var/tmp +export ANSIBLE_CACHE_PLUGIN=notjsonfile + +# check for plugin(s) config option and setting non deprecation +[ "$(ANSIBLE_CACHE_PLUGIN_TIMEOUT=1 ansible -m meta -a 'noop' localhost --playbook-dir ./ 2>&1 | grep -c 'DEPRECATION')" -eq "0" ] + +# check for plugin(s) config option setting deprecation +[ "$(ANSIBLE_NOTJSON_CACHE_PLUGIN_TIMEOUT=1 ansible -m meta -a 'noop' localhost --playbook-dir ./ 2>&1 | grep -c 'DEPRECATION')" -eq "1" ] + +# check for plugin(s) config option deprecation +[ "$(ANSIBLE_NOTJSON_CACHE_PLUGIN_REMOVEME=1 ansible -m meta -a 'noop' localhost --playbook-dir ./ 2>&1 | grep -c 'DEPRECATION')" -eq "1" ] + +# TODO: check for module deprecation +# TODO: check for module option deprecation +# TODO: check for plugin deprecation diff --git a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py index 5e3a07e33b6..ddfb8ca72d2 100644 --- a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py +++ b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py @@ -1235,7 +1235,7 @@ class ModuleValidator(Validator): self._validate_semantic_markup(entry.get(key)) if isinstance(docs.get('deprecated'), dict): - for key in ('why', 'alternative'): + for key in ('why', 'alternative', 'alternatives'): self._validate_semantic_markup(docs.get('deprecated').get(key)) self._validate_semantic_markup_options(docs.get('options')) diff --git a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py index ba4e1883fb3..a7cc666ed99 100644 --- a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py +++ b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py @@ -84,6 +84,22 @@ def date(error_code=None): return Any(isodate, error_code=error_code) +def require_only_one(keys): + def f(obj): + found = None + for k in obj.keys(): + if k in keys: + if k is None: + found = k + else: + raise Invalid('Found conflicting keys, must contain only one of {}'.format(keys)) + if found is None: + raise Invalid('Must contain one of {}'.format(keys)) + + return obj + return f + + # Roles can also be referenced by semantic markup _VALID_PLUGIN_TYPES = set(DOCUMENTABLE_PLUGINS + ('role', )) @@ -568,7 +584,9 @@ def list_dict_option_schema(for_collection, plugin_type): { # This definition makes sure everything has the correct types/values 'why': doc_string, - 'alternatives': doc_string, + # TODO: phase out either plural or singular, 'alt' is exclusive group + Exclusive('alternative', 'alt'): doc_string, + Exclusive('alternatives', 'alt'): doc_string, # vod stands for 'version or date'; this is the name of the exclusive group Exclusive('removed_at_date', 'vod'): date(), Exclusive('version', 'vod'): version(for_collection), @@ -577,7 +595,7 @@ def list_dict_option_schema(for_collection, plugin_type): { # This definition makes sure that everything we require is there Required('why'): Any(*string_types), - 'alternatives': Any(*string_types), + Required(Any('alternatives', 'alternative')): Any(*string_types), Required(Any('removed_at_date', 'version')): Any(*string_types), Required('collection_name'): Any(*string_types), }, @@ -761,13 +779,16 @@ def return_schema(for_collection, plugin_type='module'): def deprecation_schema(for_collection): + main_fields = { Required('why'): doc_string, - Required('alternative'): doc_string, - Required('removed_from_collection'): collection_name, - 'removed': Any(True), + 'alternative': doc_string, + 'alternatives': doc_string, } + if for_collection: + main_fields.update({Required('removed_from_collection'): collection_name, 'removed': Any(True)}) + date_schema = { Required('removed_at_date'): date(), } @@ -791,6 +812,7 @@ def deprecation_schema(for_collection): if for_collection: result = All( result, + require_only_one(['alternative', 'alternatives']), partial(check_removal_version, version_field='removed_in', collection_name_field='removed_from_collection',