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

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Jan 23, 2024

Now deduped globally

ISSUE TYPE
  • Bugfix Pull Request

@ansibot ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 23, 2024
@bcoca
Copy link
Member Author

bcoca commented Jan 25, 2024

cc @briantist

@ansible ansible deleted a comment from ansibot Jan 25, 2024
@ansibot ansibot removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. ci_verified Changes made in this PR are causing tests to fail. labels Jan 25, 2024
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Jan 30, 2024
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jan 30, 2024
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed ci_verified Changes made in this PR are causing tests to fail. labels Jan 30, 2024
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jan 31, 2024
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 1, 2024
@bcoca bcoca force-pushed the fix_deprecations branch 2 times, most recently from 3e441e4 to 7824434 Compare February 1, 2024 18:51
@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Feb 1, 2024
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 1, 2024
@bcoca bcoca marked this pull request as ready for review February 5, 2024 13:59
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

A few suggestions

lib/ansible/cli/doc.py Show resolved Hide resolved
lib/ansible/cli/doc.py Show resolved Hide resolved
lib/ansible/cli/doc.py Show resolved Hide resolved
lib/ansible/config/manager.py Outdated Show resolved Hide resolved
lib/ansible/constants.py Show resolved Hide resolved
lib/ansible/constants.py Show resolved Hide resolved
lib/ansible/constants.py Outdated Show resolved Hide resolved
s-hertel

This comment was marked as resolved.

@bcoca
Copy link
Member Author

bcoca commented Feb 6, 2024

did I misread the standard, is alternatives wrong then? or did we just mix both up to the point no one can tell?
I'll take a look at it when I get back, had not meant to resolve the whole issue, I was still mulling on what to do with singular/plural issue.

@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 Feb 20, 2024
@ansibot ansibot removed 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 Apr 5, 2024
@bcoca
Copy link
Member Author

bcoca commented Apr 5, 2024

@s-hertel updated to accept 'both ways' .. we can punt on standardizing on one or the other for now.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 5, 2024
@ansible ansible deleted a comment from ansibot Apr 5, 2024
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 5, 2024
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. 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. labels Apr 9, 2024
@bcoca
Copy link
Member Author

bcoca commented Apr 10, 2024

@briantist @felixfontein , would like to merge this soon, comments?

changelogs/fragments/getoffmylawn.yml Outdated Show resolved Hide resolved
},
Exclusive('singular', 'alt', msg=alt_msg): {
Required('alternative'): doc_string
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The above does not work. It rejects both alternatives and alternative with errors like extra keys not allowed @ data['deprecated']['alternative'].. Basically doing what you want isn't easily supported with voluptuous (see alecthomas/voluptuous#115), adding your own validator to make sure one of them is there is probably the best (similar to alecthomas/voluptuous#115 (comment)).

Try running this PR against a module that is deprecated (you can find some examples in community.general for example). (Would be good to have a test for that in ansible-core's integration tests for this as well.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We had very few deprecation message tests, the file above adds a bunch but i still had some 'todo' cases
https://github.com/ansible/ansible/pull/82593/files#diff-a06276ae256ddf23396903d5813205f1ed799ab8e5f687c5adc87de6a4233863R37

Copy link
Member Author

Choose a reason for hiding this comment

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

surprised this does not work, since it took it from the docs example
http://alecthomas.github.io/voluptuous/docs/_build/html/voluptuous.html#module-voluptuous.schema_builder

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you refer to:

msg = 'Please, use only one type of authentication at the same time.'
schema = Schema({
Exclusive('classic', 'auth', msg=msg):{
    Required('email'): str,
    Required('password'): str
    },
Exclusive('internal', 'auth', msg=msg):{
    Required('secret_key'): str
    },
Exclusive('social', 'auth', msg=msg):{
    Required('social_network'): str,
    Required('token'): str
    }
})

The example shows something else, as you can see from the code fragment which uses it:

with raises(er.MultipleInvalid, "Please, use only one type of authentication at the same time. @ data[<auth>]"):
    schema({'classic': {'email': 'foo@example.com', 'password': 'bar'},
            'social': {'social_network': 'barfoo', 'token': 'tEMp'}})

There you can see that classic and social are sub-dictionaries, and email/password/... are not top-level keys.

@briantist
Copy link
Contributor

would like to merge this soon, comments?

Looked over the code, no specific comments, but very much looking forward to this

@ansibot ansibot added 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. labels Apr 10, 2024
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.

@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 Apr 22, 2024
bcoca and others added 11 commits April 26, 2024 12:39
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

Co-authored-by: Matt Davis <6775756+nitzmahone@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>
@ansibot ansibot removed 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 Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug. stale_review Updates were made after the last review and the last review is more than 7 days old.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants