-
Notifications
You must be signed in to change notification settings - Fork 261
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
ansible lint fixes #658
ansible lint fixes #658
Conversation
Including fixes according to ansible lint rules for engine roles and adding ansible-lint tests to CI (oVirt#484) Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2100137
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.
Great to have this cleanup! But this is a huge commit and in order to be able to review it and merge safely, it's important to separate purely formal changes, which can be reviewed and merged quickly, from the changes that require some thought. Examples of changes that I would prefer having each in a separate commit are in comments.
cd "${SRCDIR}" | ||
|
||
# Find playbooks | ||
PARAMS=$(find ${PLABOOKS_DIR} -type f -name '*.yml' -maxdepth 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.
Not checking playbooks anymore?
connection: ssh | ||
|
||
- name: Restart glusterd on the new node | ||
connection: ssh |
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.
Why was it here (and in the other places below) and now we remove it? Better to do this in a commit separate from the purely formal changes.
gluster peer status | | ||
grep -A1 {{ gluster_old_node | mandatory }} | | ||
awk -F: '/uid/ { print $2}' | ||
register: uuid | ||
tags: | ||
- skip_ansible_lint # E301 | ||
changed_when: uuid.rc != 0 |
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 would separate this (here and below) too.
|
||
- name: Get the state from peer file | ||
shell: grep state "/var/lib/glusterd/peers/{{ uuid.stdout | trim }}" | ||
ansible.builtin.command: grep state "/var/lib/glusterd/peers/{{ uuid.stdout | trim }}" |
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.
Probably not a problem here but I think it's safer to use argv
argument of command
in situations with generated command line arguments.
path: "{{ mntpath }}" | ||
state: directory | ||
mode: '755' |
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.
Better to put possibly functional changes to a separate commit or a PR.
/ost |
Including fixes according to ansible lint rules for engine roles and adding
ansible-lint tests to CI (#484)
Fixes: #710