Adding pre-commit with ansible, yaml and other standard linters

Numerous stylistic issues with ansible code resolved. Including: - unmarked changes to the system - use of custom scripts rather than module calls - missing permissions for created or altered files - too long lines The pv.yaml template was renamed to pv.yaml.j2, in order to avoid clashes with linters and to highlight it's nature as a jinja template. Signed-off-by: Jiri Podivin <[email protected]>
danpawlik · Jun 15, 2023 · 1a590c9 · 1a590c9
1 parent 31b892f
commit 1a590c9
Show file tree

Hide file tree

Showing 15 changed files with 126 additions and 22 deletions.
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
@@ -0,0 +1,47 @@
+---
+repos:
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v4.4.0
+    hooks:
+      - id: end-of-file-fixer
+      - id: trailing-whitespace
+      - id: mixed-line-ending
+      - id: fix-byte-order-marker
+      - id: check-executables-have-shebangs
+      - id: check-merge-conflict
+      - id: check-symlinks
+      - id: debug-statements
+      - id: check-yaml
+        files: .*\.(yaml|yml)$
+        args: [--allow-multiple-documents]
+  - repo: https://github.com/pycqa/flake8
+    rev: 5.0.4
+    hooks:
+      - id: flake8
+        additional_dependencies: [flake8-typing-imports==1.6.0]
+        # List of ignored checks overrides the flake8 defaults.
+        # Changes to the list, especially extensions should be
+        # justified with relation to defaults.
+        # E501 - line is too long, would require local ignore or obfuscating changes
+        # W503 - line break before binary operator, ignored by default
+        entry: flake8 --ignore=E501,W503
+  - repo: https://github.com/ansible-community/ansible-lint
+    rev: v6.14.2
+    hooks:
+      - id: ansible-lint
+        additional_dependencies:
+          - ansible-core
+          - yamllint
+  - repo: https://github.com/openstack-dev/bashate.git
+    rev: 2.1.1
+    hooks:
+      - id: bashate
+        entry: bashate --error . --ignore=E006,E040
+        verbose: false
+        # Run bashate check for all bash scripts
+        # Ignores the following rules:
+        # E006: Line longer than 79 columns (as many scripts use jinja
+        #       templating, this is very difficult)
+        # E040: Syntax error determined using `bash -n` (as many scripts
+        #       use jinja templating, this will often fail and the syntax
+        #       error will be discovered in execution anyway)
diff --git a/.yamllint b/.yamllint
@@ -0,0 +1,11 @@
+---
+extends: default
+
+rules:
+  line-length:
+    # matches hardcoded 160 value from ansible-lint
+    max: 160
+
+ignore: |
+  zuul.d/*.yaml
+  templates/*.yaml
diff --git a/tasks/coredns.yaml b/tasks/coredns.yaml
@@ -9,6 +9,7 @@
   ansible.builtin.file:
     path: "{{ coredns_config_dir }}"
     state: directory
+    mode: "0755"
 
 - name: Check if DNS backup already exists
   become: true
@@ -21,8 +22,10 @@
   block:
     - name: Create dns configmap backup
       become: true
-      ansible.builtin.shell: |
-        oc --kubeconfig ~{{ ansible_user }}/.kube/config -n openshift-dns get configmap dns-default -o yaml > {{ coredns_config_dir }}/dns-configmap-default.yaml
+      ansible.builtin.shell: >
+        oc --kubeconfig ~{{ ansible_user }}/.kube/config -n openshift-dns get configmap dns-default
+        -o yaml > {{ coredns_config_dir }}/dns-configmap-default.yaml
+      changed_when: true
 
     - name: Copy the dns-configmap.yaml to safe location
       become: true
@@ -54,24 +57,30 @@
       ansible.builtin.template:
         src: "corefile.yaml"
         dest: "{{ coredns_config_dir }}/corefile.yaml"
+        mode: "0644"
       notify: Restart Openshift DNS
 
     - name: Dump corefile.yaml into raw content
       become: true
       ansible.builtin.shell: |
         cat {{ coredns_config_dir }}/corefile.yaml | xargs -d $'\n' printf '%s\\n' > {{ coredns_config_dir }}/corefile.raw
+      changed_when: true
 
     - name: Patch configmap with new content
       become: true
-      ansible.builtin.shell: |
-        /usr/local/bin/yq ".data.Corefile = \"$(cat {{ coredns_config_dir }}/corefile.raw)\"" /etc/microshift/dns-configmap-default.yaml > {{ coredns_config_dir }}/patched-corefile.yaml
+      ansible.builtin.shell: >
+        /usr/local/bin/yq ".data.Corefile = \"$(cat {{ coredns_config_dir }}/corefile.raw)\""
+        /etc/microshift/dns-configmap-default.yaml > {{ coredns_config_dir }}/patched-corefile.yaml
+      changed_when: true
 
     - name: Remove keys that will block coredns configmap update
       become: true
-      ansible.builtin.shell: |
-        sed -i '/  creationTimestamp:\|  resourceVersion:\|  uid:/d' {{ coredns_config_dir }}/patched-corefile.yaml
+      ansible.builtin.replace:
+        path: "{{ coredns_config_dir }}/patched-corefile.yaml"
+        regexp: '^\s+creationTimestamp:.*$|^\s+resourceVersion:.*$|^\s+uid:.*$'
 
     - name: Apply new coredns configmap
       ansible.builtin.shell: |
         kubectl apply -f {{ coredns_config_dir }}/patched-corefile.yaml
       notify: Restart Openshift DNS
+      changed_when: true
diff --git a/tasks/crio.yaml b/tasks/crio.yaml
@@ -15,6 +15,7 @@
   tags:
     - skip_ansible_lint
   register: _crio_version
+  changed_when: false
 
 - name: Use only ipv4 - new Microshift
   become: true
@@ -39,8 +40,9 @@
   ansible.builtin.copy:
     src: policy.json
     dest: /etc/containers/policy.json
+    mode: "0644"
   notify: Restart crio
   when: overwrite_container_policy
 
 - name: Flush handlers
-  meta: flush_handlers
+  ansible.builtin.meta: flush_handlers
diff --git a/tasks/dnsmasq.yaml b/tasks/dnsmasq.yaml
@@ -18,20 +18,21 @@
     src: /etc/dnsmasq.conf
     dest: /etc/NetworkManager/dnsmasq.d/dnsmasq.conf
     remote_src: true
+    mode: "0644"
   notify:
     - Disable dnsmasq service
     - Reload NetworkManager
 
 - name: Check if 99-cloud-init.conf exists
   become: true
-  stat:
+  ansible.builtin.stat:
     path: /etc/NetworkManager/conf.d/99-cloud-init.conf
   register: _cloud_init_conf
 
 - name: Comment 99-cloud-init.conf file
   become: true
   when: _cloud_init_conf.stat.exists
-  copy:
+  ansible.builtin.copy:
     content: |
       ## Created by cloud-init on instance boot automatically, do not edit.
       #
@@ -40,6 +41,7 @@
       #[main]
       #dns = none
     dest: /etc/NetworkManager/conf.d/99-cloud-init.conf
+    mode: "0644"
 
 - name: Add information about new dnsmasq configuration file location
   become: true
@@ -58,7 +60,7 @@
     - regexp: "^#server=/3.168.192.in-addr.arpa/10.1.2.3"
       line: "server={{ cloudprovider_dns | default(public_dns) | random }}"
     - regexp: '^# server=10.1.2.3@eth1'
-      line: "server={{ public_dns | random}}"
+      line: "server={{ public_dns | random }}"
     - regexp: "^interface=lo"
       line: "#interface=lo"
     - regexp: "^#address=\/double-click.net\/127.0.0.1"
@@ -79,11 +81,12 @@
           fi
           echo "${IP}"
         dest: /tmp/recognize_ingress_ip.sh
-        mode: 0755
+        mode: "0755"
 
     - name: Get Ingress IP Address
       ansible.builtin.command: /tmp/recognize_ingress_ip.sh
       register: _lb_ip
+      changed_when: true
 
     - name: Set the LB or Router IP address as default address for FQDN
       ansible.builtin.set_fact:
@@ -105,9 +108,10 @@
       [main]
       dns=dnsmasq
     dest: /etc/NetworkManager/conf.d/00-microshift-use-dnsmasq.conf
+    mode: "0644"
   notify:
     - Disable dnsmasq service
     - Reload NetworkManager
 
 - name: Flush handlers
-  meta: flush_handlers
+  ansible.builtin.meta: flush_handlers
diff --git a/tasks/firewall.yaml b/tasks/firewall.yaml
@@ -1,6 +1,6 @@
 ---
 - name: Get package facts
-  package_facts:
+  ansible.builtin.package_facts:
 
 - name: Add exception to the firewall
   when: "'firewalld' in ansible_facts.packages"

diff --git a/tasks/microshift.yaml b/tasks/microshift.yaml
@@ -6,6 +6,7 @@
       become: true
       ansible.builtin.shell: |
         dnf copr enable -y @redhat-et/microshift
+      changed_when: true
 
     - name: Install microshift package
       become: true
@@ -24,7 +25,7 @@
   when: not use_copr_microshift
 
 - name: Flush handlers
-  meta: flush_handlers
+  ansible.builtin.meta: flush_handlers
 
 - name: Wait for kubeconfig file after deploying Microshift
   become: true
@@ -38,6 +39,7 @@
   ansible.builtin.file:
     path: ~/.kube
     state: directory
+    mode: "0755"
 
 - name: Copy kubeconfig
   become: true
@@ -47,3 +49,4 @@
     remote_src: true
     owner: "{{ ansible_user }}"
     group: "{{ ansible_user }}"
+    mode: "0644"
diff --git a/tasks/olm.yaml b/tasks/olm.yaml
@@ -1,7 +1,9 @@
+---
 - name: Ensure installation directory
-  file:
+  ansible.builtin.file:
     path: "{{ repo_dir }}"
     state: directory
+    mode: "0755"
 
 - name: Fetch operator-sdk
   ansible.builtin.uri:
@@ -17,12 +19,15 @@
     "{{ repo_dir }}/operator-sdk olm status"
   register: olm_status
   failed_when: olm_status.rc not in [0, 1]
+  changed_when: true
 
 - name: Install OLM with SDK
   ansible.builtin.command:
     "{{ repo_dir }}/operator-sdk olm install --version {{ olm_version }}"
   when: olm_status.rc != 0
+  changed_when: true
 
 - name: Ensure privileged SCC for OLM
   ansible.builtin.command:
     oc adm policy add-scc-to-user privileged system:serviceaccount:olm:default
+  changed_when: true
diff --git a/tasks/openshift-storage.yaml b/tasks/openshift-storage.yaml
@@ -8,21 +8,24 @@
       ansible.builtin.command: oc get namespace openshift-storage
       register: _openshift_storage_ns
       failed_when: _openshift_storage_ns.rc not in [0, 1]
+      changed_when: true
 
     - name: Delete openshift storage namespace
       ansible.builtin.command: oc delete namespace openshift-storage
       when: _openshift_storage_ns.rc == 0
+      changed_when: true
 
     - name: Delete topolvm-provisioner storageclass
       ansible.builtin.command: oc delete storageclass topolvm-provisioner
       when: _openshift_storage_ns.rc == 0
+      changed_when: true
 
 - name: Create LVM on loop device to deploy openshift storage with topolvm
   when: not delete_openshift_storage
   block:
     - name: Check if file already exists
       become: true
-      stat:
+      ansible.builtin.stat:
         path: "{{ disk_file_path }}"
       register: _disk_file
 
@@ -48,10 +51,11 @@
           become: true
           ansible.builtin.command: losetup -f
           register: loop_device
+          changed_when: true
 
         - name: Create script to start lvm volumes after reboot
           become: true
-          copy:
+          ansible.builtin.copy:
             src: start-lvm.sh
             dest: /usr/local/bin/start-lvm.sh
             mode: "0755"
@@ -61,6 +65,7 @@
           ansible.builtin.template:
             src: microshift-loop-device.service.j2
             dest: /usr/lib/systemd/system/microshift-loop-device.service
+            mode: "0644"
 
         - name: Enable loop device service
           become: true

diff --git a/tasks/pv.yaml b/tasks/pv.yaml
@@ -15,6 +15,7 @@
     group: "{{ ansible_user }}"
     setype: container_file_t
     selevel: s0
+    mode: "0755"
 
 - name: Create PV directories
   become: true
@@ -42,22 +43,26 @@
       provisioner: kubernetes.io/no-provisioner
       volumeBindingMode: WaitForFirstConsumer
     dest: "~{{ ansible_user }}/storage-class.yaml"
+    mode: "0644"
 
 - name: Apply storage class manifest
   ansible.builtin.command: oc apply -f "~{{ ansible_user }}/storage-class.yaml"
+  changed_when: true
 
 - name: Get worker nodes
   ansible.builtin.shell: |
     oc get node -o name -l node-role.kubernetes.io/worker | sed -e 's|node/||' | head -c-1 | tr '\n' ','
   tags:
     - skip_ansible_lint
   register: _worker_nodes
+  changed_when: true
 
 - name: Add PV
   ansible.builtin.template:
-    src: pv.yaml
+    src: pv.yaml.j2
     dest: "~{{ ansible_user }}/storage.yaml"
     mode: "0644"
 
 - name: Run PV manifest
   ansible.builtin.command: oc apply -f ~{{ ansible_user }}/storage.yaml
+  changed_when: true
diff --git a/tasks/repo.yaml b/tasks/repo.yaml
@@ -5,9 +5,11 @@
   ansible.builtin.template:
     src: microshift-deps.repo.j2
     dest: /etc/yum.repos.d/microshift-deps.repo
+    mode: "0644"
 
 - name: Setup repository - Microshift
   become: true
   ansible.builtin.template:
     src: microshift.repo.j2
     dest: /etc/yum.repos.d/microshift.repo
+    mode: "0644"