Add Rich's review

README.md

@@ -2,7 +2,7 @@
 
 [![ansible-lint.yml](https://github.com/linux-system-roles/bootloader/actions/workflows/ansible-lint.yml/badge.svg)](https://github.com/linux-system-roles/bootloader/actions/workflows/ansible-lint.yml) [![ansible-test.yml](https://github.com/linux-system-roles/bootloader/actions/workflows/ansible-test.yml/badge.svg)](https://github.com/linux-system-roles/bootloader/actions/workflows/ansible-test.yml) [![markdownlint.yml](https://github.com/linux-system-roles/bootloader/actions/workflows/markdownlint.yml/badge.svg)](https://github.com/linux-system-roles/bootloader/actions/workflows/markdownlint.yml) [![woke.yml](https://github.com/linux-system-roles/bootloader/actions/workflows/woke.yml/badge.svg)](https://github.com/linux-system-roles/bootloader/actions/workflows/woke.yml)
 
-An Ansible role for bootloader and kernel command line management.
+An Ansible role for boot loader and kernel command line management.
 
 ## Requirements
 
@@ -12,11 +12,11 @@ None
 
 ### bootloader_settings
 
-A `list` of `dict` object. Each `dict` might has one or more of the following keys:
+A `list` of kernel command line parameters. Each `dict` might has one or more of the following keys:
 
-* `name` - The name the setting. `name` is omitted when using replaced.
-* `value` - The value for the setting. You must ommit `value` if the setting has no value, e.g. `quiet`.
-* `state` - `state` (default) or `absent`. The value `absent` means to remove a setting with `name` name - name must be provided.
+* `name` - The name of the setting. `name` is omitted when using `replaced`.
+* `value` - The value for the setting. You must omit `value` if the setting has no value, e.g. `quiet`.
+* `state` - `present` (default) or `absent`. The value `absent` means to remove a setting with `name` name - name must be provided.
 * `previous` - Optional - the only value is `replaced` - this is used to specify that the previous values in a group should be replaced with the given values.
 
 Default: `[]`
@@ -25,7 +25,7 @@ Type: `list`
 
 ### bootloader_timeout
 
-With this variable, you can customize the loading time of the GRUB bootloader.
+With this variable, you can customize the loading time of the GRUB boot loader.
 
 Default: `5`
 

handlers/main.yml

@@ -1,8 +1,18 @@
 # SPDX-License-Identifier: MIT
 ---
 - name: Write boot loader configuration
-  command: grub2-mkconfig -o {{ __bootloader_grub_conf }}
-  changed_when: true
+  include_tasks: tasks/write_bootloader.yml
 
 - name: Reboot system
-  include_tasks: handlers/reboot.yml
+  include_tasks: tasks/reboot.yml
+
+- name: Fix default kernel boot parameters (EL 7)
+  shell: |-
+    set -o pipefail
+    eval $(grubby --info=DEFAULT | awk '/^args/ {print $0}')
+    sed -i -e "s,^GRUB_CMDLINE_LINUX=.*,GRUB_CMDLINE_LINUX=\"$args\","
+    {{ __bootloader_default_grub }}
+  changed_when: true
+  when:
+    - ansible_distribution in ['CentOS', 'RedHat']
+    - ansible_facts.distribution_major_version is version('7', '=')

tasks/main.yml

@@ -37,19 +37,6 @@
   include_tasks: modify_settings.yml
   loop: "{{ bootloader_settings }}"
 
-# A workaround for https://bugzilla.redhat.com/show_bug.cgi?id=1152027
-- name: Fix default kernel boot parameters (EL 7)
-  shell: |-
-    set -o pipefail
-    eval $(grubby --info=DEFAULT | awk '/^args/ {print $0}')
-    sed -i -e "s,^GRUB_CMDLINE_LINUX=.*,GRUB_CMDLINE_LINUX=\"$args\","
-    {{ __bootloader_default_grub }}
-  changed_when: true
-  when:
-    - ansible_distribution in ['CentOS', 'RedHat']
-    - ansible_facts.distribution_major_version is version('7', '=')
-    - __bootloader_conf_boot_params is changed
-
 - name: Update boot loader timeout configuration
   lineinfile:
     path: "{{ __bootloader_default_grub }}"
@@ -60,14 +47,14 @@
 - name: Determine platform type
   stat:
     path: /sys/firmware/efi
-  register: efi_dir
+  register: __bootloader_efi_dir
 
 - name: Set boot loader configuration files
   vars:
     efi_path: "{{
       ansible_distribution in ['CentOS', 'RedHat'] and
       ansible_facts.distribution_major_version | int < 9 and
-      efi_dir.stat.exists }}"
+      __bootloader_efi_dir.stat.exists }}"
   set_fact:
     __bootloader_grub_conf: >-
       {{ efi_path | ternary('/boot/efi/EFI/redhat/grub.cfg',
@@ -82,7 +69,8 @@
     - name: Generate boot loader password
       shell: >-
         set -euo pipefail;
-        ( echo {{ bootloader_password }} ; echo {{ bootloader_password }} )
+        ( echo {{ bootloader_password | quote }} ;
+        echo {{ bootloader_password | quote }} )
         | LC_ALL=C grub2-mkpasswd-pbkdf2
         | grep -v '[eE]nter password:'
         | sed -e "s/PBKDF2 hash of your password is //"
@@ -97,6 +85,7 @@
         mode: 0600
       changed_when: true
       notify: Write boot loader configuration
+      no_log: true
 
 - name: Remove boot loader password configuration
   file:

tasks/modify_settings.yml

@@ -6,8 +6,9 @@
     __bootloader_with_value: >-
       {{ item.name is defined and item.value is defined }}
     __bootloader_setting: >-
-      {{ __bootloader_with_value | ternary(item.name + '=' + item.value | d(),
-      item.name) }}"
+      {{ __bootloader_with_value |
+      ternary(item.name | string + '=' + item.value | d() | string,
+      item.name) }}
     __bootloader_absent: "{{ item.state | d('present') == 'absent' }}"
   block:
     - name: Check boot setting {{ __bootloader_setting }}

tasks/write_bootloader.yml

@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: MIT
+---
+- name: Write boot loader configuration
+  command: grub2-mkconfig -o {{ __bootloader_grub_conf }}
+  changed_when: true
+
+# A workaround for https://bugzilla.redhat.com/show_bug.cgi?id=1152027
+- name: Fix default kernel boot parameters on EL 7
+  shell: |-
+    set -o pipefail
+    eval $(grubby --info=DEFAULT | awk '/^args/ {print $0}')
+    sed -i -e "s,^GRUB_CMDLINE_LINUX=.*,GRUB_CMDLINE_LINUX=\"$args\","
+    {{ __bootloader_default_grub }}
+  changed_when: true
+  when:
+    - ansible_distribution in ['CentOS', 'RedHat']
+    - ansible_facts.distribution_major_version is version('7', '=')

tests/tests_settings.yml

@@ -11,6 +11,10 @@
     - name: Replace configuration with settings
       vars:
         bootloader_settings:
+          - name: console
+            value: tty0
+          - name: net.ifnames
+            value: 0
           - name: no_timer_check
             state: present
           - name: quiet
@@ -29,7 +33,10 @@
     - name: Verify settings
       shell: >-
         set -euo pipefail;
-        grubby --info=DEFAULT | grep 'args="no_timer_check quiet rd.lvm.lv"$'
+        grubby --info=DEFAULT |
+        grep -P
+        'args="(\$tuned_params |)console=tty0 net.ifnames=0 no_timer_check
+        quiet rd.lvm.lv"$'
       changed_when: false
 
     - name: Remove some settings
@@ -45,5 +52,7 @@
     - name: Verify settings
       shell: >-
         set -euo pipefail;
-        grubby --info=DEFAULT | grep 'args="no_timer_check"$'
+        grubby --info=DEFAULT |
+        grep -P
+        'args="(\$tuned_params |)console=tty0 net.ifnames=0 no_timer_check"$'
       changed_when: false