Fix ansible-lint and yamllint issues #255

thystips · 2024-08-23T17:39:28Z

Juste in case : I have not tested playbooks in test/ directory.

Ansible-lint result

WARNING  Ignored exception from ArgsRule.matchtasks while processing tasks/pve_cluster_config.yml (tasks): No module named 'ansible.module_utils.pvesh'
WARNING  Ignored exception from ArgsRule.matchtasks while processing tasks/main.yml (tasks): No module named 'ansible.module_utils.pvesh'

Passed: 0 failure(s), 0 warning(s) on 56 files. Last profile that met the validation criteria was 'production'.

I can't fix warnings about module_utils but for ansible-lint project is't a issue

Signed-off-by: ThysTips <contact@antoinethys.com>

lae

I'll need more time to finish this review later but for the time being there are some things I need feedback on. Please also rebase against develop (and amend the commit/force push).

Consider everything reviewed except for tasks/ceph.yml and tests/test.yml for now.

lae · 2024-11-14T15:36:56Z

.travis.yml

Ignore this file.

lae · 2024-11-14T15:41:49Z

defaults/main.yml

@@ -22,13 +22,14 @@ pve_pcie_report_msrs: true
 pve_watchdog: none
 pve_watchdog_ipmi_action: power_cycle
 pve_watchdog_ipmi_timeout: 10
-pve_zfs_enabled: no
+pve_zfs_enabled: false


yamllint follows yaml 1.2 spec, which removes these truthy values, but Ansible does not. Ansible docs still heavily use yes/no as boolean and that doesn't seem like it's changing anytime soon, so I feel like these should be ignored. Thoughts? (I updated .yamllint.yml to reflect this for the time being.)

lae · 2024-11-14T15:49:10Z

defaults/main.yml

The README.md should reflect these changes, especially since we're adding an additional role variable.

lae · 2024-11-14T16:10:42Z

handlers/main.yml

These handlers are being updated without the respective notify attributes being updated in task files, which I believe breaks the role?

...that said, I don't 100% agree with ansible-lint's casing rule being forced to apply to strings passed to notify . There's some discussion at https://forum.ansible.com/t/ansible-lints-fixation-on-upper-case-initial-letters/5862 that shares my sentiment. However, while it's not mentioned there, I can see the handler tasks themselves following the casing rule because at the end they'll look like normal tasks to whoever is running the playbook so...using the listen attribute to work around this does seem like the better option.

Keep the names as-is in this file for now.

lae · 2024-11-14T16:13:42Z

tasks/ceph.yml

@@ -1,50 +1,52 @@
 # This is an Ansible version of what "pveceph install" actually does
 ---
-
- block:
+- name: Create initial Ceph config


rename to Ceph initialisation

lae · 2024-11-14T16:41:30Z

tasks/main.yml

  when:
    - not pve_cluster_enabled | bool or (pve_cluster_enabled and inventory_hostname == groups[pve_group][0])
    - pve_domains_cfg | length > 0

 - name: Select ldap-based realms with sync
-  set_fact:
+  ansible.builtin.set_fact:
    pve_ldap_realms_with_sync: |


This might not need to be wrapped? Either way, this change does not seem to follow the usual indentation rule, nor does it try to follow any alignment for readability...

lae · 2024-11-14T16:43:02Z

tasks/pve_add_node.yml

note to self: has task key order rule

lae · 2024-11-14T16:44:13Z

tasks/main.yml

note to self: has task key order rule

lae · 2024-11-14T16:56:30Z

tasks/ssh_cluster_config.yml

-    create: yes
-    mode: 0644
+    create: true
+    mode: "0644"
    dest: /root/.ssh/config


I want to move dest above create here for consistency with the earlier blockinfile task.

lae · 2024-11-14T16:59:02Z

tasks/pve_cluster_config.yml


 - name: Update PVE cluster HA groups
-  command: >-
+  ansible.builtin.command: >-


Might not need to be wrapped with updated line length limit.

lae

Okay, done checking the last two files. See comments and last review.

lae · 2024-11-15T03:21:03Z

tasks/ceph.yml

+              \\g<space>type \\g<r_type>\
+              \\g<space>min_size {{ (pve_ceph_crush_rules | selectattr(\"name\", \"match\", item.name) | list)[0].min_size | default(\"\\g<min>\") | trim }}\
+              \\g<space>max_size {{ (pve_ceph_crush_rules | selectattr(\"name\", \"match\", item.name) | list)[0].max_size | default(\"\\g<max>\") | trim }}\
+              {%- if ((pve_ceph_crush_rules | selectattr(\"name\", \"match\", item.name) | list)[0].class | default(False)) -%}\
              \\g<space>step take default class {{ (pve_ceph_crush_rules | selectattr(\"name\", \"match\", item.name) | list)[0].class }}\


this and the untouched line below should also be indented for readability (because they're inside conditionals)

lae · 2024-11-15T03:24:21Z

tasks/ceph.yml

+          ansible.builtin.command: crushtool -c crush_map_decompressed -o new_crush_map_compressed
+          register: _crushmap_valid
+          when: _crushmap.changed
+          changed_when: _crushmap_valid.rc == 0


This should probably just be true? Cause the other cases are just task error if rc != 1 or task skip...I think? If that's accurate then this statement just feels like it's an unnecessary layer of indirection cause it'll always be successful as long as it completes.

lae · 2024-11-15T03:30:31Z

tasks/ceph.yml

        pveceph fs create
        --name {{ item.name }}
        --add-storage {{ item.storage }}
        --pg_num {{ item.pgs }}
      register: _ceph_fs_create
      failed_when: _ceph_fs_create.stderr
      when: "item.name not in (_ceph_fs.stdout | from_json | map(attribute='name'))"
-      with_items: '{{ pve_ceph_fs }}'
+      with_items: "{{ pve_ceph_fs }}"
+      changed_when: _ceph_fs_create.rc == 0


Same thing about the layer of indirection here.

lae · 2024-11-15T03:36:08Z

tests/test.yml

Can you add a comment to this file stating that all the shell tasks were replaced with command tasks?
As you've said you haven't tested these so there may or may not be breakage here...I don't really think there is, but just in case, that comment will help me remember if I run into any snags when I get around to rebuilding the test environment.

refacto: Fix all ansible-lint and yamllint issues

30b13da

Signed-off-by: ThysTips <contact@antoinethys.com>

thystips changed the title ~~Fix all ansible-lint and yamllint issues~~ Fix ansible-lint and yamllint issues Aug 26, 2024

thystips marked this pull request as draft August 26, 2024 11:29

thystips marked this pull request as ready for review August 26, 2024 11:29

thystips mentioned this pull request Aug 29, 2024

Issue: Align role name and variables with ansible-lint standards #261

Closed

lae requested changes Nov 14, 2024

View reviewed changes

lae requested changes Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ansible-lint and yamllint issues #255

Fix ansible-lint and yamllint issues #255

thystips commented Aug 23, 2024 •

edited

Loading

lae left a comment •

edited

Loading

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae Nov 14, 2024

lae left a comment

lae Nov 15, 2024

lae Nov 15, 2024

lae Nov 15, 2024

lae Nov 15, 2024

Fix ansible-lint and yamllint issues #255

Are you sure you want to change the base?

Fix ansible-lint and yamllint issues #255

Conversation

thystips commented Aug 23, 2024 • edited Loading

Ansible-lint result

lae left a comment • edited Loading

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

lae 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

thystips commented Aug 23, 2024 •

edited

Loading

lae left a comment •

edited

Loading