Make roles generation compatible with ansible-lint autofix feature #82529
Conversation
ansibot
added
bug
|
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. |
|
(assuming we merge this, we should probably also backport to all upstream/downstream-supported branches as well) |
mkrizek
removed
the
needs_triage
| # 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.
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.
@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.
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.
@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.
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.
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.
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
- tagNow the only galaxy_info:
tags:
# comment indentation is different than items
- tagMaybe 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.
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.
ansibot
added
the
needs_revision
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. |
ansibot
added
the
stale_ci
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