Jose/ansible_role_proxmox_provision
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor ♻️: Refactor legacy and minified proxmoxlib.js patching logic to use handlers #2

Merged
Jose merged 7 commits from dev into main 2026-02-08 08:06:27 +01:00
Conversation 3 Commits 7 Files Changed 4 +85 -59
4 changed files with 85 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
README.md
View File

@@ -115,6 +115,8 @@ See the [LICENSE](LICENSE) file for details.
## TODO
⏳ Make the nag patch checksum-based (auto-repatch after upgrades)
⏳ add molecule tests to prove idempotency
gitea-actions commented 2026-02-08 07:13:32 +01:00
Review
Copy Link

[Score: 2] The addition of a new task suggests further development, which is fine as long as it's well-documented and integrated thoughtfully. It would be beneficial to ensure these tests are designed to thoroughly verify the idempotency of the patches.

[Score: 2] The addition of a new task suggests further development, which is fine as long as it's well-documented and integrated thoughtfully. It would be beneficial to ensure these tests are designed to thoroughly verify the idempotency of the patches.
gitea-actions commented 2026-02-08 08:00:17 +01:00
Review
Copy Link

[Score: 3] It's good practice to include automated testing for your code changes, but you might want to consider adding more context about what these Molecule tests are and why they are necessary.

[Score: 3] It's good practice to include automated testing for your code changes, but you might want to consider adding more context about what these Molecule tests are and why they are necessary.
⏳ make the patch handler trigger on pve-manager upgrades
gitea-actions commented 2026-02-08 07:13:32 +01:00
Review
Copy Link

[Score: 2] It's good to see an initiative to handle patches during pve-manager upgrades, but it's important to consider how this will be implemented without causing any potential conflicts or instability.

[Score: 2] It's good to see an initiative to handle patches during pve-manager upgrades, but it's important to consider how this will be implemented without causing any potential conflicts or instability.
gitea-actions commented 2026-02-08 08:00:17 +01:00
Review
Copy Link

[Score: 3] This change seems reasonable, but you might want to consider any potential implications or edge cases that may arise when running the patch handler after PVE Manager upgrades. Ensure proper testing is done to cover these scenarios.

[Score: 3] This change seems reasonable, but you might want to consider any potential implications or edge cases that may arise when running the patch handler after PVE Manager upgrades. Ensure proper testing is done to cover these scenarios.
❌ Add kernel power-saving tunables ?
gitea-actions commented 2026-02-08 08:05:25 +01:00
Review
Copy Link

[Score: 3] This line suggests adding kernel power-saving tunables, but no further details are provided. It would be beneficial to explain which specific tunables to add and why they might improve the system's performance or energy efficiency.

[Score: 3] This line suggests adding kernel power-saving tunables, but no further details are provided. It would be beneficial to explain which specific tunables to add and why they might improve the system's performance or energy efficiency.
🔄 Split into VE version–aware tags ?
🕒 refactor

59
handlers/main.yml
View File

@@ -14,3 +14,62 @@
- name: Reload systemd
ansible.builtin.systemd_service:
daemon_reexec: true
- name: Patch legacy proxmoxlib.js
block:
- name: Patch legacy proxmoxlib.js
gitea-actions commented 2026-02-08 07:13:32 +01:00
Outdated
Review
Copy Link

[Lines 18-20] [Score: 3] Consider adding variable names for clarify and easier maintenance. For example: patch_legacy_proxmoxlib, patch_minified_proxmoxlib_min.

[Lines 18-20] [Score: 3] Consider adding variable names for clarify and easier maintenance. For example: `patch_legacy_proxmoxlib`, `patch_minified_proxmoxlib_min`.
ansible.builtin.replace:
path: /usr/share/javascript/proxmox-widget-toolkit/proxmoxlib.js
regexp: "if \\(data.status !== 'Active'\\)"
replace: "if (false)"
gitea-actions commented 2026-02-08 08:05:25 +01:00
Review
Copy Link

[Score: 3] Replacing the condition with a constant value could lead to unexpected behavior or confusion when debugging. It's better to keep the original check if possible, or at least document the purpose of this change.

[Score: 3] Replacing the condition with a constant value could lead to unexpected behavior or confusion when debugging. It's better to keep the original check if possible, or at least document the purpose of this change.
register: patch_legacy
failed_when: patch_legacy.matched == 0
notify: Restart pveproxy
- name: Re-stat proxmoxlib.js
ansible.builtin.stat:
path: /usr/share/javascript/proxmox-widget-toolkit/proxmoxlib.js
checksum_algorithm: sha256
register: proxmoxlib_js_after
- name: Store patched checksum (legacy)
ansible.builtin.copy:
dest: /var/lib/proxmox-nag-patch/proxmoxlib.js.sha256
owner: root
group: root
mode: "0644"
content: "{{ proxmoxlib_js_after.stat.checksum }}\n"
gitea-actions commented 2026-02-08 08:05:25 +01:00
Review
Copy Link

