From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id ED91E1FF389 for ; Wed, 22 May 2024 09:00:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 671036360; Wed, 22 May 2024 09:00:47 +0200 (CEST) Mime-Version: 1.0 Date: Wed, 22 May 2024 09:00:11 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max Carrara" X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240521133148.223266-1-l.wagner@proxmox.com> In-Reply-To: <20240521133148.223266-1-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [vzdump.pm, pve2.pm, apt.pm, notify.rs, replication.pm, notify.pm, env.pm, nodestatus.pm] Subject: Re: [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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 Reviewed-by: Max Carrara > > 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 -{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> to Vec 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