fix ansible lint seperate commits #684

dangel101 · 2022-09-28T11:31:37Z

fix ansible lint: risky file permissions: File permissions unset or incorrect
fix ansible lint: risky-shell-pipe: Shells that use pipes should set the pipefail option
fix ansible lints: - role-name: Role name does not match ^*$ pattern. - name: All tasks should be named. (name[missing])
fix ansible lint: package-latest: Package installs should not use latest
fix ansible lint: no-changed-when: Commands should not change things if nothing needs doing
fix ansible lints: - command-instead-of-module: systemctl used in place of systemd module (+ merged tasks), rpm used in place of yum or rpm_key module - command-instead-of-shell: Use shell only when shell functionality is required
fix ansible lint: ignore-errors: Use failed_when and specify error conditions instead of using ignore_errors
fix ansible lint: no-handler: Tasks that run when changed should likely be handlers
adding ansible-lint tests to CI (CI: enable ansible-lint run #484)

Fixes: #710

dangel101 · 2022-09-28T11:35:38Z

/ost

dangel101 · 2022-09-29T12:30:07Z

/ost

mz-pdm

It's reviewable now, thank you for splitting it. Some comments inside. Besides that, please pay attention to the titles of the commits, almost all look weird.

mz-pdm · 2022-09-29T13:10:03Z

packaging/ansible-runner-service-project/project/roles/hc-gluster-cgroups/tasks/main.yml

  vars:
-    gluster_cgroup_cpu_quota : "{{ [(ansible_processor_vcpus/3)|int,1]|max * 100 }}"
+    gluster_cgroup_cpu_quota: "{{ [(ansible_processor_vcpus / 3) | int, 1] | max * 100 }}"


Shouldn't there be a space also after the slash?

about the titles- it's because the title is long, looks like it was split automatically
space- yes, I fixed it in a commit I posted today: 0575c14

about the titles- it's because the title is long, looks like it was split automatically

Please don't respond to comments in unrelated places, otherwise it's hard to track discussions.

space- yes, I fixed it in a commit I posted today: 0575c14

Great, OK.

mz-pdm · 2022-09-29T13:10:03Z

packaging/ansible-runner-service-project/project/roles/gluster-replace-peers/tasks/volume.yml

 - name: Fetch the directory and volume details
  block:
    - name: Get the list of volumes on the machine
-      shell: ls "{{ glusterd_libdir }}/vols"
+      ansible.builtin.command: ls "{{ glusterd_libdir }}/vols"


Better (or mandatory?) to use argv with command rather than quoting inside the command. Also in other changes from shell where switching to command, not only here.

done- 564b39e

mz-pdm · 2022-09-29T13:10:04Z

packaging/ansible-runner-service-project/project/roles/gluster-brick-create/tasks/main.yml

    path: "{{ mntpath }}"
    state: directory
+    mode: '755'


To specify the mode, '755' notation is used in some places and 0755 in other places. Since it's part of cleanup, it would be good to be consistent and preferably use one or the other everywhere.

mz-pdm · 2022-09-29T13:10:04Z

...ner-service-project/project/roles/ovirt-host-deploy-vdsm/tasks/libvirt_presets_configure.yml

@@ -14,5 +14,5 @@
        disable virtstoraged.socket
        disable virtproxyd.socket
        enable libvirtd.service
-
-  when: el_ver|int >= 9
+      mode: preserve


When the dest file doesn't exist, system umask is used. This calls for problems, is there a reason not to specify the file mode directly here?

mz-pdm · 2022-09-29T13:10:04Z

...ging/ansible-runner-service-project/project/roles/ovirt-host-deploy-firewalld/tasks/main.yml

@@ -1,36 +1,36 @@
 ---
 - block:
    - name: Stop iptables service if running
-      service:
+      ansible.builtin.service: # noqa ignore-errors


I don't understand the reasoning in the commit message behind silencing this (and the related) warnings.

for this task- the task for running firewalld (2 tasks after this one) will fail anyway if iptables is running.
If iptables is stopped and the role can continue, and we don't care about iptables at this point- I don't see a reason to handle any errors related to iptables at this point.

host upgrade-
line #12: see my discussion with @almusil here-
#413

line #46: this is not a mandatory part of the upgrade process. we added it to clear space before upgrading, and If cleaning cache fails, in most cases there'll be enough space to continue with upgrading the packages.

for this task- the task for running firewalld (2 tasks after this one) will fail anyway if iptables is running. If iptables is stopped and the role can continue, and we don't care about iptables at this point- I don't see a reason to handle any errors related to iptables at this point.

OK. Please put this information to the commit message. The current explanation there, "Use failed_when and specify error conditions instead of using ignore_errors", talks about something different and unrelated to the current version, which confused me. It has probably been forgotten to update when the patch changed?

host upgrade- line #12: see my discussion with @almusil here- #413

line #46: this is not a mandatory part of the upgrade process. we added it to clear space before upgrading, and If cleaning cache fails, in most cases there'll be enough space to continue with upgrading the packages.

I'm losing the context here.

mz-pdm · 2022-10-03T09:57:55Z

about the titles- it's because the title is long, looks like it was split automatically

Make sure each commit message has its short single-line title, separated from the following content by a blank line.

dangel101 · 2022-10-03T11:20:37Z

about the titles- it's because the title is long, looks like it was split automatically

Make sure each commit message has its short single-line title, separated from the following content by a blank line.

done

dangel101 · 2022-10-06T12:57:04Z

@mz-pdm I pushed the fixes we discussed
thanks

dangel101 · 2022-10-06T12:57:14Z

/ost

mwperina · 2022-10-07T08:15:57Z

/ost

mwperina · 2022-10-07T12:29:40Z

/ost

dangel101 · 2022-10-20T09:44:12Z

/ost

dangel101 · 2022-10-20T11:47:56Z

/ost

dangel101 · 2022-10-23T07:22:44Z

/ost

dangel101 · 2022-10-23T09:18:46Z

/ost

dangel101 · 2022-10-24T08:26:17Z

/ost

dangel101 · 2022-10-24T11:06:43Z

/ost

dangel101 · 2022-10-25T15:16:44Z

/ost

dangel101 · 2022-10-26T08:06:30Z

/ost

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=2100137 including fixes for: - use FQCN for builtin actions - missing starting space in comment - jinja2 spacing could be improved - line too long - no new line character at the end of file - trailing spaces - too many spaces before colon - too many spaces after colon - too many blank lines - all names should start with an uppercase letter - truthy value should be one of (yaml[truthy]) - wrong indentation

'File permissions unset or incorrect'

'Shells that use pipes should set the pipefail option'

- role-name: Role name <role-name> does not match ``^*$`` pattern. - name: All tasks should be named. (name[missing])

'Commands should not change things if nothing needs doing' Since the tag 'skip_ansible_lint oVirt#301' doesn't work and '# noqa no-changed-when' cannot be set on code blocks, adding it to skip list

- command-instead-of-module: systemctl used in place of systemd module (+ merged tasks), rpm used in place of yum or rpm_key module - command-instead-of-shell: Use shell only when shell functionality is required

The error 'Use failed_when and specify error conditions instead of using ignore_errors' was handled by adding '# noqa ignore-errors' tag, since failure in these tasks shouldn't block from continuing the execution of the role

Tasks that run when changed should likely be handlers

'Templated files should use template instead of copy'

fixed in all places that replaced shell with command

- use FQCN for builtin module actions (include_tasks, import_tasks) - Use FQCN for module actions

Ignoring because when using 'set -o pipefail &&' the task fails due to non zero return code

dangel101 · 2022-10-26T11:32:12Z

/ost

mwperina

+1

dangel101 requested review from mwperina, ahadas, bennyz, emesika, michalskrivanek, oliel, sgratch, smelamud, didib and ljelinkova as code owners September 28, 2022 11:31

dangel101 requested review from mnecas, mz-pdm and almusil September 28, 2022 11:32

dangel101 mentioned this pull request Sep 28, 2022

ansible lint fixes #658

Closed

dangel101 force-pushed the fix_ansible_lint_seperate_commits branch from 6b8a262 to c50a679 Compare September 29, 2022 08:29

mz-pdm reviewed Sep 29, 2022

View reviewed changes

dangel101 force-pushed the fix_ansible_lint_seperate_commits branch from 1079e9a to 983aa07 Compare October 3, 2022 11:18

dangel101 force-pushed the fix_ansible_lint_seperate_commits branch from 983aa07 to 564b39e Compare October 6, 2022 12:55

mnecas mentioned this pull request Oct 6, 2022

CI: enable ansible-lint run #484

Closed

mwperina force-pushed the fix_ansible_lint_seperate_commits branch 2 times, most recently from 48fe11a to fcf3b2d Compare October 7, 2022 07:19

mwperina force-pushed the fix_ansible_lint_seperate_commits branch from fcf3b2d to 7d3601c Compare October 7, 2022 13:13

dangel101 force-pushed the fix_ansible_lint_seperate_commits branch from fd72d56 to f85875f Compare October 20, 2022 11:10

dangel101 force-pushed the fix_ansible_lint_seperate_commits branch from b6a7f48 to a798cae Compare October 24, 2022 10:31

dangel101 force-pushed the fix_ansible_lint_seperate_commits branch from a798cae to 56a7a69 Compare October 25, 2022 14:50

dangel101 added 15 commits October 26, 2022 13:49

fix ansible lint: risky file permissions:

b78c284

'File permissions unset or incorrect'

fix ansible lint: risky-shell-pipe:

36fc63b

'Shells that use pipes should set the pipefail option'

fix ansible lints: naming

b8cc1b2

- role-name: Role name <role-name> does not match ``^*$`` pattern. - name: All tasks should be named. (name[missing])

fix ansible lint: package-latest: Package installs should not use latest

d6d253c

fix ansible lint: no-changed-when:

243fc68

'Commands should not change things if nothing needs doing' Since the tag 'skip_ansible_lint oVirt#301' doesn't work and '# noqa no-changed-when' cannot be set on code blocks, adding it to skip list

fix ansible lints: using command

0b19b51

- command-instead-of-module: systemctl used in place of systemd module (+ merged tasks), rpm used in place of yum or rpm_key module - command-instead-of-shell: Use shell only when shell functionality is required

fix ansible lint: ignore-errors:

329a250

The error 'Use failed_when and specify error conditions instead of using ignore_errors' was handled by adding '# noqa ignore-errors' tag, since failure in these tasks shouldn't block from continuing the execution of the role

fix ansible lint: no-handler:

2280aea

Tasks that run when changed should likely be handlers

adding ansible-lint tests to CI (oVirt#484)

fc15f6b

fix ansible lint: add spacing

bb6ca23

fix ansible lint: template-instead-of-copy:

6ebc06f

'Templated files should use template instead of copy'

use argv with command instead of quoting inside the command

9545e85

fixed in all places that replaced shell with command

fix ansible lint: use FQCN

efe235b

- use FQCN for builtin module actions (include_tasks, import_tasks) - Use FQCN for module actions

fix ansible lint: ignore risky-shell-pipe

88febb7

Ignoring because when using 'set -o pipefail &&' the task fails due to non zero return code

dangel101 force-pushed the fix_ansible_lint_seperate_commits branch from 56a7a69 to 88febb7 Compare October 26, 2022 10:54

mwperina approved these changes Oct 27, 2022

View reviewed changes

mwperina merged commit 0a578cb into oVirt:master Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ansible lint seperate commits #684

fix ansible lint seperate commits #684

dangel101 commented Sep 28, 2022 •

edited by mwperina

dangel101 commented Sep 28, 2022

dangel101 commented Sep 29, 2022

mz-pdm left a comment

mz-pdm Sep 29, 2022

dangel101 Sep 29, 2022

mz-pdm Oct 3, 2022

mz-pdm Sep 29, 2022

dangel101 Oct 6, 2022

mz-pdm Sep 29, 2022

dangel101 Oct 3, 2022

mz-pdm Sep 29, 2022

dangel101 Oct 3, 2022

mz-pdm Sep 29, 2022

dangel101 Oct 2, 2022

mz-pdm Oct 3, 2022

mz-pdm commented Oct 3, 2022

dangel101 commented Oct 3, 2022

dangel101 commented Oct 6, 2022

dangel101 commented Oct 6, 2022

mwperina commented Oct 7, 2022

mwperina commented Oct 7, 2022

dangel101 commented Oct 20, 2022

dangel101 commented Oct 20, 2022

dangel101 commented Oct 23, 2022

dangel101 commented Oct 23, 2022

dangel101 commented Oct 24, 2022

dangel101 commented Oct 24, 2022

dangel101 commented Oct 25, 2022

dangel101 commented Oct 26, 2022

dangel101 commented Oct 26, 2022

mwperina left a comment

fix ansible lint seperate commits #684

fix ansible lint seperate commits #684

Conversation

dangel101 commented Sep 28, 2022 • edited by mwperina

dangel101 commented Sep 28, 2022

dangel101 commented Sep 29, 2022

mz-pdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mz-pdm commented Oct 3, 2022

dangel101 commented Oct 3, 2022

dangel101 commented Oct 6, 2022

dangel101 commented Oct 6, 2022

mwperina commented Oct 7, 2022

mwperina commented Oct 7, 2022

dangel101 commented Oct 20, 2022

dangel101 commented Oct 20, 2022

dangel101 commented Oct 23, 2022

dangel101 commented Oct 23, 2022

dangel101 commented Oct 24, 2022

dangel101 commented Oct 24, 2022

dangel101 commented Oct 25, 2022

dangel101 commented Oct 26, 2022

dangel101 commented Oct 26, 2022

mwperina left a comment

Choose a reason for hiding this comment

dangel101 commented Sep 28, 2022 •

edited by mwperina