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

Extend validate-modules to also validate plugins #71734

Merged
merged 11 commits into from
Mar 4, 2022
Merged

Extend validate-modules to also validate plugins #71734

merged 11 commits into from
Mar 4, 2022

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Sep 12, 2020 •
edited
Loading

SUMMARY

Adds a --plugin-type option to validate-modules, and modifies ansible-test to use it.

There are a lot of things left to standardize / decide, but this is a first attempt to see how it looks :)

It includes #71679 and #71735 since I think these can get merged soon, and I didn't want to have conflicts with them or wait for them...

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ansible-test validate-modules

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.11 docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. 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. labels Sep 12, 2020
@felixfontein felixfontein mentioned this pull request Sep 12, 2020
Merged
@ansibot

This comment has been minimized.

Sign in to view
@felixfontein
Copy link
Contributor Author

To discuss:

  • what's with name: next to description: in option descriptions? (see default_callback docs_fragment for extensive usage)
  • plugin types which must have / can have / cannot have examples
  • author: required with same conditions as for modules, or optional, or other conditions?
  • <plugin_type>: <name> vs. plugin_type: <plugin_type> and name: <name>
  • option type: should long forms boolean, string, integer etc. be allowed?
  • return value type: required or not for plugins?

@felixfontein felixfontein mentioned this pull request Sep 12, 2020
Closed
This was referenced Sep 12, 2020
Merged
Merged
@ansibot

This comment has been minimized.

Sign in to view
This was referenced Sep 12, 2020
Merged
Merged
@felixfontein
Copy link
Contributor Author

Another thing that can be discussed:

  • for callback plugins, type vs. callback_type - @abadger chose type in antsibull-docs, but callback_type seems to be very common as well (and is named more closely to the CALLBACK_TYPE field in the plugin itself)

@ansibot

This comment has been minimized.

Sign in to view
gundalow
gundalow reviewed Sep 13, 2020
View reviewed changes
Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

Amazing. Thank you so much for this.
Only done a quick skim so far.

I wonder if fixing the new the documentation problems this finds is something that people could help with in the next Hackathon?

test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py Outdated Show resolved Hide resolved
test/lib/ansible_test/_internal/sanity/validate_modules.py Outdated Show resolved Hide resolved
@gundalow gundalow requested a review from sivel September 13, 2020 12:49
@Shrews Shrews removed the needs_triage Needs a first human triage before being processed. label Sep 15, 2020
@patchback patchback bot mentioned this pull request Sep 16, 2020
Merged
This was referenced Sep 17, 2020
Closed
Closed
Closed
Closed
Closed
@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 Jan 30, 2022
@bcoca bcoca mentioned this pull request Feb 17, 2022
Closed
@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. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Feb 17, 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. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Feb 23, 2022
@s-hertel s-hertel mentioned this pull request Mar 3, 2022
Closed
@s-hertel s-hertel self-requested a review March 3, 2022 15:29
s-hertel
s-hertel approved these changes Mar 3, 2022
View reviewed changes
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

@felixfontein could you add a changelog?

@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 Mar 4, 2022
@felixfontein
Copy link
Contributor Author

Looks like I completely forgot about that one 😆 Now it's there, thanks!

@s-hertel s-hertel merged commit 0990c4c into ansible:devel Mar 4, 2022
@s-hertel
Copy link
Contributor

s-hertel commented Mar 4, 2022

Thanks @felixfontein for all the work on this one 😄

@felixfontein felixfontein deleted the validate-plugins branch March 4, 2022 16:24
@felixfontein
Copy link
Contributor Author

@gundalow @bcoca @mattclay @s-hertel thanks a lot for all the reviews and patience with this one :)

@felixfontein felixfontein mentioned this pull request Mar 4, 2022
Closed
@briantist briantist mentioned this pull request Mar 4, 2022
Merged
@ansible ansible locked and limited conversation to collaborators Mar 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gundalow gundalow gundalow left review comments

@bcoca bcoca bcoca left review comments

@s-hertel s-hertel s-hertel approved these changes

@mattclay mattclay mattclay approved these changes

@sivel sivel Awaiting requested review from sivel

Assignees
No one assigned
Labels
affects_2.11 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. feature This issue/PR relates to a feature request. has_issue 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.
10 participants
@felixfontein @ansibot @mattclay @Andersson007 @mkrizek @s-hertel @bcoca @gundalow @Shrews @samdoran