[Score: 3] Using a variable for the checksum directly in the file content could potentially expose sensitive information, especially if this playbook is run on an unsecured machine or shared environment. Consider hashing the checksum value before using it to avoid such risks.

[Score: 3] Using a variable for the checksum directly in the file content could potentially expose sensitive information, especially if this playbook is run on an unsecured machine or shared environment. Consider hashing the checksum value before using it to avoid such risks.
- name: Restart pveproxy
ansible.builtin.systemd:
name: pveproxy
state: restarted
- name: Patch minified proxmoxlib.js
block:
- name: Patch minified proxmoxlib.min.js
ansible.builtin.replace:
path: /usr/share/javascript/proxmox-widget-toolkit/proxmoxlib.min.js
regexp: "data.status!=='Active'"
replace: "false"
register: patch_minified
failed_when: patch_minified.matched == 0
- name: Re-stat proxmoxlib.min.js
ansible.builtin.stat:
path: /usr/share/javascript/proxmox-widget-toolkit/proxmoxlib.min.js
gitea-actions commented 2026-02-08 08:05:25 +01:00
Review
Copy Link

[Score: 3] It's good practice to document the purpose and usage of variables used throughout a playbook, especially when they contain sensitive information like file paths or checksums. Adding comments explaining what proxmoxlib_min_after is for would improve maintainability.

[Score: 3] It's good practice to document the purpose and usage of variables used throughout a playbook, especially when they contain sensitive information like file paths or checksums. Adding comments explaining what `proxmoxlib_min_after` is for would improve maintainability.
checksum_algorithm: sha256
register: proxmoxlib_min_after
- name: Store patched checksum (minified)
ansible.builtin.copy:
dest: /var/lib/proxmox-nag-patch/proxmoxlib.min.js.sha256
owner: root
group: root
mode: "0644"
content: "{{ proxmoxlib_min_after.stat.checksum }}\n"
- name: Restart pveproxy
ansible.builtin.systemd:
name: pveproxy
state: restarted
gitea-actions commented 2026-02-08 08:00:17 +01:00
Review
Copy Link

[Score: 4] Logic error in patching the proxmoxlib files that may affect VM management through Proxmox UI.

[Score: 4] Logic error in patching the proxmoxlib files that may affect VM management through Proxmox UI.

14
tasks/repos.yml
View File

@@ -1,11 +1,11 @@
---
- name: repos | Remove enterprise repo files (all known locations)
ansible.builtin.file:
path: "{{ item }}"
state: absent
loop:
- /etc/apt/sources.list.d/pve-enterprise.list
- /etc/apt/sources.list.d/ceph.list
- name: Comment out Proxmox enterprise repo lines
ansible.builtin.replace:
path: /etc/apt/sources.list.d/pve-enterprise.list
regexp: '^(deb\s+)'
replace: '# \1'
when: ansible.builtin.stat(path='/etc/apt/sources.list.d/pve-enterprise.list').stat.exists
notify: apt update
- name: repos | Enable Proxmox no-subscription repo
ansible.builtin.copy:
gitea-actions commented 2026-02-08 08:05:25 +01:00
Review
Copy Link

[Lines 1-11] [Score: 2] The task name for enabling the Proxmox no-subscription repository should be more descriptive, e.g., "Enable Proxmox no-subscription repo".

[Lines 1-11] [Score: 2] The task name for enabling the Proxmox no-subscription repository should be more descriptive, e.g., "Enable Proxmox no-subscription repo".

69
tasks/subscription.yml
View File

