Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

actually show plugin config warnings/deprecations #82593

Open
wants to merge 11 commits into
base: devel
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions 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.
19 changes: 9 additions & 10 deletions lib/ansible/cli/doc.py
Expand Up @@ -1083,7 +1083,7 @@ def format_plugin_doc(plugin, plugin_type, doc, plainexamples, returndocs, metad
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)
bcoca marked this conversation as resolved.
Show resolved Hide resolved

return text

Expand Down Expand Up @@ -1368,16 +1368,15 @@ def get_man_text(doc, collection_name='', plugin_type=''):
if doc.get('deprecated', False):
bcoca marked this conversation as resolved.
Show resolved Hide resolved
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))
bcoca marked this conversation as resolved.
Show resolved Hide resolved
except KeyError as e:
raise AnsibleError("Invalid deprecation documentation structure", orig_exc=e)
bcoca marked this conversation as resolved.
Show resolved Hide resolved
else:
text.append("%s" % doc.pop('deprecated'))
text.append("%s" % doc['deprecated'])
del doc['deprecated']

if doc.pop('has_action', False):
text.append("")
Expand Down
33 changes: 32 additions & 1 deletion lib/ansible/config/base.yml
Expand Up @@ -2052,4 +2052,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
14 changes: 14 additions & 0 deletions lib/ansible/config/manager.py
Expand Up @@ -617,3 +617,17 @@ def initialize_plugin_configuration_definitions(self, plugin_type, name, defs):
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}"
31 changes: 26 additions & 5 deletions lib/ansible/constants.py
Expand Up @@ -15,6 +15,10 @@
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 '''
Expand All @@ -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
bcoca marked this conversation as resolved.
Show resolved Hide resolved
d = display.deprecated
else:
w = _warning
bcoca marked this conversation as resolved.
Show resolved Hide resolved
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])
bcoca marked this conversation as resolved.
Show resolved Hide resolved
d(msg, version=dep[1]['version'])


def set_constant(name, value, export=vars()):
''' sets constants and returns resolved options dict '''
export[name] = value
Expand Down Expand Up @@ -218,11 +242,8 @@ def __getitem__(self, y):
)

# 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()
2 changes: 2 additions & 0 deletions lib/ansible/plugins/__init__.py
Expand Up @@ -92,6 +92,7 @@ def get_options(self, hostvars=None):

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):
'''
Expand All @@ -108,6 +109,7 @@ def set_options(self, task_keys=None, var_options=None, direct=None):
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:
Expand Down
2 changes: 1 addition & 1 deletion test/integration/targets/ansible-doc/test.yml
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions test/integration/targets/deprecations/aliases
@@ -0,0 +1,2 @@
shippable/posix/group3
context/controller
82 changes: 82 additions & 0 deletions 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
@@ -0,0 +1,2 @@
[testing]
deprecated=false
@@ -0,0 +1,3 @@
[testing]
# ini key not deprecated, but parent setting is
valid2=true
@@ -0,0 +1,2 @@
[testing]
valid=false
78 changes: 78 additions & 0 deletions 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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since when is deprecated allowed for module option docs? Since this is new and intended, it should be mentioned in the changelog.

(Also schema.py in this PR is still disallowing this for plugin_type == 'module'.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were allowed since deprecations were added for docs, but I don't think it has been used more than 1 or 2 times. I'll update schema too, not sure why that was disabled there, since 'per option deprecation' existed for modules much longer than for plugins.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of the time we just used aliases to rename an option and keep old name as alias/backwards compat, I don't think we've removed an option from a module since 1.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option deprecations for modules were allowed, but documenting them with deprecated never was so far.

most of the time we just used aliases to rename an option and keep old name as alias/backwards compat, I don't think we've removed an option from a module since 1.0

I think this happened quite a lot, especially in collections, but also in ansible/ansible. Actually it even happened in ansible-core: 9f82fc2 Some removals from Ansible 2.9: 808bf02 88f0c85

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option deprecations for modules were allowed, but documenting them with deprecated never was so far.

It was, for most of Ansible's lifetime there were no restrictions on docs, mostly a few requirements. The core engine still does not implement many restrictions.

The restrictions on docs are a relatively recent development in the 12yr lifetime of the project and part of CI/site docs build, not the runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict validation of module option docs (which effectively disallowed deprecated in it) was added in 04e816e, which is now there for a bit over 7 years. That's more than half of the 12 years you mentioned.

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()