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.

Sign up for GitHub

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

Jump to bottom

module sanity checks: improve alias handling #59060

Merged
merged 5 commits into from Nov 21, 2019
Merged

module sanity checks: improve alias handling #59060

merged 5 commits into from Nov 21, 2019

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Jul 13, 2019 •
edited

SUMMARY

This improves module sanity checking with regard to option aliases:

  1. Reports an error if the option name is mentioned as an alias, or an alias is mentioned twice;
  2. Reports if an option is documented multiple times (by different aliases);
  3. Updates the type / default / ... sanity checks for options so that if an alias is documented (instead of the option itself), the docs for this alias will be sanity-checked (and not the non-existing documentation for the option's real name).

This is WIP since I want #58646 to get merged first. This will improve the checks done in there, and will be another big update to ignore.txt (which I haven't done yet because it will need to be redone anyway once #58646 is merged).

CC @bcoca @sivel

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME

test/sanity/validate-modules/main.py

@ansibot
Copy link
Contributor

ansibot commented Jul 13, 2019

cc @gundalow @mattclay
click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Jul 13, 2019
@ansibot

This comment has been minimized.

Sign in to view
@felixfontein
Copy link
Contributor Author

(The failing sanity tests are intentional, I will start with updating ignore.txt once #58646 is merged and this is rebased.)

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jul 16, 2019
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 24, 2019
@felixfontein felixfontein force-pushed the sanity-aliases branch 2 times, most recently from e1cf9e1 to e23f7bf Compare August 1, 2019 20:39
@ansibot
Copy link
Contributor

ansibot commented Aug 1, 2019

cc @acozine
click here for bot help

@ansibot ansibot added docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. small_patch and removed 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 Aug 1, 2019
@felixfontein
Copy link
Contributor Author

(Updated ignore.txt with https://gist.github.com/felixfontein/219b19f8c39b7755b4482e3c28212d7e)

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Aug 2, 2019
@felixfontein felixfontein mentioned this pull request Aug 6, 2019
Closed
@ansibot ansibot added has_issue and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Aug 6, 2019
@felixfontein felixfontein changed the title [WIP] module sanity checks: improve alias handling module sanity checks: improve alias handling Aug 6, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Aug 6, 2019
@ansibot ansibot added 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. labels Aug 8, 2019
@felixfontein
Copy link
Contributor Author

felixfontein commented Aug 13, 2019 •
edited

TODO:

  • consolidate with 322 / 323 errors
  • check for same alias used by multiple options

@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 Aug 21, 2019
@gundalow
Copy link
Contributor

gundalow commented Sep 3, 2019

Just seen this for the first time, looks good :)

@felixfontein
Copy link
Contributor Author

(Removed conflicts, updated ignore.txt. Haven't touched TODOs yet.)

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed 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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 16, 2019
@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed small_patch labels Nov 17, 2019
@felixfontein
Copy link
Contributor Author

I've reworked the PR according to the discussion (search for 59060 in https://meetbot.fedoraproject.org/ansible-meeting/2019-08-13/ansible_core_public_irc_meeting.2019-08-13-19.00.log.html). I think it should be OK now.

@felixfontein felixfontein mentioned this pull request Nov 18, 2019
Closed
samdoran
samdoran approved these changes Nov 21, 2019
View reviewed changes
Copy link
Contributor

@samdoran samdoran left a comment

Choose a reason for hiding this comment

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

Tested this out and it works as advertised.

@samdoran samdoran merged commit 784e507 into ansible:devel Nov 21, 2019
@felixfontein felixfontein deleted the sanity-aliases branch November 22, 2019 06:51
@felixfontein
Copy link
Contributor Author

Thanks a lot to everyone who helped reviewing this and commented on this (here and during the meetings)! :)

@felixfontein felixfontein mentioned this pull request Nov 22, 2019
Closed
D3DeFi pushed a commit to D3DeFi/ansible that referenced this pull request Dec 8, 2019
@felixfontein @D3DeFi
module sanity checks: improve alias handling (ansible#59060)
e975ccf 
* add_file_common_args is only of interest on top-level.
* Handle undocumented arguments in one place.
* Update ignore.txt
* Add changelog
anshulbehl pushed a commit to anshulbehl/ansible that referenced this pull request Dec 10, 2019
@felixfontein @anshulbehl
module sanity checks: improve alias handling (ansible#59060)
0708abf 
* add_file_common_args is only of interest on top-level.
* Handle undocumented arguments in one place.
* Update ignore.txt
* Add changelog
@ansible ansible locked and limited conversation to collaborators Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samdoran samdoran samdoran approved these changes

Assignees
No one assigned
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. has_issue support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@felixfontein @ansibot @gundalow @samdoran @bcoca