Re: [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files

["Max Carrara" <m.carrara@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