-
Notifications
You must be signed in to change notification settings - Fork 317
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
Improve linting #514
Improve linting #514
Conversation
Use strict rules for `ansible-lint` and `yamlling` and ignore misleading code like `molecule` and GitHub-Actions.
Change the older values of `yes`/`no` to the newer consistent values of `ansible-lint`: `true`/`false`
Rename all builtin modules to the matching FQCN.
Rename all Windows and APK modules to the matching FQCN.
More simple changes to use only one styles of quotes and remove trailing whitespaces and empty lines.
Ensure variable `consul_autopilot_cleanup_dead_Servers` is matching the naming guidelines: change variable name to lower-case.
According to `ansible-lint` all task-names have to start with a capital lettter.
Use the new order for blocks. This nearly unreadble change puts all meta-data for blocks, like `register`, when`, `become` and similar attributes to the top and makes this configuration more readable. This change also introduces names for blocks.
There are some tasks that need to be executed in this exact order and there are tasks that are generating things, e.g. keys and secrets. Around these tasks are safeguards that justify the implentation of the `noqa` statements.
@@ -112,7 +112,7 @@ | |||
# Check for unzip binary | |||
|
|||
- name: Check if unzip is installed on control host | |||
shell: "command -v unzip -h >/dev/null 2>&1" | |||
ansible.builtin.shell: "command -v unzip -h >/dev/null 2>&1" |
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.
CRITICAL Error see build log
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.
ansible.builtin.
(collection) available starting from ansible 3 (2.10).
And this means that there will be no compatibility with ansible 2.9 which is still used by some distributions (e.g. debian 10, ubuntu 20.04)
@ppuschmann Consider ignoring this warning and leaving compatibility with older versions of Ansible.
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.
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.
Rather re-work the PR to do a remote install as the unzip won't work in an execution environment (AWX/AAP)
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.
I don't quite get the issue.
Is there a general issue with with this unzip? 8and maybe limited to AWX/AAP?
-> I don't really want to touch this right now.
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.
I think I'll just close this PR for now and think about re-opening another one or multiple smaller ones.
Especially after also stumbling over specialties of yamllint
requiring "two" spaces before a comment.
redhat-developer/vscode-yaml#433
Sorry.
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.
tasks/asserts.yml action starting on line 114 is unparsable.
Let me have look at this tomorrow. |
@bbaassssiiee Another try... ;-)