search

ansible-collections / ansible-consul

:satellite: Ansible role for Hashicorp Consul clusters
https://galaxy.ansible.com/ansible-community/consul/
BSD 2-Clause "Simplified" License
450 stars 313 forks source link

Fix ansible lint #574

Closed rndmh3ro closed 7 months ago

nre-ableton commented 7 months ago

Thanks for the PR @rndmh3ro, but unfortunately CI is broken due to the role name (though I see that you've already run into this and tried to fix it). I am working on refactoring this role into a collection and will post a proposal soon.

rndmh3ro commented 7 months ago

@nre-ableton, I was able to work-around this. It may not be pretty but we can build upon these changes. So please take a look. :)

Nethertheless converting this into a collection is the right way to go.

nre-ableton commented 7 months ago

Yes, I see that you got CI working, though I need to review this code before approving/merging. I agree that the solution isn't pretty, but it might be enough to help get us migrated to the collection. I'd prefer if the molecule fixes in particular were short-term. Nonetheless, I'll review this PR, but I'd prefer to merge it after we get some clarity on the migration process (but if that ends up taking too long, we can always merge anyways).

As promised, here's my comment on the proposed collection migration process: https://github.com/ansible-collections/ansible-inclusion/discussions/64#discussioncomment-6962013

nre-ableton commented 7 months ago

Would documenting style choices be an idea?

Yes, we should do that at some point. If we adopt a quoting style, it should probably be the same one that Ansible uses.

To that end, Ansible is not very clear about quoting style, the only explicit recommendation I've found on the topic is in the Windows guide:

You should only quote strings when it is absolutely necessary or required by YAML, and then use single quotes.

It would be nice if ansible-lint enforced this (see https://github.com/ansible/ansible-lint/discussions/3937), but that probably won't happen for awhile.

rndmh3ro commented 7 months ago

This role used to have an (implicit) code-style where strings having Jinja2 were double-quoted, and strings having non-alphabetic characters were single-quoted. Would documenting style choices be an idea?

It would be nice if ansible-lint enforced this (see https://github.com/ansible/ansible-lint/discussions/3937), but that probably won't happen for awhile.

I actually used ansible-lint --fix - and it changed the single quotes to double quotes. So I'm in favor of keeping this standard. If ansible-lint decides to use single-quotes or makes it configurable, we can change it IMO.

nre-ableton commented 7 months ago

I actually used ansible-lint --fix - and it changed the single quotes to double quotes. So I'm in favor of keeping this standard. If ansible-lint decides to use single-quotes or makes it configurable, we can change it IMO.

Until we have a standard or style guide, I'm personally ok with this approach.

bbaassssiiee commented 7 months ago

Nice to see that this is in good hands now.