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

Role argspec: allow new argument spec file #74582

Merged
merged 11 commits into from May 11, 2021
Merged

Role argspec: allow new argument spec file #74582

merged 11 commits into from May 11, 2021

Conversation

Shrews
Copy link
Contributor

@Shrews Shrews commented May 5, 2021 •
edited

SUMMARY

Roles containing a defined argument spec in meta/main.yml do not currently work on ansible-core versions less than 2.11 because those versions do not allow for the argument_specs key in meta/main.yml. To solve this, we will allow the role arg specs to be defined in a new file (meta/argument_specs.[yml|yaml]) which will allow updated roles to still work on older versions of ansible-core.

Argument specs in meta/main.yml will still be supported, but roles choosing to use this file will work only on ansible-core >= 2.11. The new spec file will have precedence over meta/main.yml. Data will not be combined between the two files, if both exist.

This also adds a new test for collection-based roles that tests both validation success and validation failure.

Fixes #74525

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

roles

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.12 bug This issue/PR relates to a bug. 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. labels May 5, 2021
Shrews
Shrews commented May 5, 2021
View reviewed changes
lib/ansible/cli/doc.py Outdated Show resolved Hide resolved
samdoran
samdoran reviewed May 5, 2021
View reviewed changes
lib/ansible/cli/doc.py Outdated Show resolved Hide resolved
samdoran
samdoran reviewed May 5, 2021
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.

We probably needs some tests for a spec in meta/main.yml as well as a test for specs in both meta/main.yml and meta/argument_specs.yml to make sure that meta/argument_specs.yml is preferred. And maybe a test with meta/argument_specs.yaml to test the multiple file extension support.

@Shrews
Copy link
Contributor Author

Shrews commented May 6, 2021

We probably needs some tests for a spec in meta/main.yml as well as a test for specs in both meta/main.yml and meta/argument_specs.yml to make sure that meta/argument_specs.yml is preferred. And maybe a test with meta/argument_specs.yaml to test the multiple file extension support.

I had planned to add the first part. The second part is already covered by some of the file renames. I have some roles using argument_specs.yml, some using argument_specs.yaml, and some using main.yml.

@Shrews
Copy link
Contributor Author

Shrews commented May 6, 2021

We probably needs some tests for a spec in meta/main.yml as well as a test for specs in both meta/main.yml and meta/argument_specs.yml to make sure that meta/argument_specs.yml is preferred. And maybe a test with meta/argument_specs.yaml to test the multiple file extension support.

I had planned to add the first part. The second part is already covered by some of the file renames. I have some roles using argument_specs.yml, some using argument_specs.yaml, and some using main.yml.

Aaaand.... I just realized I hadn't yet pushed up that change with the remaining renames. But yeah, it's covered. :)

@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. docs This issue/PR relates to or includes documentation. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels May 6, 2021
@Shrews Shrews marked this pull request as ready for review May 6, 2021 18:22
@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 May 6, 2021
@felixfontein felixfontein mentioned this pull request May 6, 2021
Merged
@felixfontein
Copy link
Contributor

This needs a changelog fragment :)

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label May 6, 2021
Shrews
Shrews commented May 6, 2021
View reviewed changes
lib/ansible/cli/doc.py Outdated Show resolved Hide resolved
s-hertel
s-hertel reviewed May 6, 2021
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.

LGTM

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels May 6, 2021
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed shipit This PR is ready to be merged by Core labels May 7, 2021
@ansibot ansibot added collection Related to Ansible Collections work collection:ktdreyer.errata_tool_ansible labels May 7, 2021
felixfontein
felixfontein approved these changes May 8, 2021
View reviewed changes
test/integration/targets/ansible-doc/roles/test_role1/meta/main.yml
- choice1
- choice2
groot:
short_description: I am Groot
Copy link
Contributor

Choose a reason for hiding this comment

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

🤣

@sivel sivel merged commit 8fb5488 into ansible:devel May 11, 2021
@Shrews Shrews deleted the role_argspec_sep_file branch May 11, 2021 14:30
Shrews added a commit to Shrews/ansible that referenced this pull request May 11, 2021
@Shrews
Role argspec: allow new argument spec file (ansible#74582)
3112019 
* support separate role argspec file in ansible-doc

* support separate role argspec file in ansible-core

* support both .yml and .yaml extensions on argspec file in ansible-doc

* fix filename building bug and rename some argspec files to test variations

* use yaml extensions from constants

* add superfluous meta/main.yml files to tests

* Update lib/ansible/cli/doc.py

Co-authored-by: Sam Doran <sdoran@redhat.com>

* update docs

* ci_complete

* add changelog and allow for main.yml variations

* add collection role testing

Co-authored-by: Sam Doran <sdoran@redhat.com>
(cherry picked from commit 8fb5488)
relrod pushed a commit that referenced this pull request May 17, 2021
@Shrews
Role argspec: allow new argument spec file (#74582) (#74656)
fc84de8 
* support separate role argspec file in ansible-core
* support both .yml and .yaml extensions on argspec file in ansible-doc
* fix filename building bug and rename some argspec files to test variations
* use yaml extensions from constants
* add superfluous meta/main.yml files to tests
* Update lib/ansible/cli/doc.py
* update docs
* add changelog and allow for main.yml variations
* add collection role testing

Co-authored-by: Sam Doran <sdoran@redhat.com>
(cherry picked from commit 8fb5488)
@ansible ansible locked and limited conversation to collaborators Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bcoca bcoca bcoca left review comments

@samdoran samdoran samdoran left review comments

@s-hertel s-hertel s-hertel left review comments

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
affects_2.12 bug This issue/PR relates to a bug. collection:ktdreyer.errata_tool_ansible collection Related to Ansible Collections work core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ansible < 2.11 fails on roles that contain argument specification
7 participants
@Shrews @felixfontein @bcoca @samdoran @s-hertel @sivel @ansibot