-
Notifications
You must be signed in to change notification settings - Fork 23.8k
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
Make roles generation compatible with ansible-lint autofix feature #82529
base: devel
Are you sure you want to change the base?
Conversation
Thank you @shatakshiiii. Regarding #82394, while I agree it is not necessarily a bug, we certainly have an opportunity here to improve the developer experience. |
I don't really have a problem with this particular change, but it does make me nervous that ansible-lint may be delegating the rule implementation that's driving it to a non-spec YAML comment roundtripping implementation that's subject to change (further discussion here). |
at this point in time, the dep packages used to reformat yaml can't really change. But I agree, it is a conversation we should keep having. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it sounds like the YAML formatting requirement is limited to "whatever ruamel says", and I know from experience that's pretty difficult to change everywhere, we should probably just merge this one for internal consistency.
(assuming we merge this, we should probably also backport to all upstream/downstream-supported branches as well) |
# 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a changelog fragment https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to.
I think the original template was more intuitive to actually fill out. Adding an item inline with a comment is exactly how I'd normally comment a yaml list: galaxy_info:
tags:
# comment is inline with items
- tag Now the only galaxy_info:
tags:
# comment indentation is different than items
- tag Maybe we could move the comment on top of the keys they describe since they're no longer at the same indentation as items? |
I also find this weird. The yamllint rule is supposed to enforce same-level indents: https://yamllint.rtfd.io/en/stable/rules.html#module-yamllint.rules.comments_indentation. Honestly, I'd like it better if the skeleton used multi-line list items rather than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Against this change, as you can see in discussions this leads us to follow the linting, not actual parser correctness.
Aside from changing a lot more often and arbitrarily, the linting is an opinion, which is far from universal, as seen in the back and forth above.
While all true, there is a commitment that ansible-lint reformats ansible files for ansible users. Currently that is done by a RT through ruamel, with some tuning of ruamel's behaviour. Given these are simply formating changes and will lead to a more consistent output and experience for ansible users, I am still in favor of moving forward with this. |
SUMMARY
While generating Ansible role using the ansible-galaxy role init command, it creates a default meta/main.yml file. If we later run ansible-lint --fix against that role, it changes the comment indentation for the galaxy_tags section.
All the required fields for meta/main.yml to pass ansible-lint are in the default file created by ansible-galaxy role init and its indention should be suitable for ansible-lint --fix.
Hence, this PR tries to make the generated meta/main.yml file compatible with ansible-lint --fix.
ISSUE TYPE
COMPONENT NAME
Related: #82394, Fixes: ansible/ansible-lint#3941