@@ -10,12 +10,6 @@
############################
# Legacy proxmoxlib.js
############################
- name: subscription | Read stored checksum (legacy)
ansible.builtin.slurp:
src: /var/lib/proxmox-nag-patch/proxmoxlib.js.sha256
register: proxmoxlib_js_checksum_stored
when: proxmoxlib_js.stat.exists
failed_when: false
- name: subscription | Check for legacy proxmoxlib.js
ansible.builtin.stat:
@@ -23,32 +17,21 @@
checksum_algorithm: sha256
register: proxmoxlib_js
- name: subscription | Remove subscription nag (legacy proxmoxlib.js)
ansible.builtin.replace:
path: /usr/share/javascript/proxmox-widget-toolkit/proxmoxlib.js
regexp: "if \\(data.status !== 'Active'\\)"
replace: "if (false)"
- name: subscription | Read stored checksum (legacy)
ansible.builtin.slurp:
src: /var/lib/proxmox-nag-patch/proxmoxlib.js.sha256
register: proxmoxlib_js_checksum_stored
when: proxmoxlib_js.stat.exists
failed_when: false
- name: subscription | Trigger legacy nag patch if needed
ansible.builtin.meta: flush_handlers
when:
- proxmoxlib_js.stat.exists
- proxmoxlib_js_checksum_stored.content is not defined
or (proxmoxlib_js.stat.checksum
!= (proxmoxlib_js_checksum_stored.content | b64decode | trim))
register: patch_legacy
failed_when:
- proxmoxlib_js.stat.exists
- patch_legacy.matched == 0
notify: restart pveproxy
- name: subscription | Store patched checksum (legacy)
ansible.builtin.copy:
dest: /var/lib/proxmox-nag-patch/proxmoxlib.js.sha256
owner: root
group: root
mode: "0644"
content: "{{ proxmoxlib_js.stat.checksum }}\n"
when:
- proxmoxlib_js.stat.exists
- patch_legacy is changed
or proxmoxlib_js.stat.checksum
!= (proxmoxlib_js_checksum_stored.content | b64decode | trim)
gitea-actions commented 2026-02-08 08:05:25 +01:00
Review
Copy Link

[Lines 31-33] [Score: 2] The condition checks if proxmoxlib_js_checksum_stored.content is not defined, but it's actually registered in the previous task. This means this check may never be true, and the "Patch legacy proxmoxlib.js" notification will always be triggered when the playbook runs. To fix this issue, you can change the condition to only check if proxmoxlib_js.stat.checksum is different from the stored checksum.

[Lines 31-33] [Score: 2] The condition checks if `proxmoxlib_js_checksum_stored.content` is not defined, but it's actually registered in the previous task. This means this check may never be true, and the "Patch legacy proxmoxlib.js" notification will always be triggered when the playbook runs. To fix this issue, you can change the condition to only check if `proxmoxlib_js.stat.checksum` is different from the stored checksum.
notify: Ppatch legacy proxmoxlib.js
gitea-actions commented 2026-02-08 07:13:32 +01:00
Review
Copy Link

[Lines 28-35] [Score: 2] This task could be simplified by combining the ansible.builtin.meta: flush_handlers and the subsequent notify call into a single line for better readability.

[Lines 28-35] [Score: 2] This task could be simplified by combining the `ansible.builtin.meta: flush_handlers` and the subsequent `notify` call into a single line for better readability.
############################
# Minified proxmoxlib.min.js (VE 8/9)
@@ -67,29 +50,11 @@
when: proxmoxlib_min_js.stat.exists
failed_when: false
- name: subscription | Remove subscription nag (minified bundle for VE 8/9)
ansible.builtin.replace:
path: /usr/share/javascript/proxmox-widget-toolkit/proxmoxlib.min.js
regexp: "data.status!=='Active'"
replace: "false"
- name: subscription | Trigger minified nag patch if needed
ansible.builtin.meta: flush_handlers
when:
- proxmoxlib_min_js.stat.exists
- proxmoxlib_min_checksum_stored.content is not defined
or (proxmoxlib_min_js.stat.checksum
!= (proxmoxlib_min_checksum_stored.content | b64decode | trim))
register: patch_minified
failed_when:
- proxmoxlib_min_js.stat.exists
- patch_minified.matched == 0
notify: restart pveproxy
- name: subscription | Store patched checksum (minified)
ansible.builtin.copy:
dest: /var/lib/proxmox-nag-patch/proxmoxlib.min.js.sha256
owner: root
group: root
mode: "0644"
content: "{{ proxmoxlib_min_js.stat.checksum }}\n"
when:
- proxmoxlib_min_js.stat.exists
- patch_minified is changed
or proxmoxlib_min_js.stat.checksum
!= (proxmoxlib_min_checksum_stored.content | b64decode | trim)
notify: Patch minified proxmoxlib.js
gitea-actions commented 2026-02-08 07:13:32 +01:00
Review
Copy Link

[Lines 54-60] [Score: 2] Similar to the previous suggestion, this task could also be simplified by combining the ansible.builtin.meta: flush_handlers and the subsequent notify call into a single line for better readability.

[Lines 54-60] [Score: 2] Similar to the previous suggestion, this task could also be simplified by combining the `ansible.builtin.meta: flush_handlers` and the subsequent `notify` call into a single line for better readability.