On Tue May 21, 2024 at 3:31 PM CEST, Lukas Wagner wrote:
> These changes adapts the PVE notification stack to the changes introduced
> in proxmox-notify 0.4.
Applied all patches, bumped deps manually where needed and gave this
series a spin on both my development VM and my new HA test cluster.
Building
--------
No issues here, except that the dependency bump(s) for `proxmox-notify`
could *maybe* already be included in a (separate?) patch IMO, but no
hard feelings there.
Testing
-------
* installed all relevant deb packages on my development VM
* packages installed cleanly, didn't notice any issues
* ran a random backup, vzdump notification was received as expected
* installed all relevant deb packages on my HA test cluster VMs
* packages also installed cleanly there, didn't notice any issues
(HA kept working)
* intentionally stopped one of the HA nodes on which a VM was running
to force the VM being fenced
* received fencing notifications as expected
* altered one of the fencing notif templates and forced another fence
for good measure
* altered notification came through, as expected (neat!)
Seems to work flawlessly. Nice job!
Didn't go through every single template here; just wanted to ensure that
I covered a rather common case (running backups) and a case that's
considered more critical (HA fencing, definitely don't wanna miss that).
Code Review
-----------
Nothing special here - as in, the patches are logically separated, easy
to follow and formatted according to our style guides & `cargo fmt`.
Summary
-------
LGTM.
Tested-by: Max Carrara <m.carr...@proxmox.com>
Reviewed-by: Max Carrara <m.carr...@proxmox.com>
>
> The notification system uses handlebar templates to render the subject
> and the body of notifications. Previously, the template strings were
> defined inline at the call site. This patch series extracts the templates
> into template files and installs them at
> /usr/share/pve-manager/templates/default
>
> where they stored as <template-name>-{body,subject}.{txt,html}.hbs
>
> The 'default' part in the path is a preparation for translated
> notifications and/or overridable notification templates.
> Future work could provide notifications translated to e.g. German
> in `templates/de` or similar. This will be a first for having
> translated strings on the backend-side, so there is need for further
> research.
>
> Folke kindly did some off-list testing before this was posted, hence
> his T-bs were included.
>
> Bumps/dependencies:
> - libproxmox-rs-perl needs to have its proxmox-notify dep updated to 0.4
> and breaks old libpve-notify-perl (versioned break)
> - libpve-notify-perl breaks old pve-manager and old pve-ha-manager
> (versioned break)
>
> The versioned breaks are necessary due to changed semantics in the API
> (passing
> a template name instead of template strings) and due to changes in how
> templates are rendered (separate templates for HTML/plain text, whereas before
> both were rendered from the same template string, with some magic from
> handlebar helpers)
>
> Changes since v2:
> - Dropped already applied patches for 'proxmox'
> - Rebased, quick smoke-test to check if anything broke
> in the meanwhile
>
> Changes since v1:
> - Incorporated feedback from @Fiona and @Fabian - thanks!
> - most noteworthy: Change template path from /usr/share/proxmox-ve
> to /usr/share/pve-manager
> - apart from that mostly just cosmetics/style
>
> proxmox-perl-rs:
>
> Lukas Wagner (3):
> notify: use file based notification templates
> notify: don't pass config structs by reference
> notify: adapt to Option<Vec<T>> to Vec<T> changes in proxmox_notify
>
> common/src/notify.rs | 48 +++++++++++++++++++++-----------------------
> 1 file changed, 23 insertions(+), 25 deletions(-)
>
>
> pve-cluster:
>
> Lukas Wagner (1):
> notify: use named template instead of passing template strings
>
> src/PVE/Notify.pm | 29 ++++++++++++-----------------
> 1 file changed, 12 insertions(+), 17 deletions(-)
>
>
> pve-ha-manager:
>
> Lukas Wagner (1):
> env: notify: use named templates instead of passing template strings
>
> debian/pve-ha-manager.install | 3 +++
> src/Makefile | 1 +
> src/PVE/HA/Env/PVE2.pm | 4 ++--
> src/PVE/HA/NodeStatus.pm | 20 +------------------
> src/PVE/HA/Sim/Env.pm | 3 ++-
> src/templates/Makefile | 10 ++++++++++
> src/templates/default/fencing-body.html.hbs | 14 +++++++++++++
> src/templates/default/fencing-body.txt.hbs | 11 ++++++++++
> src/templates/default/fencing-subject.txt.hbs | 1 +
> 9 files changed, 45 insertions(+), 22 deletions(-)
> create mode 100644 src/templates/Makefile
> create mode 100644 src/templates/default/fencing-body.html.hbs
> create mode 100644 src/templates/default/fencing-body.txt.hbs
> create mode 100644 src/templates/default/fencing-subject.txt.hbs
>
>
> pve-manager:
>
> Lukas Wagner (3):
> gitignore: ignore any test artifacts
> tests: remove vzdump_notification test
> notifications: use named templates instead of in-code templates
>
> .gitignore | 2 +
> Makefile | 2 +-
> PVE/API2/APT.pm | 9 +-
> PVE/API2/Replication.pm | 20 +---
> PVE/VZDump.pm | 20 +---
> templates/Makefile | 24 +++++
> .../default/package-updates-body.html.hbs | 6 ++
> .../default/package-updates-body.txt.hbs | 3 +
> .../default/package-updates-subject.txt.hbs | 1 +
> templates/default/replication-body.html.hbs | 18 ++++
> templates/default/replication-body.txt.hbs | 12 +++
> templates/default/replication-subject.txt.hbs | 1 +
> templates/default/test-body.html.hbs | 1 +
> templates/default/test-body.txt.hbs | 1 +
> templates/default/test-subject.txt.hbs | 1 +
> templates/default/vzdump-body.html.hbs | 11 ++
> templates/default/vzdump-body.txt.hbs | 10 ++
> templates/default/vzdump-subject.txt.hbs | 1 +
> test/Makefile | 6 +-
> test/vzdump_notification_test.pl | 101 ------------------
> 20 files changed, 98 insertions(+), 152 deletions(-)
> create mode 100644 templates/Makefile
> create mode 100644 templates/default/package-updates-body.html.hbs
> create mode 100644 templates/default/package-updates-body.txt.hbs
> create mode 100644 templates/default/package-updates-subject.txt.hbs
> create mode 100644 templates/default/replication-body.html.hbs
> create mode 100644 templates/default/replication-body.txt.hbs
> create mode 100644 templates/default/replication-subject.txt.hbs
> create mode 100644 templates/default/test-body.html.hbs
> create mode 100644 templates/default/test-body.txt.hbs
> create mode 100644 templates/default/test-subject.txt.hbs
> create mode 100644 templates/default/vzdump-body.html.hbs
> create mode 100644 templates/default/vzdump-body.txt.hbs
> create mode 100644 templates/default/vzdump-subject.txt.hbs
> delete mode 100755 test/vzdump_notification_test.pl
>
>
> Summary over all repositories:
> 31 files changed, 178 insertions(+), 216 deletions(-)
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
