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

Make roles generation compatible with ansible-lint autofix feature #82529

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
+19 −19
Open

Make roles generation compatible with ansible-lint autofix feature #82529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions lib/ansible/galaxy/data/default/role/meta/main.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ galaxy_info:
# - 99.99

galaxy_tags: []
# List tags for your role here, one per line. A tag is a keyword that describes
# and categorizes the role. Users find roles by searching for tags. Be sure to
# remove the '[]' above, if you add tags to this list.
#
# NOTE: A tag is limited to a single word comprised of alphanumeric characters.
# Maximum 20 tags per role.
# List tags for your role here, one per line. A tag is a keyword that describes
# and categorizes the role. Users find roles by searching for tags. Be sure to
# remove the '[]' above, if you add tags to this list.
#
# NOTE: A tag is limited to a single word comprised of alphanumeric characters.
# Maximum 20 tags per role.

dependencies: []
# List your role dependencies here, one per line. Be sure to remove the '[]' above,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like these lines should also be corrected? It doesn't affect anything because there is nothing after, but I'm guessing the formatting gets changed here too. If we're going to correct it, we should correct all places that get reformatted.

Copy link
Author

Choose a reason for hiding this comment

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

@sivel, Thanks for the review. I double-checked the behavior of comments in "dependencies" section. It does not get reformatted while using ansible-lint --fix.

Copy link
Member

Choose a reason for hiding this comment

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

Is the formatting arbitrary, then? It doesn't feel right accepting a change that introduces such an inconsistency.

Copy link
Contributor

@s-hertel s-hertel Jan 17, 2024 •
edited

Choose a reason for hiding this comment

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

@shatakshiiii From my testing, if something follows the comment it is also unindented. I think it should look visually-consistent, since the one-off change by the linter a bit unfriendly to usability. Another change ansible-lint --fix makes is add --- to the top of all the YAML files, why is the indentation changing but not that?

Copy link
Member

Choose a reason for hiding this comment

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

The underlying problem with all of this is that lint is not really formatting-aware, but reformatting sometimes occurs as a side-effect of round-tripping the document through ruamel (which invents its own rules to tie YAML comments to YAML blocks and indents them to match). IIRC its general rule is that a comment on a non-content line is tied to the next content element below it and indented to match. The comment on this line has no content element below it (since it's EOF), so it's just left alone.

Changing these to follow a general rule of "comments always above content" should make that behavior more consistent and predictable- I think that's preferable to, eg, adding bogus empty block lists that have to be removed, just to keep the "below the element" comment formatting.

The doc-start marker is another problem case- IIRC ruamel doesn't have a "re-emit if present" knob for that, so it's either always going to remove it or always going to add it, so if lint's not shutting that off, then yeah, the template should also include that.

Longer-term, I'm still not sure lint's going to be able to get away without having some standard wrappers or customization around ruamel's round-trip reformatting.

Copy link

Choose a reason for hiding this comment

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

For consistency, having the explanation about a given key above that key, as several are, sounds reasonable to me.

Expand Down
12 changes: 6 additions & 6 deletions lib/ansible/galaxy/data/network/meta/main.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ galaxy_info:
# - 99.99

galaxy_tags: []
# List tags for your role here, one per line. A tag is a keyword that describes
# and categorizes the role. Users find roles by searching for tags. Be sure to
# remove the '[]' above, if you add tags to this list.
#
# NOTE: A tag is limited to a single word comprised of alphanumeric characters.
# Maximum 20 tags per role.
# List tags for your role here, one per line. A tag is a keyword that describes
# and categorizes the role. Users find roles by searching for tags. Be sure to
# remove the '[]' above, if you add tags to this list.
#
# NOTE: A tag is limited to a single word comprised of alphanumeric characters.
# Maximum 20 tags per role.

dependencies: []
# List your role dependencies here, one per line. Be sure to remove the '[]' above,
Expand Down
14 changes: 7 additions & 7 deletions test/units/cli/test_data/role_skeleton/meta/main.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ galaxy_info:
# - 99.99

galaxy_tags: []
# List tags for your role here, one per line. A tag is
# a keyword that describes and categorizes the role.
# Users find roles by searching for tags. Be sure to
# remove the '[]' above if you add tags to this list.
#
# NOTE: A tag is limited to a single word comprised of
# alphanumeric characters. Maximum 20 tags per role.
# List tags for your role here, one per line. A tag is
# a keyword that describes and categorizes the role.
# Users find roles by searching for tags. Be sure to
# remove the '[]' above if you add tags to this list.
#
# NOTE: A tag is limited to a single word comprised of
# alphanumeric characters. Maximum 20 tags per role.

dependencies: []
# List your role dependencies here, one per line.
Expand Down