From: "Max Carrara" <m.carrara@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files
Date: Wed, 22 May 2024 09:00:11 +0200 [thread overview]
Message-ID: <D1FZ9RA91XD3.2W5YL07SBVZ8V@proxmox.com> (raw)
In-Reply-To: <20240521133148.223266-1-l.wagner@proxmox.com>
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.carrara@proxmox.com>
Reviewed-by: Max Carrara <m.carrara@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
next prev parent reply other threads:[~2024-05-22 7:00 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-21 13:31 Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH proxmox-perl-rs v3 1/8] notify: use file based notification templates Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH proxmox-perl-rs v3 2/8] notify: don't pass config structs by reference Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH proxmox-perl-rs v3 3/8] notify: adapt to Option<Vec<T>> to Vec<T> changes in proxmox_notify Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH cluster v3 4/8] notify: use named template instead of passing template strings Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH pve-ha-manager v3 5/8] env: notify: use named templates " Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH manager v3 6/8] gitignore: ignore any test artifacts Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH manager v3 7/8] tests: remove vzdump_notification test Lukas Wagner
2024-05-21 13:31 ` [pve-devel] [PATCH manager v3 8/8] notifications: use named templates instead of in-code templates Lukas Wagner
2024-05-22 7:00 ` Max Carrara [this message]
2024-05-29 12:29 ` [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Maximiliano Sandoval
2024-06-04 9:14 ` [pve-devel] applied-series: " Wolfgang Bumiller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D1FZ9RA91XD3.2W5YL07SBVZ8V@proxmox.com \
--to=m.carrara@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox