public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files
@ 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
                   ` (10 more replies)
  0 siblings, 11 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

These changes adapts the PVE notification stack to the changes introduced
in proxmox-notify 0.4.

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(-)

-- 
Generated by git-murpp 0.7.1


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH proxmox-perl-rs v3 1/8] notify: use file based notification templates
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
@ 2024-05-21 13:31 ` 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
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

Instead of passing literal template strings to the notification
system, we now only pass an identifier. This identifier will be used
load the template files from a product-specific directory.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 common/src/notify.rs | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/common/src/notify.rs b/common/src/notify.rs
index 8f9f38f..d965417 100644
--- a/common/src/notify.rs
+++ b/common/src/notify.rs
@@ -94,16 +94,14 @@ mod export {
     fn send(
         #[try_from_ref] this: &NotificationConfig,
         severity: Severity,
-        title: String,
-        body: String,
+        template_name: String,
         template_data: Option<JSONValue>,
         fields: Option<HashMap<String, String>>,
     ) -> Result<(), HttpError> {
         let config = this.config.lock().unwrap();
-        let notification = Notification::new_templated(
+        let notification = Notification::from_template(
             severity,
-            title,
-            body,
+            template_name,
             template_data.unwrap_or_default(),
             fields.unwrap_or_default(),
         );
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH proxmox-perl-rs v3 2/8] notify: don't pass config structs by reference
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files 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 ` 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
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

proxmox_notify's api functions have been changed so that they take
ownership of config structs.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 common/src/notify.rs | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/common/src/notify.rs b/common/src/notify.rs
index d965417..00a6056 100644
--- a/common/src/notify.rs
+++ b/common/src/notify.rs
@@ -151,7 +151,7 @@ mod export {
 
         api::sendmail::add_endpoint(
             &mut config,
-            &SendmailConfig {
+            SendmailConfig {
                 name,
                 mailto,
                 mailto_user,
@@ -185,7 +185,7 @@ mod export {
         api::sendmail::update_endpoint(
             &mut config,
             name,
-            &SendmailConfigUpdater {
+            SendmailConfigUpdater {
                 mailto,
                 mailto_user,
                 from_address,
@@ -236,7 +236,7 @@ mod export {
         let mut config = this.config.lock().unwrap();
         api::gotify::add_endpoint(
             &mut config,
-            &GotifyConfig {
+            GotifyConfig {
                 name: name.clone(),
                 server,
                 comment,
@@ -244,7 +244,7 @@ mod export {
                 filter: None,
                 origin: None,
             },
-            &GotifyPrivateConfig { name, token },
+            GotifyPrivateConfig { name, token },
         )
     }
 
@@ -266,12 +266,12 @@ mod export {
         api::gotify::update_endpoint(
             &mut config,
             name,
-            &GotifyConfigUpdater {
+            GotifyConfigUpdater {
                 server,
                 comment,
                 disable,
             },
-            &GotifyPrivateConfigUpdater { token },
+            GotifyPrivateConfigUpdater { token },
             delete.as_deref(),
             digest.as_deref(),
         )
@@ -323,7 +323,7 @@ mod export {
         let mut config = this.config.lock().unwrap();
         api::smtp::add_endpoint(
             &mut config,
-            &SmtpConfig {
+            SmtpConfig {
                 name: name.clone(),
                 server,
                 port,
@@ -337,7 +337,7 @@ mod export {
                 disable,
                 origin: None,
             },
-            &SmtpPrivateConfig { name, password },
+            SmtpPrivateConfig { name, password },
         )
     }
 
@@ -366,7 +366,7 @@ mod export {
         api::smtp::update_endpoint(
             &mut config,
             name,
-            &SmtpConfigUpdater {
+            SmtpConfigUpdater {
                 server,
                 port,
                 mode,
@@ -378,7 +378,7 @@ mod export {
                 comment,
                 disable,
             },
-            &SmtpPrivateConfigUpdater { password },
+            SmtpPrivateConfigUpdater { password },
             delete.as_deref(),
             digest.as_deref(),
         )
@@ -427,7 +427,7 @@ mod export {
         let mut config = this.config.lock().unwrap();
         api::matcher::add_matcher(
             &mut config,
-            &MatcherConfig {
+            MatcherConfig {
                 name,
                 match_severity,
                 match_field,
@@ -464,7 +464,7 @@ mod export {
         api::matcher::update_matcher(
             &mut config,
             name,
-            &MatcherConfigUpdater {
+            MatcherConfigUpdater {
                 match_severity,
                 match_field,
                 match_calendar,
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH proxmox-perl-rs v3 3/8] notify: adapt to Option<Vec<T>> to Vec<T> changes in proxmox_notify
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files 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 ` 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
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 common/src/notify.rs | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/common/src/notify.rs b/common/src/notify.rs
index 00a6056..e1b006b 100644
--- a/common/src/notify.rs
+++ b/common/src/notify.rs
@@ -153,8 +153,8 @@ mod export {
             &mut config,
             SendmailConfig {
                 name,
-                mailto,
-                mailto_user,
+                mailto: mailto.unwrap_or_default(),
+                mailto_user: mailto_user.unwrap_or_default(),
                 from_address,
                 author,
                 comment,
@@ -329,8 +329,8 @@ mod export {
                 port,
                 mode,
                 username,
-                mailto,
-                mailto_user,
+                mailto: mailto.unwrap_or_default(),
+                mailto_user: mailto_user.unwrap_or_default(),
                 from_address,
                 author,
                 comment,
@@ -429,10 +429,10 @@ mod export {
             &mut config,
             MatcherConfig {
                 name,
-                match_severity,
-                match_field,
-                match_calendar,
-                target,
+                match_severity: match_severity.unwrap_or_default(),
+                match_field: match_field.unwrap_or_default(),
+                match_calendar: match_calendar.unwrap_or_default(),
+                target: target.unwrap_or_default(),
                 mode,
                 invert_match,
                 comment,
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH cluster v3 4/8] notify: use named template instead of passing template strings
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (2 preceding siblings ...)
  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 ` Lukas Wagner
  2024-05-21 13:31 ` [pve-devel] [PATCH pve-ha-manager v3 5/8] env: notify: use named templates " Lukas Wagner
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

The notification system will now load template files from a defined
location. The template to use is now passed to proxmox_notify, instead
of separate template strings for subject/body.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 src/PVE/Notify.pm | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/src/PVE/Notify.pm b/src/PVE/Notify.pm
index 872eb25..c514111 100644
--- a/src/PVE/Notify.pm
+++ b/src/PVE/Notify.pm
@@ -58,17 +58,16 @@ sub write_config {
 }
 
 my $send_notification = sub {
-    my ($severity, $title, $message, $template_data, $fields, $config) = @_;
+    my ($severity, $template_name, $template_data, $fields, $config) = @_;
     $config = read_config() if !defined($config);
-    $config->send($severity, $title, $message, $template_data, $fields);
+    $config->send($severity, $template_name, $template_data, $fields);
 };
 
 sub notify {
-    my ($severity, $title, $message, $template_data, $fields, $config) = @_;
+    my ($severity, $template_name, $template_data, $fields, $config) = @_;
     $send_notification->(
         $severity,
-        $title,
-        $message,
+        $template_name,
         $template_data,
         $fields,
         $config
@@ -76,11 +75,10 @@ sub notify {
 }
 
 sub info {
-    my ($title, $message, $template_data, $fields, $config) = @_;
+    my ($template_name, $template_data, $fields, $config) = @_;
     $send_notification->(
         'info',
-        $title,
-        $message,
+        $template_name,
         $template_data,
         $fields,
         $config
@@ -88,11 +86,10 @@ sub info {
 }
 
 sub notice {
-    my ($title, $message, $template_data, $fields, $config) = @_;
+    my ($template_name, $template_data, $fields, $config) = @_;
     $send_notification->(
         'notice',
-        $title,
-        $message,
+        $template_name,
         $template_data,
         $fields,
         $config
@@ -100,11 +97,10 @@ sub notice {
 }
 
 sub warning {
-    my ($title, $message, $template_data, $fields, $config) = @_;
+    my ($template_name, $template_data, $fields, $config) = @_;
     $send_notification->(
         'warning',
-        $title,
-        $message,
+        $template_name,
         $template_data,
         $fields,
         $config
@@ -112,11 +108,10 @@ sub warning {
 }
 
 sub error {
-    my ($title, $message, $template_data, $fields, $config) = @_;
+    my ($template_name, $template_data, $fields, $config) = @_;
     $send_notification->(
         'error',
-        $title,
-        $message,
+        $template_name,
         $template_data,
         $fields,
         $config
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH pve-ha-manager v3 5/8] env: notify: use named templates instead of passing template strings
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (3 preceding siblings ...)
  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 ` Lukas Wagner
  2024-05-21 13:31 ` [pve-devel] [PATCH manager v3 6/8] gitignore: ignore any test artifacts Lukas Wagner
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 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

diff --git a/debian/pve-ha-manager.install b/debian/pve-ha-manager.install
index a7598a9..0ffbd8d 100644
--- a/debian/pve-ha-manager.install
+++ b/debian/pve-ha-manager.install
@@ -38,3 +38,6 @@
 /usr/share/perl5/PVE/HA/Usage/Static.pm
 /usr/share/perl5/PVE/Service/pve_ha_crm.pm
 /usr/share/perl5/PVE/Service/pve_ha_lrm.pm
+/usr/share/pve-manager/templates/default/fencing-body.html.hbs
+/usr/share/pve-manager/templates/default/fencing-body.txt.hbs
+/usr/share/pve-manager/templates/default/fencing-subject.txt.hbs
diff --git a/src/Makefile b/src/Makefile
index 87bb0de..56bd360 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -73,6 +73,7 @@ install: watchdog-mux pve-ha-crm pve-ha-lrm ha-manager.1 pve-ha-crm.8 pve-ha-lrm
 	install -d $(DESTDIR)/$(MAN1DIR)
 	install -m 0644 ha-manager.1 $(DESTDIR)/$(MAN1DIR)
 	gzip -9 $(DESTDIR)/$(MAN1DIR)/ha-manager.1
+	$(MAKE) -C templates $@
 
 .PHONY: test
 test: 
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index fcb60a9..cb73bcf 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -221,10 +221,10 @@ sub log {
 }
 
 sub send_notification {
-    my ($self, $subject, $text, $template_data, $metadata_fields) = @_;
+    my ($self, $template_name, $template_data, $metadata_fields) = @_;
 
     eval {
-	PVE::Notify::error($subject, $text, $template_data, $metadata_fields);
+	PVE::Notify::error($template_name, $template_data, $metadata_fields);
     };
 
     $self->log("warning", "could not notify: $@") if $@;
diff --git a/src/PVE/HA/NodeStatus.pm b/src/PVE/HA/NodeStatus.pm
index e053c55..9e6d898 100644
--- a/src/PVE/HA/NodeStatus.pm
+++ b/src/PVE/HA/NodeStatus.pm
@@ -188,23 +188,6 @@ sub update {
    }
 }
 
-my $body_template = <<EOT;
-{{#verbatim}}
-The node '{{node}}' failed and needs manual intervention.
-
-The PVE HA manager tries  to fence it and recover the configured HA resources to
-a healthy node if possible.
-
-Current fence status: {{subject-prefix}}
-{{subject}}
-{{/verbatim}}
-
-{{heading-2 "Overall Cluster status:"}}
-{{object status-data}}
-EOT
-
-my $subject_template = "{{subject-prefix}}: {{subject}}";
-
 # assembles a commont text for fence emails
 my $send_fence_state_email = sub {
     my ($self, $subject_prefix, $subject, $node) = @_;
@@ -228,8 +211,7 @@ my $send_fence_state_email = sub {
     };
 
     $haenv->send_notification(
-	$subject_template,
-	$body_template,
+	"fencing",
 	$template_data,
 	$metadata_fields,
     );
diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
index d3aea8d..0f77065 100644
--- a/src/PVE/HA/Sim/Env.pm
+++ b/src/PVE/HA/Sim/Env.pm
@@ -289,11 +289,12 @@ sub log {
 }
 
 sub send_notification {
-    my ($self, $subject, $text, $properties) = @_;
+    my ($self, $template_name, $properties) = @_;
 
     # The template for the subject is "{{subject-prefix}}: {{subject}}"
     # We have to perform poor-man's template rendering to pass the test cases.
 
+    my $subject = "{{subject-prefix}}: {{subject}}";
     $subject = $subject =~ s/\{\{subject-prefix}}/$properties->{"subject-prefix"}/r;
     $subject = $subject =~ s/\{\{subject}}/$properties->{"subject"}/r;
 
diff --git a/src/templates/Makefile b/src/templates/Makefile
new file mode 100644
index 0000000..396759f
--- /dev/null
+++ b/src/templates/Makefile
@@ -0,0 +1,10 @@
+NOTIFICATION_TEMPLATES=							\
+	default/fencing-subject.txt.hbs				\
+	default/fencing-body.txt.hbs				\
+	default/fencing-body.html.hbs				\
+
+.PHONY: install
+install:
+	install -dm 0755 $(DESTDIR)/usr/share/pve-manager/templates/default
+	$(foreach i,$(NOTIFICATION_TEMPLATES), \
+	    install -m644 $(i) $(DESTDIR)/usr/share/pve-manager/templates/$(i) ;)
diff --git a/src/templates/default/fencing-body.html.hbs b/src/templates/default/fencing-body.html.hbs
new file mode 100644
index 0000000..1420348
--- /dev/null
+++ b/src/templates/default/fencing-body.html.hbs
@@ -0,0 +1,14 @@
+<html>
+    <body>
+    The node '{{node}}' failed and needs manual intervention.<br/><br/>
+
+    The PVE HA manager tries to fence it and recover the configured HA resources to
+    a healthy node if possible.<br/><br/>
+
+    Current fence status: {{subject-prefix}}<br/>
+    {{subject}}<br/>
+
+    <h2 style="font-size: 1em">Overall Cluster status:</h2>
+    {{object status-data}}
+    </body>
+</html>
diff --git a/src/templates/default/fencing-body.txt.hbs b/src/templates/default/fencing-body.txt.hbs
new file mode 100644
index 0000000..e46a1fd
--- /dev/null
+++ b/src/templates/default/fencing-body.txt.hbs
@@ -0,0 +1,11 @@
+The node '{{node}}' failed and needs manual intervention.
+
+The PVE HA manager tries to fence it and recover the configured HA resources to
+a healthy node if possible.
+
+Current fence status: {{subject-prefix}}
+{{subject}}
+
+Overall Cluster status:
+-----------------------
+{{object status-data}}
diff --git a/src/templates/default/fencing-subject.txt.hbs b/src/templates/default/fencing-subject.txt.hbs
new file mode 100644
index 0000000..43651f9
--- /dev/null
+++ b/src/templates/default/fencing-subject.txt.hbs
@@ -0,0 +1 @@
+{{subject-prefix}}: {{subject}}
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH manager v3 6/8] gitignore: ignore any test artifacts
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (4 preceding siblings ...)
  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 ` Lukas Wagner
  2024-05-21 13:31 ` [pve-devel] [PATCH manager v3 7/8] tests: remove vzdump_notification test Lukas Wagner
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 .gitignore | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.gitignore b/.gitignore
index e8d1eb27..481ae1e0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,3 +9,5 @@ dest/
 /www/mobile/pvemanager-mobile.js
 /www/touch/touch-[0-9]*/
 /pve-manager-[0-9]*/
+/test/.mocked_*
+/test/*.tmp
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH manager v3 7/8] tests: remove vzdump_notification test
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (5 preceding siblings ...)
  2024-05-21 13:31 ` [pve-devel] [PATCH manager v3 6/8] gitignore: ignore any test artifacts Lukas Wagner
@ 2024-05-21 13:31 ` 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
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

With the upcoming changes in how we send notifications, this one
really becomes pretty annoying to keep working. The location where
templates are looked up are defined in the proxmox_notify crate, so
there is no easy way to mock this for testing.
The test itself seemed not super valuable, mainly testing if
the backup logs are shortened if they ware too long - so they are just
removed.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 test/Makefile                    |   6 +-
 test/vzdump_notification_test.pl | 101 -------------------------------
 2 files changed, 1 insertion(+), 106 deletions(-)
 delete mode 100755 test/vzdump_notification_test.pl

diff --git a/test/Makefile b/test/Makefile
index 62d75050..743804c8 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -5,7 +5,7 @@ all:
 export PERLLIB=..
 
 .PHONY: check
-check: test-replication test-balloon test-vzdump-notification test-vzdump test-osd
+check: test-replication test-balloon test-vzdump test-osd
 
 .PHONY: test-balloon
 test-balloon:
@@ -17,10 +17,6 @@ test-replication: replication1.t replication2.t replication3.t replication4.t re
 replication%.t: replication_test%.pl
 	./$<
 
-.PHONY: test-vzdump-notification
-test-vzdump-notification:
-	./vzdump_notification_test.pl
-
 .PHONY: test-vzdump
 test-vzdump: test-vzdump-guest-included test-vzdump-new
 
diff --git a/test/vzdump_notification_test.pl b/test/vzdump_notification_test.pl
deleted file mode 100755
index 631606bb..00000000
--- a/test/vzdump_notification_test.pl
+++ /dev/null
@@ -1,101 +0,0 @@
-#!/usr/bin/perl
-
-use strict;
-use warnings;
-
-use lib '..';
-
-use Test::More tests => 3;
-use Test::MockModule;
-
-use PVE::VZDump;
-
-my $STATUS = qr/.*status.*/;
-my $NO_LOGFILE = qr/.*Could not open log file.*/;
-my $LOG_TOO_LONG = qr/.*Log output was too long.*/;
-my $TEST_FILE_PATH       = '/tmp/mail_test';
-my $TEST_FILE_WRONG_PATH = '/tmp/mail_test_wrong';
-
-sub prepare_mail_with_status {
-    open(TEST_FILE, '>', $TEST_FILE_PATH); # Removes previous content
-    print TEST_FILE "start of log file\n";
-    print TEST_FILE "status: 0\% this should not be in the mail\n";
-    print TEST_FILE "status: 55\% this should not be in the mail\n";
-    print TEST_FILE "status: 100\% this should not be in the mail\n";
-    print TEST_FILE "end of log file\n";
-    close(TEST_FILE);
-}
-
-sub prepare_long_mail {
-    open(TEST_FILE, '>', $TEST_FILE_PATH); # Removes previous content
-    # 0.5 MB * 2 parts + the overview tables gives more than 1 MB mail
-    print TEST_FILE "a" x (1024*1024);
-    close(TEST_FILE);
-}
-
-my $result_text;
-my $result_properties;
-
-my $mock_notification_module = Test::MockModule->new('PVE::Notify');
-my $mocked_notify = sub {
-    my ($severity, $title, $text, $properties, $metadata) = @_;
-
-    $result_text = $text;
-    $result_properties = $properties;
-};
-my $mocked_notify_short = sub {
-    my (@params) = @_;
-    return $mocked_notify->('<some severity>', @params);
-};
-
-$mock_notification_module->mock(
-    'notify' => $mocked_notify,
-    'info' => $mocked_notify_short,
-    'notice' => $mocked_notify_short,
-    'warning' => $mocked_notify_short,
-    'error' => $mocked_notify_short,
-);
-$mock_notification_module->mock('cfs_read_file', sub {
-    my $path = shift;
-
-    if ($path eq 'datacenter.cfg') {
-        return {};
-    } elsif ($path eq 'notifications.cfg' || $path eq 'priv/notifications.cfg') {
-        return '';
-    } else {
-	die "unexpected cfs_read_file\n";
-    }
-});
-
-my $MAILTO = ['test_address@proxmox.com'];
-my $SELF = {
-    opts => { mailto => $MAILTO },
-    cmdline => 'test_command_on_cli',
-};
-
-my $task = { state => 'ok', vmid => '100', };
-my $tasklist;
-sub prepare_test {
-    $result_text = undef;
-    $task->{tmplog} = shift;
-    $tasklist = [ $task ];
-}
-
-{
-    prepare_test($TEST_FILE_WRONG_PATH);
-    PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
-    like($result_properties->{logs}, $NO_LOGFILE, "Missing logfile is detected");
-}
-{
-    prepare_test($TEST_FILE_PATH);
-    prepare_mail_with_status();
-    PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
-    unlike($result_properties->{"status-text"}, $STATUS, "Status are not in text part of mails");
-}
-{
-    prepare_test($TEST_FILE_PATH);
-    prepare_long_mail();
-    PVE::VZDump::send_notification($SELF, $tasklist, 0, undef, undef, undef);
-    like($result_properties->{logs}, $LOG_TOO_LONG, "Text part of mails gets shortened");
-}
-unlink $TEST_FILE_PATH;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] [PATCH manager v3 8/8] notifications: use named templates instead of in-code templates
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (6 preceding siblings ...)
  2024-05-21 13:31 ` [pve-devel] [PATCH manager v3 7/8] tests: remove vzdump_notification test Lukas Wagner
@ 2024-05-21 13:31 ` Lukas Wagner
  2024-05-22  7:00 ` [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Max Carrara
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: Lukas Wagner @ 2024-05-21 13:31 UTC (permalink / raw)
  To: pve-devel

This commit adapts notification sending for
    - package update
    - replication
    - backups

to use named templates (installed in /usr/share/pve-manager/templates)
instead of passing template strings defined in code to the
notification stack.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 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 +
 17 files changed, 95 insertions(+), 46 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

diff --git a/Makefile b/Makefile
index 28295395..337682b3 100644
--- a/Makefile
+++ b/Makefile
@@ -10,7 +10,7 @@ DSC=$(PACKAGE)_$(DEB_VERSION).dsc
 DEB=$(PACKAGE)_$(DEB_VERSION)_$(DEB_HOST_ARCH).deb
 
 DESTDIR=
-SUBDIRS = aplinfo PVE bin www services configs network-hooks test
+SUBDIRS = aplinfo PVE bin www services configs network-hooks test templates
 
 all: $(SUBDIRS)
 	set -e && for i in $(SUBDIRS); do $(MAKE) -C $$i; done
diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 19f0baca..d0e7c544 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -238,12 +238,6 @@ __PACKAGE__->register_method({
 	return $pkglist;
     }});
 
-my $updates_available_subject_template = "New software packages available ({{hostname}})";
-my $updates_available_body_template = <<EOT;
-The following updates are available:
-{{table updates}}
-EOT
-
 __PACKAGE__->register_method({
     name => 'update_database',
     path => 'update',
@@ -358,8 +352,7 @@ __PACKAGE__->register_method({
 		};
 
 		PVE::Notify::info(
-		    $updates_available_subject_template,
-		    $updates_available_body_template,
+		    "package-updates",
 		    $template_data,
 		    $metadata_fields,
 		);
diff --git a/PVE/API2/Replication.pm b/PVE/API2/Replication.pm
index 0dc944c9..d84ac1ab 100644
--- a/PVE/API2/Replication.pm
+++ b/PVE/API2/Replication.pm
@@ -92,23 +92,6 @@ my sub _should_mail_at_failcount {
     return $i * 48 == $fail_count;
 };
 
-my $replication_error_subject_template = "Replication Job: '{{job-id}}' failed";
-my $replication_error_body_template = <<EOT;
-{{#verbatim}}
-Replication job '{{job-id}}' with target '{{job-target}}' and schedule '{{job-schedule}}' failed!
-
-Last successful sync: {{timestamp last-sync}}
-Next sync try: {{timestamp next-sync}}
-Failure count: {{failure-count}}
-
-{{#if (eq failure-count 3)}}
-Note: The system  will now reduce the frequency of error reports, as the job
-appears to be stuck.
-{{/if}}
-Error:
-{{verbatim-monospaced error}}
-{{/verbatim}}
-EOT
 
 my sub _handle_job_err {
     my ($job, $err, $mail) = @_;
@@ -146,8 +129,7 @@ my sub _handle_job_err {
 
     eval {
 	PVE::Notify::error(
-	    $replication_error_subject_template,
-	    $replication_error_body_template,
+	    "replication",
 	    $template_data,
 	    $metadata_fields
 	);
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index ed3e3bfb..5b7080f3 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -477,20 +477,6 @@ my sub get_hostname {
     return $hostname;
 }
 
-my $subject_template = "vzdump backup status ({{hostname}}): {{status-text}}";
-
-my $body_template = <<EOT;
-{{error-message}}
-{{heading-1 "Details"}}
-{{table guest-table}}
-{{#verbatim}}
-Total running time: {{duration total-time}}
-Total size: {{human-bytes total-size}}
-{{/verbatim}}
-{{heading-1 "Logs"}}
-{{verbatim-monospaced logs}}
-EOT
-
 use constant MAX_LOG_SIZE => 1024*1024;
 
 sub send_notification {
@@ -588,8 +574,7 @@ sub send_notification {
 
 	    PVE::Notify::notify(
 		$severity,
-		$subject_template,
-		$body_template,
+		"vzdump",
 		$notification_props,
 		$fields,
 		$notification_config
@@ -600,8 +585,7 @@ sub send_notification {
 	# no email addresses were configured.
 	PVE::Notify::notify(
 	    $severity,
-	    $subject_template,
-	    $body_template,
+	    "vzdump",
 	    $notification_props,
 	    $fields,
 	);
diff --git a/templates/Makefile b/templates/Makefile
new file mode 100644
index 00000000..236988c5
--- /dev/null
+++ b/templates/Makefile
@@ -0,0 +1,24 @@
+NOTIFICATION_TEMPLATES=					\
+	default/test-subject.txt.hbs			\
+	default/test-body.txt.hbs			\
+	default/test-body.html.hbs			\
+	default/vzdump-subject.txt.hbs			\
+	default/vzdump-body.txt.hbs			\
+	default/vzdump-body.html.hbs			\
+	default/replication-subject.txt.hbs		\
+	default/replication-body.txt.hbs		\
+	default/replication-body.html.hbs		\
+	default/package-updates-subject.txt.hbs		\
+	default/package-updates-body.txt.hbs		\
+	default/package-updates-body.html.hbs		\
+
+all:
+
+.PHONY: install
+install:
+	install -dm 0755 $(DESTDIR)/usr/share/pve-manager/templates/default
+	$(foreach i,$(NOTIFICATION_TEMPLATES), \
+	    install -m644 $(i) $(DESTDIR)/usr/share/pve-manager/templates/$(i) ;)
+
+
+clean:
diff --git a/templates/default/package-updates-body.html.hbs b/templates/default/package-updates-body.html.hbs
new file mode 100644
index 00000000..d058e114
--- /dev/null
+++ b/templates/default/package-updates-body.html.hbs
@@ -0,0 +1,6 @@
+<html>
+    <body>
+        The following updates are available:
+        {{table updates}}
+    </body>
+</html>
diff --git a/templates/default/package-updates-body.txt.hbs b/templates/default/package-updates-body.txt.hbs
new file mode 100644
index 00000000..14bdbf9e
--- /dev/null
+++ b/templates/default/package-updates-body.txt.hbs
@@ -0,0 +1,3 @@
+The following updates are available:
+
+{{table updates}}
diff --git a/templates/default/package-updates-subject.txt.hbs b/templates/default/package-updates-subject.txt.hbs
new file mode 100644
index 00000000..556a67b8
--- /dev/null
+++ b/templates/default/package-updates-subject.txt.hbs
@@ -0,0 +1 @@
+New software packages available ({{hostname}})
diff --git a/templates/default/replication-body.html.hbs b/templates/default/replication-body.html.hbs
new file mode 100644
index 00000000..d1dea6a1
--- /dev/null
+++ b/templates/default/replication-body.html.hbs
@@ -0,0 +1,18 @@
+<html>
+    <body>
+        Replication job '{{job-id}}' with target '{{job-target}}' and schedule '{{job-schedule}}' failed!<br/><br/>
+
+        Last successful sync: {{timestamp last-sync}}<br/>
+        Next sync try: {{timestamp next-sync}}<br/>
+        Failure count: {{failure-count}}<br/>
+
+        {{#if (eq failure-count 3)}}
+            Note: The system  will now reduce the frequency of error reports, as the job appears to be stuck.
+        {{/if}}
+        <br/>
+        Error:<br/>
+        <pre>
+{{error}}
+        </pre>
+    </body>
+</html>
diff --git a/templates/default/replication-body.txt.hbs b/templates/default/replication-body.txt.hbs
new file mode 100644
index 00000000..a9273fef
--- /dev/null
+++ b/templates/default/replication-body.txt.hbs
@@ -0,0 +1,12 @@
+Replication job '{{job-id}}' with target '{{job-target}}' and schedule '{{job-schedule}}' failed!
+
+Last successful sync: {{timestamp last-sync}}
+Next sync try: {{timestamp next-sync}}
+Failure count: {{failure-count}}
+
+{{#if (eq failure-count 3)}}
+Note: The system  will now reduce the frequency of error reports, as the job
+appears to be stuck.
+{{/if}}
+Error:
+{{ error }}
diff --git a/templates/default/replication-subject.txt.hbs b/templates/default/replication-subject.txt.hbs
new file mode 100644
index 00000000..9dbaafcd
--- /dev/null
+++ b/templates/default/replication-subject.txt.hbs
@@ -0,0 +1 @@
+Replication Job: '{{job-id}}' failed
diff --git a/templates/default/test-body.html.hbs b/templates/default/test-body.html.hbs
new file mode 100644
index 00000000..26a43dde
--- /dev/null
+++ b/templates/default/test-body.html.hbs
@@ -0,0 +1 @@
+This is a test of the notification target '{{ target }}'.
diff --git a/templates/default/test-body.txt.hbs b/templates/default/test-body.txt.hbs
new file mode 100644
index 00000000..26a43dde
--- /dev/null
+++ b/templates/default/test-body.txt.hbs
@@ -0,0 +1 @@
+This is a test of the notification target '{{ target }}'.
diff --git a/templates/default/test-subject.txt.hbs b/templates/default/test-subject.txt.hbs
new file mode 100644
index 00000000..cb8e1320
--- /dev/null
+++ b/templates/default/test-subject.txt.hbs
@@ -0,0 +1 @@
+Test notification
diff --git a/templates/default/vzdump-body.html.hbs b/templates/default/vzdump-body.html.hbs
new file mode 100644
index 00000000..b6730cbb
--- /dev/null
+++ b/templates/default/vzdump-body.html.hbs
@@ -0,0 +1,11 @@
+<html>
+    <body>
+        {{error-message}}
+        <h1 style="font-size: 1.2em">Details</h1>
+        {{table guest-table}}
+        Total running time: {{duration total-time}}<br/>
+        Total size: {{human-bytes total-size}}<br/>
+        <h1 style="font-size: 1.2em">Logs</h1>
+        <pre>{{logs}}</pre>
+    </body>
+</html>
diff --git a/templates/default/vzdump-body.txt.hbs b/templates/default/vzdump-body.txt.hbs
new file mode 100644
index 00000000..90f5b0a4
--- /dev/null
+++ b/templates/default/vzdump-body.txt.hbs
@@ -0,0 +1,10 @@
+{{error-message}}
+Details
+=======
+{{table guest-table}}
+Total running time: {{duration total-time}}
+Total size: {{human-bytes total-size}}
+
+Logs
+====
+{{logs}}
diff --git a/templates/default/vzdump-subject.txt.hbs b/templates/default/vzdump-subject.txt.hbs
new file mode 100644
index 00000000..98a3d9aa
--- /dev/null
+++ b/templates/default/vzdump-subject.txt.hbs
@@ -0,0 +1 @@
+vzdump backup status ({{hostname}}): {{status-text}}
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (7 preceding siblings ...)
  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
  2024-05-29 12:29 ` Maximiliano Sandoval
  2024-06-04  9:14 ` [pve-devel] applied-series: " Wolfgang Bumiller
  10 siblings, 0 replies; 12+ messages in thread
From: Max Carrara @ 2024-05-22  7:00 UTC (permalink / raw)
  To: Proxmox VE development discussion

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


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (8 preceding siblings ...)
  2024-05-22  7:00 ` [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Max Carrara
@ 2024-05-29 12:29 ` Maximiliano Sandoval
  2024-06-04  9:14 ` [pve-devel] applied-series: " Wolfgang Bumiller
  10 siblings, 0 replies; 12+ messages in thread
From: Maximiliano Sandoval @ 2024-05-29 12:29 UTC (permalink / raw)
  To: Proxmox VE development discussion


ping.

Its not possible to build proxmox-perl-rs at the moment without this
patch series.

Lukas Wagner <l.wagner@proxmox.com> writes:

> These changes adapts the PVE notification stack to the changes introduced
> in proxmox-notify 0.4.
>
> 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)

-- 
Maximiliano


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
* [pve-devel] applied-series: [PATCH many v3 0/8] notifications: move template strings to template files
  2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Lukas Wagner
                   ` (9 preceding siblings ...)
  2024-05-29 12:29 ` Maximiliano Sandoval
@ 2024-06-04  9:14 ` Wolfgang Bumiller
  10 siblings, 0 replies; 12+ messages in thread
From: Wolfgang Bumiller @ 2024-06-04  9:14 UTC (permalink / raw)
  To: Lukas Wagner; +Cc: pve-devel

applied series & bumped packages, thanks


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-06-04  9:14 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-21 13:31 [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files 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 ` [pve-devel] [PATCH many v3 0/8] notifications: move template strings to template files Max Carrara
2024-05-29 12:29 ` Maximiliano Sandoval
2024-06-04  9:14 ` [pve-devel] applied-series: " Wolfgang Bumiller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal