Skip to content

now show plugin config runtime deprecations #77057

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

Closed
wants to merge 3 commits into from

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Feb 17, 2022

requires #77056 to avoid spamming duplicate messages

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

config

@ansibot ansibot added affects_2.13 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 17, 2022
@bcoca bcoca mentioned this pull request Feb 17, 2022
1 task
@briantist
Copy link
Contributor

It will be some time before I can test this (probably not until after it's merged, but I'll see), speaking of which should there be tests for this?

Also any chance for backports? 🙏

I have a workaround in my code for displaying deprecations, I presume that if this feature is available, then it will run before my code, and so when I check for deprecations there won't be any to display. But is there something I could check for in my code? Conditionally import or whatever to skip my deprecation code altogether?

If it helps, my code: https://github.com/ansible-collections/community.hashi_vault/blob/4a64d4cde1e8de16b91eee558d8d2d9ff3e3674f/plugins/plugin_utils/_hashi_vault_plugin.py#L50-L86

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Feb 17, 2022
@bcoca
Copy link
Member Author

bcoca commented Feb 17, 2022

@briantist backports would also depend on #77056 ,I put this together in 5 mins once i saw that as it solved the major blocker for this, so my pr 'works' but not ready to merge. I would like to tweak presentation/msg a lot more.

The issues with your code are the same as mine w/o the sivel's PR, you'll get the same message over and over every time the option is accessed (and normal flow for most plugins, man at least x2).

@briantist
Copy link
Contributor

@briantist backports would also depend on #77056 ,I put this together in 5 mins once i saw that as it solved the major blocker for this, so my pr 'works' but not ready to merge. I would like to tweak presentation/msg a lot more.

Makes sense, thanks for the explanations!

The issues with your code are the same as mine w/o the sivel's PR, you'll get the same message over and over every time the option is accessed (and normal flow for most plugins, man at least x2).

Mine doesn't get called on every option access automatically; this gets called once toward the beginning of the plugin, after set_options() is called, which seems to be sufficient to trigger the existing deprecation processing (which adds deprecations to the list but doesn't display them), on options that are set, which is sufficient for my purposes but I know there are scenarios you've laid out before where that's not always the right thing. Anyway, I only ever call the function I linked once, so there's no duplicate messages that I've ever seen; possible there are and I haven't noticed.

In any case, I look forward to an "official" fix and to get rid of this code.. that'll take a lot more time if it isn't backported, but that's ok I just have to make sure my little workaround doesn't get in the way in the versions where it's not needed.

@bcoca
Copy link
Member Author

bcoca commented Feb 21, 2022

@briantist yes, that makes 'less' noise and lookups only get called explicitly once, but this 'once' is per task/host in different forks, which means you still get the message N times, other plugins get set_options called multiple times, why we could not use your implementation as is (nor mine) for all plugins.

Why sivel's PR is crucial to avoid repetition as warnings get relayed to the main process and deduped there.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Feb 21, 2022
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Feb 22, 2022
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 22, 2022
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Mar 1, 2022
@ansibot
Copy link
Contributor

ansibot commented Mar 1, 2022

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/plugins/__init__.py:59:18: too-many-format-args: Too many arguments for format string
lib/ansible/plugins/__init__.py:60:12: collection-invalid-deprecated-version: Invalid deprecated version (<Subscript l.60 at 0x7f7ddd620d90>) found in call to Display.deprecated or AnsibleModule.deprecate

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 1, 2022
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 9, 2022
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 19, 2022
@bcoca bcoca marked this pull request as draft August 24, 2022 19:37
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Aug 24, 2022
@bcoca bcoca force-pushed the deprecatednotice branch from 0ee5d01 to fe72da1 Compare March 9, 2023 17:48
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 9, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 17, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label May 12, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 24, 2023
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 28, 2025
@bcoca
Copy link
Member Author

bcoca commented Jun 9, 2025

already works in 2.19

@bcoca bcoca closed this Jun 9, 2025
@bcoca
Copy link
Member Author

bcoca commented Jun 9, 2025

>ansible -m debug -a msg="{{q('depme', start=0, end=8, format='testuser%02x')}}" localhost --playbook-dir ./
[WARNING]: No inventory was parsed, only implicit localhost is available
[WARNING]: Deprecation warnings can be disabled by setting `deprecation_warnings=False` in ansible.cfg.
[DEPRECATION WARNING]: format option. Reason: cause i wanna
Alternatives: nope. This feature will be removed from lookup plugin 'depme' in the future.

@bcoca bcoca deleted the deprecatednotice branch June 9, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.13 bug This issue/PR relates to a bug. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_pr This PR has not been pushed to for more than one year. support:core This issue/PR relates to code supported by the Ansible Engineering Team. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants