* [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-19 8:14 ` Fiona Ebner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 02/19] notify: make api methods take config struct ownership Lukas Wagner
` (18 subsequent siblings)
19 siblings, 1 reply; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
Instead of passing the template strings for subject and body when
constructing a notification, we pass only the name of a template.
When rendering the template, the name of the template is used to find
corresponding template files. For PVE, they are located at
/usr/share/proxmox-ve/templates/default. The `default` part is
the 'template namespace', which is a preparation for user-customizable
and/or translatable notifications.
Previously, the same template string was used to render HTML and
plaintext notifications. This was achieved by providing some template
helpers that 'abstract away' HTML/plaintext formatting. However,
in hindsight this turned out to be pretty finicky. Since the
current changes lay the foundations for user-customizable notification
templates, I ripped these abstractions out. Now there are simply two
templates, one for plaintext, one for HTML.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@proxmox.com>
---
proxmox-notify/examples/render.rs | 63 --------
proxmox-notify/src/context/mod.rs | 10 +-
proxmox-notify/src/context/pbs.rs | 16 ++
proxmox-notify/src/context/pve.rs | 15 ++
proxmox-notify/src/context/test.rs | 9 ++
proxmox-notify/src/endpoints/gotify.rs | 9 +-
proxmox-notify/src/endpoints/sendmail.rs | 13 +-
proxmox-notify/src/endpoints/smtp.rs | 11 +-
proxmox-notify/src/lib.rs | 27 ++--
proxmox-notify/src/matcher.rs | 24 +--
proxmox-notify/src/renderer/html.rs | 14 --
proxmox-notify/src/renderer/mod.rs | 197 +++++++----------------
proxmox-notify/src/renderer/plaintext.rs | 39 -----
13 files changed, 137 insertions(+), 310 deletions(-)
delete mode 100644 proxmox-notify/examples/render.rs
diff --git a/proxmox-notify/examples/render.rs b/proxmox-notify/examples/render.rs
deleted file mode 100644
index d705fd0..0000000
--- a/proxmox-notify/examples/render.rs
+++ /dev/null
@@ -1,63 +0,0 @@
-use proxmox_notify::renderer::{render_template, TemplateRenderer};
-use proxmox_notify::Error;
-
-use serde_json::json;
-
-const TEMPLATE: &str = r#"
-{{ heading-1 "Backup Report"}}
-A backup job on host {{host}} was run.
-
-{{ heading-2 "Guests"}}
-{{ table table }}
-The total size of all backups is {{human-bytes total-size}}.
-
-The backup job took {{duration total-time}}.
-
-{{ heading-2 "Logs"}}
-{{ verbatim-monospaced logs}}
-
-{{ heading-2 "Objects"}}
-{{ object table }}
-"#;
-
-fn main() -> Result<(), Error> {
- let properties = json!({
- "host": "pali",
- "logs": "100: starting backup\n100: backup failed",
- "total-size": 1024 * 1024 + 2048 * 1024,
- "total-time": 100,
- "table": {
- "schema": {
- "columns": [
- {
- "label": "VMID",
- "id": "vmid"
- },
- {
- "label": "Size",
- "id": "size",
- "renderer": "human-bytes"
- }
- ],
- },
- "data" : [
- {
- "vmid": 1001,
- "size": "1048576"
- },
- {
- "vmid": 1002,
- "size": 2048 * 1024,
- }
- ]
- }
- });
-
- let output = render_template(TemplateRenderer::Html, TEMPLATE, &properties)?;
- println!("{output}");
-
- let output = render_template(TemplateRenderer::Plaintext, TEMPLATE, &properties)?;
- println!("{output}");
-
- Ok(())
-}
diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
index cc68603..c0a5a13 100644
--- a/proxmox-notify/src/context/mod.rs
+++ b/proxmox-notify/src/context/mod.rs
@@ -1,6 +1,8 @@
use std::fmt::Debug;
use std::sync::Mutex;
+use crate::Error;
+
#[cfg(any(feature = "pve-context", feature = "pbs-context"))]
pub mod common;
#[cfg(feature = "pbs-context")]
@@ -20,8 +22,14 @@ pub trait Context: Send + Sync + Debug {
fn default_sendmail_from(&self) -> String;
/// Proxy configuration for the current node
fn http_proxy_config(&self) -> Option<String>;
- // Return default config for built-in targets/matchers.
+ /// Return default config for built-in targets/matchers.
fn default_config(&self) -> &'static str;
+ /// Lookup a template in a certain (optional) namespace
+ fn lookup_template(
+ &self,
+ filename: &str,
+ namespace: Option<&str>,
+ ) -> Result<Option<String>, Error>;
}
#[cfg(not(test))]
diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
index 5b97af7..70e993f 100644
--- a/proxmox-notify/src/context/pbs.rs
+++ b/proxmox-notify/src/context/pbs.rs
@@ -1,9 +1,11 @@
use serde::Deserialize;
+use std::path::Path;
use proxmox_schema::{ObjectSchema, Schema, StringSchema};
use proxmox_section_config::{SectionConfig, SectionConfigPlugin};
use crate::context::{common, Context};
+use crate::Error;
const PBS_USER_CFG_FILENAME: &str = "/etc/proxmox-backup/user.cfg";
const PBS_NODE_CFG_FILENAME: &str = "/etc/proxmox-backup/node.cfg";
@@ -98,6 +100,20 @@ impl Context for PBSContext {
fn default_config(&self) -> &'static str {
return DEFAULT_CONFIG;
}
+
+ fn lookup_template(
+ &self,
+ filename: &str,
+ namespace: Option<&str>,
+ ) -> Result<Option<String>, Error> {
+ let path = Path::new("/usr/share/proxmox-backup/templates")
+ .join(namespace.unwrap_or("default"))
+ .join(filename);
+
+ let template_string = proxmox_sys::fs::file_read_optional_string(path)
+ .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
+ Ok(template_string)
+ }
}
#[cfg(test)]
diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
index 39e0e4a..321f6f9 100644
--- a/proxmox-notify/src/context/pve.rs
+++ b/proxmox-notify/src/context/pve.rs
@@ -1,4 +1,6 @@
use crate::context::{common, Context};
+use crate::Error;
+use std::path::Path;
fn lookup_mail_address(content: &str, user: &str) -> Option<String> {
common::normalize_for_return(content.lines().find_map(|line| {
@@ -51,6 +53,19 @@ impl Context for PVEContext {
fn default_config(&self) -> &'static str {
return DEFAULT_CONFIG;
}
+
+ fn lookup_template(
+ &self,
+ filename: &str,
+ namespace: Option<&str>,
+ ) -> Result<Option<String>, Error> {
+ let path = Path::new("/usr/share/proxmox-ve/templates")
+ .join(namespace.unwrap_or("default"))
+ .join(filename);
+ let template_string = proxmox_sys::fs::file_read_optional_string(path)
+ .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
+ Ok(template_string)
+ }
}
pub static PVE_CONTEXT: PVEContext = PVEContext;
diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs
index 61f794a..5df25d0 100644
--- a/proxmox-notify/src/context/test.rs
+++ b/proxmox-notify/src/context/test.rs
@@ -1,4 +1,5 @@
use crate::context::Context;
+use crate::Error;
#[derive(Debug)]
pub struct TestContext;
@@ -23,4 +24,12 @@ impl Context for TestContext {
fn default_config(&self) -> &'static str {
""
}
+
+ fn lookup_template(
+ &self,
+ _filename: &str,
+ _namespace: Option<&str>,
+ ) -> Result<Option<String>, Error> {
+ Ok(Some(String::new()))
+ }
}
diff --git a/proxmox-notify/src/endpoints/gotify.rs b/proxmox-notify/src/endpoints/gotify.rs
index 90ae959..4c1f9e0 100644
--- a/proxmox-notify/src/endpoints/gotify.rs
+++ b/proxmox-notify/src/endpoints/gotify.rs
@@ -9,7 +9,7 @@ use proxmox_schema::api_types::COMMENT_SCHEMA;
use proxmox_schema::{api, Updater};
use crate::context::context;
-use crate::renderer::TemplateRenderer;
+use crate::renderer::TemplateType;
use crate::schema::ENTITY_NAME_SCHEMA;
use crate::{renderer, Content, Endpoint, Error, Notification, Origin, Severity};
@@ -92,14 +92,13 @@ impl Endpoint for GotifyEndpoint {
fn send(&self, notification: &Notification) -> Result<(), Error> {
let (title, message) = match ¬ification.content {
Content::Template {
- title_template,
- body_template,
+ template_name,
data,
} => {
let rendered_title =
- renderer::render_template(TemplateRenderer::Plaintext, title_template, data)?;
+ renderer::render_template(TemplateType::Subject, template_name, data)?;
let rendered_message =
- renderer::render_template(TemplateRenderer::Plaintext, body_template, data)?;
+ renderer::render_template(TemplateType::PlaintextBody, template_name, data)?;
(rendered_title, rendered_message)
}
diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index 4fc92b4..6178b5e 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -3,9 +3,9 @@ use serde::{Deserialize, Serialize};
use proxmox_schema::api_types::COMMENT_SCHEMA;
use proxmox_schema::{api, Updater};
-use crate::context::context;
+use crate::context;
use crate::endpoints::common::mail;
-use crate::renderer::TemplateRenderer;
+use crate::renderer::TemplateType;
use crate::schema::{EMAIL_SCHEMA, ENTITY_NAME_SCHEMA, USER_SCHEMA};
use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
@@ -103,16 +103,15 @@ impl Endpoint for SendmailEndpoint {
match ¬ification.content {
Content::Template {
- title_template,
- body_template,
+ template_name,
data,
} => {
let subject =
- renderer::render_template(TemplateRenderer::Plaintext, title_template, data)?;
+ renderer::render_template(TemplateType::Subject, template_name, data)?;
let html_part =
- renderer::render_template(TemplateRenderer::Html, body_template, data)?;
+ renderer::render_template(TemplateType::HtmlBody, template_name, data)?;
let text_part =
- renderer::render_template(TemplateRenderer::Plaintext, body_template, data)?;
+ renderer::render_template(TemplateType::PlaintextBody, template_name, data)?;
let author = self
.config
diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
index 5470257..f0c836a 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -11,7 +11,7 @@ use proxmox_schema::{api, Updater};
use crate::context::context;
use crate::endpoints::common::mail;
-use crate::renderer::TemplateRenderer;
+use crate::renderer::TemplateType;
use crate::schema::{EMAIL_SCHEMA, ENTITY_NAME_SCHEMA, USER_SCHEMA};
use crate::{renderer, Content, Endpoint, Error, Notification, Origin};
@@ -202,16 +202,15 @@ impl Endpoint for SmtpEndpoint {
let mut email = match ¬ification.content {
Content::Template {
- title_template,
- body_template,
+ template_name,
data,
} => {
let subject =
- renderer::render_template(TemplateRenderer::Plaintext, title_template, data)?;
+ renderer::render_template(TemplateType::Subject, template_name, data)?;
let html_part =
- renderer::render_template(TemplateRenderer::Html, body_template, data)?;
+ renderer::render_template(TemplateType::HtmlBody, template_name, data)?;
let text_part =
- renderer::render_template(TemplateRenderer::Plaintext, body_template, data)?;
+ renderer::render_template(TemplateType::PlaintextBody, template_name, data)?;
email_builder = email_builder.subject(subject);
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index ee1e445..9f8683f 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -162,10 +162,8 @@ pub trait Endpoint {
pub enum Content {
/// Title and body will be rendered as a template
Template {
- /// Template for the notification title.
- title_template: String,
- /// Template for the notification body.
- body_template: String,
+ /// Name of the used template
+ template_name: String,
/// Data that can be used for template rendering.
data: Value,
},
@@ -203,10 +201,9 @@ pub struct Notification {
}
impl Notification {
- pub fn new_templated<S: AsRef<str>>(
+ pub fn from_template<S: AsRef<str>>(
severity: Severity,
- title: S,
- body: S,
+ template_name: S,
template_data: Value,
fields: HashMap<String, String>,
) -> Self {
@@ -217,8 +214,7 @@ impl Notification {
timestamp: proxmox_time::epoch_i64(),
},
content: Content::Template {
- title_template: title.as_ref().to_string(),
- body_template: body.as_ref().to_string(),
+ template_name: template_name.as_ref().to_string(),
data: template_data,
},
}
@@ -549,8 +545,7 @@ impl Bus {
timestamp: proxmox_time::epoch_i64(),
},
content: Content::Template {
- title_template: "Test notification".into(),
- body_template: "This is a test of the notification target '{{ target }}'".into(),
+ template_name: "test".to_string(),
data: json!({ "target": target }),
},
};
@@ -623,10 +618,9 @@ mod tests {
bus.add_matcher(matcher);
// Send directly to endpoint
- bus.send(&Notification::new_templated(
+ bus.send(&Notification::from_template(
Severity::Info,
- "Title",
- "Body",
+ "test",
Default::default(),
Default::default(),
));
@@ -661,10 +655,9 @@ mod tests {
});
let send_with_severity = |severity| {
- let notification = Notification::new_templated(
+ let notification = Notification::from_template(
severity,
- "Title",
- "Body",
+ "test",
Default::default(),
Default::default(),
);
diff --git a/proxmox-notify/src/matcher.rs b/proxmox-notify/src/matcher.rs
index 0f86445..d6051ac 100644
--- a/proxmox-notify/src/matcher.rs
+++ b/proxmox-notify/src/matcher.rs
@@ -456,7 +456,7 @@ mod tests {
fields.insert("foo".into(), "bar".into());
let notification =
- Notification::new_templated(Severity::Notice, "test", "test", Value::Null, fields);
+ Notification::from_template(Severity::Notice, "test", Value::Null, fields);
let matcher: FieldMatcher = "exact:foo=bar".parse().unwrap();
assert!(matcher.matches(¬ification).unwrap());
@@ -474,14 +474,14 @@ mod tests {
fields.insert("foo".into(), "test".into());
let notification =
- Notification::new_templated(Severity::Notice, "test", "test", Value::Null, fields);
+ Notification::from_template(Severity::Notice, "test", Value::Null, fields);
assert!(matcher.matches(¬ification).unwrap());
let mut fields = HashMap::new();
fields.insert("foo".into(), "notthere".into());
let notification =
- Notification::new_templated(Severity::Notice, "test", "test", Value::Null, fields);
+ Notification::from_template(Severity::Notice, "test", Value::Null, fields);
assert!(!matcher.matches(¬ification).unwrap());
assert!("regex:'3=b.*".parse::<FieldMatcher>().is_err());
@@ -489,13 +489,8 @@ mod tests {
}
#[test]
fn test_severities() {
- let notification = Notification::new_templated(
- Severity::Notice,
- "test",
- "test",
- Value::Null,
- Default::default(),
- );
+ let notification =
+ Notification::from_template(Severity::Notice, "test", Value::Null, Default::default());
let matcher: SeverityMatcher = "info,notice,warning,error".parse().unwrap();
assert!(matcher.matches(¬ification).unwrap());
@@ -503,13 +498,8 @@ mod tests {
#[test]
fn test_empty_matcher_matches_always() {
- let notification = Notification::new_templated(
- Severity::Notice,
- "test",
- "test",
- Value::Null,
- Default::default(),
- );
+ let notification =
+ Notification::from_template(Severity::Notice, "test", Value::Null, Default::default());
for mode in [MatchModeOperator::All, MatchModeOperator::Any] {
let config = MatcherConfig {
diff --git a/proxmox-notify/src/renderer/html.rs b/proxmox-notify/src/renderer/html.rs
index f794902..77545f7 100644
--- a/proxmox-notify/src/renderer/html.rs
+++ b/proxmox-notify/src/renderer/html.rs
@@ -5,7 +5,6 @@ use handlebars::{
use serde_json::Value;
use super::{table::Table, value_to_string};
-use crate::define_helper_with_prefix_and_postfix;
use crate::renderer::BlockRenderFunctions;
fn render_html_table(
@@ -79,22 +78,9 @@ fn render_object(
Ok(())
}
-define_helper_with_prefix_and_postfix!(verbatim_monospaced, "<pre>", "</pre>");
-define_helper_with_prefix_and_postfix!(heading_1, "<h1 style=\"font-size: 1.2em\">", "</h1>");
-define_helper_with_prefix_and_postfix!(heading_2, "<h2 style=\"font-size: 1em\">", "</h2>");
-define_helper_with_prefix_and_postfix!(
- verbatim,
- "<pre style=\"font-family: sans-serif\">",
- "</pre>"
-);
-
pub(super) fn block_render_functions() -> BlockRenderFunctions {
BlockRenderFunctions {
table: Box::new(render_html_table),
- verbatim_monospaced: Box::new(verbatim_monospaced),
object: Box::new(render_object),
- heading_1: Box::new(heading_1),
- heading_2: Box::new(heading_2),
- verbatim: Box::new(verbatim),
}
}
diff --git a/proxmox-notify/src/renderer/mod.rs b/proxmox-notify/src/renderer/mod.rs
index e9f36e6..a51ece6 100644
--- a/proxmox-notify/src/renderer/mod.rs
+++ b/proxmox-notify/src/renderer/mod.rs
@@ -12,7 +12,7 @@ use serde_json::Value;
use proxmox_human_byte::HumanByte;
use proxmox_time::TimeSpan;
-use crate::Error;
+use crate::{context, Error};
mod html;
mod plaintext;
@@ -165,41 +165,47 @@ impl ValueRenderFunction {
}
}
-/// Available renderers for notification templates.
+/// Available template types
#[derive(Copy, Clone)]
-pub enum TemplateRenderer {
- /// Render to HTML code
- Html,
- /// Render to plain text
- Plaintext,
+pub enum TemplateType {
+ /// HTML body template
+ HtmlBody,
+ /// Plaintext body template
+ PlaintextBody,
+ /// Plaintext body template
+ Subject,
}
-impl TemplateRenderer {
- fn prefix(&self) -> &str {
+impl TemplateType {
+ fn file_suffix(&self) -> &'static str {
match self {
- TemplateRenderer::Html => "<html>\n<body>\n",
- TemplateRenderer::Plaintext => "",
+ TemplateType::HtmlBody => "body.html.hbs",
+ TemplateType::PlaintextBody => "body.txt.hbs",
+ TemplateType::Subject => "subject.txt.hbs",
}
}
- fn postfix(&self) -> &str {
- match self {
- TemplateRenderer::Html => "\n</body>\n</html>",
- TemplateRenderer::Plaintext => "",
+ fn postprocess(&self, mut rendered: String) -> String {
+ if let Self::Subject = self {
+ rendered = rendered.replace('\n', " ");
}
+
+ rendered
}
fn block_render_fns(&self) -> BlockRenderFunctions {
match self {
- TemplateRenderer::Html => html::block_render_functions(),
- TemplateRenderer::Plaintext => plaintext::block_render_functions(),
+ TemplateType::HtmlBody => html::block_render_functions(),
+ TemplateType::Subject => plaintext::block_render_functions(),
+ TemplateType::PlaintextBody => plaintext::block_render_functions(),
}
}
fn escape_fn(&self) -> fn(&str) -> String {
match self {
- TemplateRenderer::Html => handlebars::html_escape,
- TemplateRenderer::Plaintext => handlebars::no_escape,
+ TemplateType::PlaintextBody => handlebars::no_escape,
+ TemplateType::Subject => handlebars::no_escape,
+ TemplateType::HtmlBody => handlebars::html_escape,
}
}
}
@@ -208,28 +214,20 @@ type HelperFn = dyn HelperDef + Send + Sync;
struct BlockRenderFunctions {
table: Box<HelperFn>,
- verbatim_monospaced: Box<HelperFn>,
object: Box<HelperFn>,
- heading_1: Box<HelperFn>,
- heading_2: Box<HelperFn>,
- verbatim: Box<HelperFn>,
}
impl BlockRenderFunctions {
fn register_helpers(self, handlebars: &mut Handlebars) {
handlebars.register_helper("table", self.table);
- handlebars.register_helper("verbatim", self.verbatim);
- handlebars.register_helper("verbatim-monospaced", self.verbatim_monospaced);
handlebars.register_helper("object", self.object);
- handlebars.register_helper("heading-1", self.heading_1);
- handlebars.register_helper("heading-2", self.heading_2);
}
}
fn render_template_impl(
template: &str,
data: &Value,
- renderer: TemplateRenderer,
+ renderer: TemplateType,
) -> Result<String, Error> {
let mut handlebars = Handlebars::new();
handlebars.register_escape_fn(renderer.escape_fn());
@@ -248,61 +246,45 @@ fn render_template_impl(
/// Render a template string.
///
-/// The output format can be chosen via the `renderer` parameter (see [TemplateRenderer]
+/// The output format can be chosen via the `renderer` parameter (see [TemplateType]
/// for available options).
pub fn render_template(
- renderer: TemplateRenderer,
+ mut ty: TemplateType,
template: &str,
data: &Value,
) -> Result<String, Error> {
- let mut rendered_template = String::from(renderer.prefix());
+ let filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
+
+ let template_string = context::context().lookup_template(&filename, None)?;
+
+ let (template_string, fallback) = match (template_string, ty) {
+ (None, TemplateType::HtmlBody) => {
+ ty = TemplateType::PlaintextBody;
+ let plaintext_filename = format!("{template}-{suffix}", suffix = ty.file_suffix());
+ log::info!("html template '{filename}' not found, falling back to plain text template '{plaintext_filename}'");
+ (
+ context::context().lookup_template(&plaintext_filename, None)?,
+ true,
+ )
+ }
+ (template_string, _) => (template_string, false),
+ };
- rendered_template.push_str(&render_template_impl(template, data, renderer)?);
- rendered_template.push_str(renderer.postfix());
+ let template_string = template_string.ok_or(Error::Generic(format!(
+ "could not load template '{template}'"
+ )))?;
- Ok(rendered_template)
-}
+ let mut rendered = render_template_impl(&template_string, data, ty)?;
+ rendered = ty.postprocess(rendered);
-#[macro_export]
-macro_rules! define_helper_with_prefix_and_postfix {
- ($name:ident, $pre:expr, $post:expr) => {
- fn $name<'reg, 'rc>(
- h: &Helper<'reg, 'rc>,
- handlebars: &'reg Handlebars,
- context: &'rc Context,
- render_context: &mut RenderContext<'reg, 'rc>,
- out: &mut dyn Output,
- ) -> HelperResult {
- use handlebars::Renderable;
-
- let block_text = h.template();
- let param = h.param(0);
-
- out.write($pre)?;
- match (param, block_text) {
- (None, Some(block_text)) => {
- block_text.render(handlebars, context, render_context, out)
- }
- (Some(param), None) => {
- let value = param.value();
- let text = value.as_str().ok_or_else(|| {
- HandlebarsRenderError::new(format!("value {value} is not a string"))
- })?;
+ if fallback {
+ rendered = format!(
+ "<html><body><pre>{}</pre></body></html>",
+ handlebars::html_escape(&rendered)
+ );
+ }
- out.write(text)?;
- Ok(())
- }
- (Some(_), Some(_)) => Err(HandlebarsRenderError::new(
- "Cannot use parameter and template at the same time",
- )),
- (None, None) => Err(HandlebarsRenderError::new(
- "Neither parameter nor template was provided",
- )),
- }?;
- out.write($post)?;
- Ok(())
- }
- };
+ Ok(rendered)
}
#[cfg(test)]
@@ -310,73 +292,6 @@ mod tests {
use super::*;
use serde_json::json;
- #[test]
- fn test_render_template() -> Result<(), Error> {
- let data = json!({
- "dur": 12345,
- "size": 1024 * 15,
-
- "table": {
- "schema": {
- "columns": [
- {
- "id": "col1",
- "label": "Column 1"
- },
- {
- "id": "col2",
- "label": "Column 2"
- }
- ]
- },
- "data": [
- {
- "col1": "val1",
- "col2": "val2"
- },
- {
- "col1": "val3",
- "col2": "val4"
- },
- ]
- }
-
- });
-
- let template = r#"
-{{heading-1 "Hello World"}}
-
-{{heading-2 "Hello World"}}
-
-{{human-bytes size}}
-{{duration dur}}
-
-{{table table}}"#;
-
- let expected_plaintext = r#"
-Hello World
-===========
-
-Hello World
------------
-
-15 KiB
-3h 25min 45s
-
-Column 1 Column 2
-val1 val2
-val3 val4
-"#;
-
- let rendered_plaintext = render_template(TemplateRenderer::Plaintext, template, &data)?;
-
- // Let's not bother about testing the HTML output, too fragile.
-
- assert_eq!(rendered_plaintext, expected_plaintext);
-
- Ok(())
- }
-
#[test]
fn test_helpers() {
assert_eq!(value_to_byte_size(&json!(1024)), Some("1 KiB".to_string()));
diff --git a/proxmox-notify/src/renderer/plaintext.rs b/proxmox-notify/src/renderer/plaintext.rs
index 437e412..59e917a 100644
--- a/proxmox-notify/src/renderer/plaintext.rs
+++ b/proxmox-notify/src/renderer/plaintext.rs
@@ -7,7 +7,6 @@ use handlebars::{
use serde_json::Value;
use super::{table::Table, value_to_string};
-use crate::define_helper_with_prefix_and_postfix;
use crate::renderer::BlockRenderFunctions;
fn optimal_column_widths(table: &Table) -> HashMap<&str, usize> {
@@ -76,40 +75,6 @@ fn render_plaintext_table(
Ok(())
}
-macro_rules! define_underlining_heading_fn {
- ($name:ident, $underline:expr) => {
- fn $name<'reg, 'rc>(
- h: &Helper<'reg, 'rc>,
- _handlebars: &'reg Handlebars,
- _context: &'rc Context,
- _render_context: &mut RenderContext<'reg, 'rc>,
- out: &mut dyn Output,
- ) -> HelperResult {
- let param = h
- .param(0)
- .ok_or_else(|| HandlebarsRenderError::new("No parameter provided"))?;
-
- let value = param.value();
- let text = value.as_str().ok_or_else(|| {
- HandlebarsRenderError::new(format!("value {value} is not a string"))
- })?;
-
- out.write(text)?;
- out.write("\n")?;
-
- for _ in 0..text.len() {
- out.write($underline)?;
- }
- Ok(())
- }
- };
-}
-
-define_helper_with_prefix_and_postfix!(verbatim_monospaced, "", "");
-define_underlining_heading_fn!(heading_1, "=");
-define_underlining_heading_fn!(heading_2, "-");
-define_helper_with_prefix_and_postfix!(verbatim, "", "");
-
fn render_object(
h: &Helper,
_: &Handlebars,
@@ -133,10 +98,6 @@ fn render_object(
pub(super) fn block_render_functions() -> BlockRenderFunctions {
BlockRenderFunctions {
table: Box::new(render_plaintext_table),
- verbatim_monospaced: Box::new(verbatim_monospaced),
- verbatim: Box::new(verbatim),
object: Box::new(render_object),
- heading_1: Box::new(heading_1),
- heading_2: Box::new(heading_2),
}
}
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system Lukas Wagner
@ 2024-04-19 8:14 ` Fiona Ebner
2024-04-19 8:45 ` Lukas Wagner
0 siblings, 1 reply; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 8:14 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> Instead of passing the template strings for subject and body when
> constructing a notification, we pass only the name of a template.
> When rendering the template, the name of the template is used to find
> corresponding template files. For PVE, they are located at
> /usr/share/proxmox-ve/templates/default. The `default` part is
> the 'template namespace', which is a preparation for user-customizable
> and/or translatable notifications.
>
Is the plan to create different namespaces there ourselves or tell users
they can put their custom templates there? In the latter case, I'm not
sure /usr/share is the best place, rather than some place under /etc/
Who adds the template files? I don't see a patch for proxmox-ve in this
series. Does this series require some versioned breaks to some package?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system
2024-04-19 8:14 ` Fiona Ebner
@ 2024-04-19 8:45 ` Lukas Wagner
2024-04-19 8:57 ` Fiona Ebner
0 siblings, 1 reply; 38+ messages in thread
From: Lukas Wagner @ 2024-04-19 8:45 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2024-04-19 10:14, Fiona Ebner wrote:
> Am 09.04.24 um 15:25 schrieb Lukas Wagner:
>> Instead of passing the template strings for subject and body when
>> constructing a notification, we pass only the name of a template.
>> When rendering the template, the name of the template is used to find
>> corresponding template files. For PVE, they are located at
>> /usr/share/proxmox-ve/templates/default. The `default` part is
>> the 'template namespace', which is a preparation for user-customizable
>> and/or translatable notifications.
>>
>
> Is the plan to create different namespaces there ourselves or tell users
> they can put their custom templates there? In the latter case, I'm not
> sure /usr/share is the best place, rather than some place under /etc/
The idea would be to implement translations as other namespaces, e.g. `de` or `fr`
instead of `default`.
For user-overridable templates we would extend the implementation to search for a template
in another location first and then fall back to the templates provided by us in '/usr/share/...`
I have not made up my mind yet on where these user-provided templates would be located, either
in /usr/local/share/.... or somewhere in /etc (/etc/pve would ensure that we have the same templates
on all nodes, but I'm not sure if it is a good idea to put custom, user-created files in there...)
Combining both ideas: assuming that we want to render a fencing notification translated to German, assuming
that the user-override is in /usr/local/share:
- First try to load /usr/local/share/proxmox-ve/templates/de/fencing.txt.hbs
- If not found, try loading the shipped template at /usr/share/proxmox-ve/templates/de/...
- If that one also does not exist, fall back to `default` namespace
- first user-location
- finally shipped template
>
> Who adds the template files? I don't see a patch for proxmox-ve in this
> series. Does this series require some versioned breaks to some package?
The pve-manager and pve-ha-manager (for fencing notifications) patches add the templates.
I can't use `/usr/share/pve-manager` and `/usr/share/pve-ha-manager` because
proxmox_notify needs to have a single base directory from where to load template files.
Maybe we should use some other base dir to avoid confusion with the `proxmox-ve` metapackage?
In terms of versions:
pve-{ha}-manager needs to pull in a bumped libpve-notify-perl
libpve-notify-perl needs to pull in bumped libpve-rs-perl/libproxmox-rs-perl
libpve-rs-perl needs to pull in bumped librust-proxmox-notify
I really wish the dep-chain was a bit easier, yet here we are.
--
- Lukas
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system
2024-04-19 8:45 ` Lukas Wagner
@ 2024-04-19 8:57 ` Fiona Ebner
2024-04-19 9:31 ` Lukas Wagner
0 siblings, 1 reply; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 8:57 UTC (permalink / raw)
To: Lukas Wagner, Proxmox VE development discussion
Am 19.04.24 um 10:45 schrieb Lukas Wagner:
>> Who adds the template files? I don't see a patch for proxmox-ve in this
>> series. Does this series require some versioned breaks to some package?
>
> The pve-manager and pve-ha-manager (for fencing notifications) patches add the templates.
> I can't use `/usr/share/pve-manager` and `/usr/share/pve-ha-manager` because
> proxmox_notify needs to have a single base directory from where to load template files.
> Maybe we should use some other base dir to avoid confusion with the `proxmox-ve` metapackage?
>
Ah, I see. Yes, maybe a directory named based on libpve-notify-perl
would be better or proxmox-notify (but would need to be a bit careful
with co-installed PBS and PVE to not create accidental conflicts).
> In terms of versions:
> pve-{ha}-manager needs to pull in a bumped libpve-notify-perl
> libpve-notify-perl needs to pull in bumped libpve-rs-perl/libproxmox-rs-perl
> libpve-rs-perl needs to pull in bumped librust-proxmox-notify
>
> I really wish the dep-chain was a bit easier, yet here we are.
>
But there also is a need for versioned breaks, right? Because installing
new libpve-notify-perl (using new proxmox-perl-rs using new
proxmox-notify) without also upgrading pve-manager and pve-ha-manager
will be broken or am I missing something?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system
2024-04-19 8:57 ` Fiona Ebner
@ 2024-04-19 9:31 ` Lukas Wagner
0 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-19 9:31 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2024-04-19 10:57, Fiona Ebner wrote:
> Am 19.04.24 um 10:45 schrieb Lukas Wagner:
>>> Who adds the template files? I don't see a patch for proxmox-ve in this
>>> series. Does this series require some versioned breaks to some package?
>>
>> The pve-manager and pve-ha-manager (for fencing notifications) patches add the templates.
>> I can't use `/usr/share/pve-manager` and `/usr/share/pve-ha-manager` because
>> proxmox_notify needs to have a single base directory from where to load template files.
>> Maybe we should use some other base dir to avoid confusion with the `proxmox-ve` metapackage?
>>>
> Ah, I see. Yes, maybe a directory named based on libpve-notify-perl
> would be better or proxmox-notify (but would need to be a bit careful
> with co-installed PBS and PVE to not create accidental conflicts).
mhmm. In PBS we install the templates in /usr/share/proxmox-backup/... (there it makes sense,
because the templates are part of the .deb with the same name).
Using `proxmox-notify` would IMO be not so good. We might have similar notification templates in
both products (e.g. package-updates) which are not necessarily 100% compatible.
Of course we could unify them, but I kinda prefer the flexibility of
having these separate.
A name based on libpve-notify implies that the templates belong to
PVE, which is good. However, that only shifts the problem:
pve-{ha-,}manager still install templates to a location which appears
to be owned by another package, this time libpve-notify...
Thinking about the options, and also about 'user experience' when
adding overridable templates, I think we could just keep on
using 'proxmox-ve' here.
>
>> In terms of versions:
>> pve-{ha}-manager needs to pull in a bumped libpve-notify-perl
>> libpve-notify-perl needs to pull in bumped libpve-rs-perl/libproxmox-rs-perl
>> libpve-rs-perl needs to pull in bumped librust-proxmox-notify
>>
>> I really wish the dep-chain was a bit easier, yet here we are.
>>
>
> But there also is a need for versioned breaks, right? Because installing
> new libpve-notify-perl (using new proxmox-perl-rs using new
> proxmox-notify) without also upgrading pve-manager and pve-ha-manager
> will be broken or am I missing something?
Ah yes, sorry, my packaging knowledge is not yet the best.
A versioned break will be necessary due to the API changes (passing a template name
instead of a title/body) and due to the fact that the template
files must be present.
--
- Lukas
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [pve-devel] [PATCH proxmox 02/19] notify: make api methods take config struct ownership
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 03/19] notify: convert Option<Vec<T>> -> Vec<T> in config structs Lukas Wagner
` (17 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
This saves us from some of the awkward cloning steps when updating.
Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@proxmox.com>
---
proxmox-notify/src/api/gotify.rs | 46 +++++++++---------
proxmox-notify/src/api/matcher.rs | 38 +++++++--------
proxmox-notify/src/api/mod.rs | 14 +++---
proxmox-notify/src/api/sendmail.rs | 40 +++++++--------
proxmox-notify/src/api/smtp.rs | 78 +++++++++++++++---------------
5 files changed, 108 insertions(+), 108 deletions(-)
diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs
index a93a024..15d94cb 100644
--- a/proxmox-notify/src/api/gotify.rs
+++ b/proxmox-notify/src/api/gotify.rs
@@ -41,8 +41,8 @@ pub fn get_endpoint(config: &Config, name: &str) -> Result<GotifyConfig, HttpErr
/// Panics if the names of the private config and the public config do not match.
pub fn add_endpoint(
config: &mut Config,
- endpoint_config: &GotifyConfig,
- private_endpoint_config: &GotifyPrivateConfig,
+ endpoint_config: GotifyConfig,
+ private_endpoint_config: GotifyPrivateConfig,
) -> Result<(), HttpError> {
if endpoint_config.name != private_endpoint_config.name {
// Programming error by the user of the crate, thus we panic
@@ -51,11 +51,11 @@ pub fn add_endpoint(
super::ensure_unique(config, &endpoint_config.name)?;
- set_private_config_entry(config, private_endpoint_config)?;
+ set_private_config_entry(config, &private_endpoint_config)?;
config
.config
- .set_data(&endpoint_config.name, GOTIFY_TYPENAME, endpoint_config)
+ .set_data(&endpoint_config.name, GOTIFY_TYPENAME, &endpoint_config)
.map_err(|e| {
http_err!(
INTERNAL_SERVER_ERROR,
@@ -75,8 +75,8 @@ pub fn add_endpoint(
pub fn update_endpoint(
config: &mut Config,
name: &str,
- endpoint_config_updater: &GotifyConfigUpdater,
- private_endpoint_config_updater: &GotifyPrivateConfigUpdater,
+ endpoint_config_updater: GotifyConfigUpdater,
+ private_endpoint_config_updater: GotifyPrivateConfigUpdater,
delete: Option<&[DeleteableGotifyProperty]>,
digest: Option<&[u8]>,
) -> Result<(), HttpError> {
@@ -93,11 +93,11 @@ pub fn update_endpoint(
}
}
- if let Some(server) = &endpoint_config_updater.server {
- endpoint.server = server.into();
+ if let Some(server) = endpoint_config_updater.server {
+ endpoint.server = server;
}
- if let Some(token) = &private_endpoint_config_updater.token {
+ if let Some(token) = private_endpoint_config_updater.token {
set_private_config_entry(
config,
&GotifyPrivateConfig {
@@ -107,12 +107,12 @@ pub fn update_endpoint(
)?;
}
- if let Some(comment) = &endpoint_config_updater.comment {
- endpoint.comment = Some(comment.into());
+ if let Some(comment) = endpoint_config_updater.comment {
+ endpoint.comment = Some(comment)
}
- if let Some(disable) = &endpoint_config_updater.disable {
- endpoint.disable = Some(*disable);
+ if let Some(disable) = endpoint_config_updater.disable {
+ endpoint.disable = Some(disable);
}
config
@@ -173,13 +173,13 @@ mod tests {
pub fn add_default_gotify_endpoint(config: &mut Config) -> Result<(), HttpError> {
add_endpoint(
config,
- &GotifyConfig {
+ GotifyConfig {
name: "gotify-endpoint".into(),
server: "localhost".into(),
comment: Some("comment".into()),
..Default::default()
},
- &GotifyPrivateConfig {
+ GotifyPrivateConfig {
name: "gotify-endpoint".into(),
token: "supersecrettoken".into(),
},
@@ -196,8 +196,8 @@ mod tests {
assert!(update_endpoint(
&mut config,
"test",
- &Default::default(),
- &Default::default(),
+ Default::default(),
+ Default::default(),
None,
None
)
@@ -214,8 +214,8 @@ mod tests {
assert!(update_endpoint(
&mut config,
"gotify-endpoint",
- &Default::default(),
- &Default::default(),
+ Default::default(),
+ Default::default(),
None,
Some(&[0; 32])
)
@@ -234,12 +234,12 @@ mod tests {
update_endpoint(
&mut config,
"gotify-endpoint",
- &GotifyConfigUpdater {
+ GotifyConfigUpdater {
server: Some("newhost".into()),
comment: Some("newcomment".into()),
..Default::default()
},
- &GotifyPrivateConfigUpdater {
+ GotifyPrivateConfigUpdater {
token: Some("changedtoken".into()),
},
None,
@@ -263,8 +263,8 @@ mod tests {
update_endpoint(
&mut config,
"gotify-endpoint",
- &Default::default(),
- &Default::default(),
+ Default::default(),
+ Default::default(),
Some(&[DeleteableGotifyProperty::Comment]),
None,
)?;
diff --git a/proxmox-notify/src/api/matcher.rs b/proxmox-notify/src/api/matcher.rs
index ca01bc9..f0eabd9 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -36,7 +36,7 @@ pub fn get_matcher(config: &Config, name: &str) -> Result<MatcherConfig, HttpErr
/// Returns a `HttpError` if:
/// - an entity with the same name already exists (`400 Bad request`)
/// - the configuration could not be saved (`500 Internal server error`)
-pub fn add_matcher(config: &mut Config, matcher_config: &MatcherConfig) -> Result<(), HttpError> {
+pub fn add_matcher(config: &mut Config, matcher_config: MatcherConfig) -> Result<(), HttpError> {
super::ensure_unique(config, &matcher_config.name)?;
if let Some(targets) = matcher_config.target.as_deref() {
@@ -45,7 +45,7 @@ pub fn add_matcher(config: &mut Config, matcher_config: &MatcherConfig) -> Resul
config
.config
- .set_data(&matcher_config.name, MATCHER_TYPENAME, matcher_config)
+ .set_data(&matcher_config.name, MATCHER_TYPENAME, &matcher_config)
.map_err(|e| {
http_err!(
INTERNAL_SERVER_ERROR,
@@ -67,7 +67,7 @@ pub fn add_matcher(config: &mut Config, matcher_config: &MatcherConfig) -> Resul
pub fn update_matcher(
config: &mut Config,
name: &str,
- matcher_updater: &MatcherConfigUpdater,
+ matcher_updater: MatcherConfigUpdater,
delete: Option<&[DeleteableMatcherProperty]>,
digest: Option<&[u8]>,
) -> Result<(), HttpError> {
@@ -90,16 +90,16 @@ pub fn update_matcher(
}
}
- if let Some(match_severity) = &matcher_updater.match_severity {
- matcher.match_severity = Some(match_severity.clone());
+ if let Some(match_severity) = matcher_updater.match_severity {
+ matcher.match_severity = Some(match_severity);
}
- if let Some(match_field) = &matcher_updater.match_field {
- matcher.match_field = Some(match_field.clone());
+ if let Some(match_field) = matcher_updater.match_field {
+ matcher.match_field = Some(match_field);
}
- if let Some(match_calendar) = &matcher_updater.match_calendar {
- matcher.match_calendar = Some(match_calendar.clone());
+ if let Some(match_calendar) = matcher_updater.match_calendar {
+ matcher.match_calendar = Some(match_calendar);
}
if let Some(mode) = matcher_updater.mode {
@@ -110,17 +110,17 @@ pub fn update_matcher(
matcher.invert_match = Some(invert_match);
}
- if let Some(comment) = &matcher_updater.comment {
- matcher.comment = Some(comment.into());
+ if let Some(comment) = matcher_updater.comment {
+ matcher.comment = Some(comment);
}
- if let Some(disable) = &matcher_updater.disable {
- matcher.disable = Some(*disable);
+ if let Some(disable) = matcher_updater.disable {
+ matcher.disable = Some(disable);
}
- if let Some(target) = &matcher_updater.target {
+ if let Some(target) = matcher_updater.target {
super::ensure_endpoints_exist(config, target.as_slice())?;
- matcher.target = Some(target.clone());
+ matcher.target = Some(target);
}
config
@@ -178,7 +178,7 @@ matcher: matcher2
#[test]
fn test_update_not_existing_returns_error() -> Result<(), HttpError> {
let mut config = empty_config();
- assert!(update_matcher(&mut config, "test", &Default::default(), None, None).is_err());
+ assert!(update_matcher(&mut config, "test", Default::default(), None, None).is_err());
Ok(())
}
@@ -188,7 +188,7 @@ matcher: matcher2
assert!(update_matcher(
&mut config,
"matcher1",
- &Default::default(),
+ Default::default(),
None,
Some(&[0u8; 32])
)
@@ -206,7 +206,7 @@ matcher: matcher2
update_matcher(
&mut config,
"matcher1",
- &MatcherConfigUpdater {
+ MatcherConfigUpdater {
mode: Some(MatchModeOperator::Any),
match_field: None,
match_severity: None,
@@ -230,7 +230,7 @@ matcher: matcher2
update_matcher(
&mut config,
"matcher1",
- &Default::default(),
+ Default::default(),
Some(&[
DeleteableMatcherProperty::InvertMatch,
DeleteableMatcherProperty::Mode,
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index 7cc2593..bb0371d 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -173,13 +173,13 @@ fn get_referenced_entities(config: &Config, entity: &str) -> HashSet<String> {
#[allow(unused)]
fn set_private_config_entry<T: Serialize>(
config: &mut Config,
- private_config: &T,
+ private_config: T,
typename: &str,
name: &str,
) -> Result<(), HttpError> {
config
.private_config
- .set_data(name, typename, private_config)
+ .set_data(name, typename, &private_config)
.map_err(|e| {
http_err!(
INTERNAL_SERVER_ERROR,
@@ -217,7 +217,7 @@ mod tests {
sendmail::add_endpoint(
&mut config,
- &SendmailConfig {
+ SendmailConfig {
name: "sendmail".to_string(),
mailto: Some(vec!["foo@example.com".to_string()]),
..Default::default()
@@ -226,7 +226,7 @@ mod tests {
sendmail::add_endpoint(
&mut config,
- &SendmailConfig {
+ SendmailConfig {
name: "builtin".to_string(),
mailto: Some(vec!["foo@example.com".to_string()]),
origin: Some(Origin::Builtin),
@@ -236,12 +236,12 @@ mod tests {
gotify::add_endpoint(
&mut config,
- &GotifyConfig {
+ GotifyConfig {
name: "gotify".to_string(),
server: "localhost".to_string(),
..Default::default()
},
- &GotifyPrivateConfig {
+ GotifyPrivateConfig {
name: "gotify".to_string(),
token: "foo".to_string(),
},
@@ -249,7 +249,7 @@ mod tests {
matcher::add_matcher(
&mut config,
- &MatcherConfig {
+ MatcherConfig {
name: "matcher".to_string(),
target: Some(vec![
"sendmail".to_string(),
diff --git a/proxmox-notify/src/api/sendmail.rs b/proxmox-notify/src/api/sendmail.rs
index e911505..3285ac7 100644
--- a/proxmox-notify/src/api/sendmail.rs
+++ b/proxmox-notify/src/api/sendmail.rs
@@ -37,7 +37,7 @@ pub fn get_endpoint(config: &Config, name: &str) -> Result<SendmailConfig, HttpE
/// - an entity with the same name already exists (`400 Bad request`)
/// - the configuration could not be saved (`500 Internal server error`)
/// - mailto *and* mailto_user are both set to `None`
-pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<(), HttpError> {
+pub fn add_endpoint(config: &mut Config, endpoint: SendmailConfig) -> Result<(), HttpError> {
super::ensure_unique(config, &endpoint.name)?;
if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
@@ -49,7 +49,7 @@ pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<()
config
.config
- .set_data(&endpoint.name, SENDMAIL_TYPENAME, endpoint)
+ .set_data(&endpoint.name, SENDMAIL_TYPENAME, &endpoint)
.map_err(|e| {
http_err!(
INTERNAL_SERVER_ERROR,
@@ -69,7 +69,7 @@ pub fn add_endpoint(config: &mut Config, endpoint: &SendmailConfig) -> Result<()
pub fn update_endpoint(
config: &mut Config,
name: &str,
- updater: &SendmailConfigUpdater,
+ updater: SendmailConfigUpdater,
delete: Option<&[DeleteableSendmailProperty]>,
digest: Option<&[u8]>,
) -> Result<(), HttpError> {
@@ -90,28 +90,28 @@ pub fn update_endpoint(
}
}
- if let Some(mailto) = &updater.mailto {
- endpoint.mailto = Some(mailto.iter().map(String::from).collect());
+ if let Some(mailto) = updater.mailto {
+ endpoint.mailto = Some(mailto);
}
- if let Some(mailto_user) = &updater.mailto_user {
- endpoint.mailto_user = Some(mailto_user.iter().map(String::from).collect());
+ if let Some(mailto_user) = updater.mailto_user {
+ endpoint.mailto_user = Some(mailto_user);
}
- if let Some(from_address) = &updater.from_address {
- endpoint.from_address = Some(from_address.into());
+ if let Some(from_address) = updater.from_address {
+ endpoint.from_address = Some(from_address);
}
- if let Some(author) = &updater.author {
- endpoint.author = Some(author.into());
+ if let Some(author) = updater.author {
+ endpoint.author = Some(author);
}
- if let Some(comment) = &updater.comment {
- endpoint.comment = Some(comment.into());
+ if let Some(comment) = updater.comment {
+ endpoint.comment = Some(comment);
}
- if let Some(disable) = &updater.disable {
- endpoint.disable = Some(*disable);
+ if let Some(disable) = updater.disable {
+ endpoint.disable = Some(disable);
}
if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
@@ -162,7 +162,7 @@ pub mod tests {
) -> Result<(), HttpError> {
add_endpoint(
config,
- &SendmailConfig {
+ SendmailConfig {
name: name.into(),
mailto: Some(vec!["user1@example.com".into()]),
mailto_user: None,
@@ -193,7 +193,7 @@ pub mod tests {
fn test_update_not_existing_returns_error() -> Result<(), HttpError> {
let mut config = empty_config();
- assert!(update_endpoint(&mut config, "test", &Default::default(), None, None,).is_err());
+ assert!(update_endpoint(&mut config, "test", Default::default(), None, None,).is_err());
Ok(())
}
@@ -206,7 +206,7 @@ pub mod tests {
assert!(update_endpoint(
&mut config,
"sendmail-endpoint",
- &SendmailConfigUpdater {
+ SendmailConfigUpdater {
mailto: Some(vec!["user2@example.com".into(), "user3@example.com".into()]),
mailto_user: None,
from_address: Some("root@example.com".into()),
@@ -232,7 +232,7 @@ pub mod tests {
update_endpoint(
&mut config,
"sendmail-endpoint",
- &SendmailConfigUpdater {
+ SendmailConfigUpdater {
mailto: Some(vec!["user2@example.com".into(), "user3@example.com".into()]),
mailto_user: Some(vec!["root@pam".into()]),
from_address: Some("root@example.com".into()),
@@ -262,7 +262,7 @@ pub mod tests {
update_endpoint(
&mut config,
"sendmail-endpoint",
- &Default::default(),
+ Default::default(),
Some(&[
DeleteableSendmailProperty::FromAddress,
DeleteableSendmailProperty::Author,
diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp.rs
index 6bd0c4b..16d103c 100644
--- a/proxmox-notify/src/api/smtp.rs
+++ b/proxmox-notify/src/api/smtp.rs
@@ -40,8 +40,8 @@ pub fn get_endpoint(config: &Config, name: &str) -> Result<SmtpConfig, HttpError
/// - mailto *and* mailto_user are both set to `None`
pub fn add_endpoint(
config: &mut Config,
- endpoint_config: &SmtpConfig,
- private_endpoint_config: &SmtpPrivateConfig,
+ endpoint_config: SmtpConfig,
+ private_endpoint_config: SmtpPrivateConfig,
) -> Result<(), HttpError> {
if endpoint_config.name != private_endpoint_config.name {
// Programming error by the user of the crate, thus we panic
@@ -66,7 +66,7 @@ pub fn add_endpoint(
config
.config
- .set_data(&endpoint_config.name, SMTP_TYPENAME, endpoint_config)
+ .set_data(&endpoint_config.name, SMTP_TYPENAME, &endpoint_config)
.map_err(|e| {
http_err!(
INTERNAL_SERVER_ERROR,
@@ -86,8 +86,8 @@ pub fn add_endpoint(
pub fn update_endpoint(
config: &mut Config,
name: &str,
- updater: &SmtpConfigUpdater,
- private_endpoint_config_updater: &SmtpPrivateConfigUpdater,
+ updater: SmtpConfigUpdater,
+ private_endpoint_config_updater: SmtpPrivateConfigUpdater,
delete: Option<&[DeleteableSmtpProperty]>,
digest: Option<&[u8]>,
) -> Result<(), HttpError> {
@@ -105,7 +105,7 @@ pub fn update_endpoint(
DeleteableSmtpProperty::MailtoUser => endpoint.mailto_user = None,
DeleteableSmtpProperty::Password => super::set_private_config_entry(
config,
- &SmtpPrivateConfig {
+ SmtpPrivateConfig {
name: name.to_string(),
password: None,
},
@@ -118,49 +118,49 @@ pub fn update_endpoint(
}
}
- if let Some(mailto) = &updater.mailto {
- endpoint.mailto = Some(mailto.iter().map(String::from).collect());
+ if let Some(mailto) = updater.mailto {
+ endpoint.mailto = Some(mailto);
}
- if let Some(mailto_user) = &updater.mailto_user {
- endpoint.mailto_user = Some(mailto_user.iter().map(String::from).collect());
+ if let Some(mailto_user) = updater.mailto_user {
+ endpoint.mailto_user = Some(mailto_user);
}
- if let Some(from_address) = &updater.from_address {
- endpoint.from_address = from_address.into();
+ if let Some(from_address) = updater.from_address {
+ endpoint.from_address = from_address;
}
- if let Some(server) = &updater.server {
- endpoint.server = server.into();
+ if let Some(server) = updater.server {
+ endpoint.server = server;
}
- if let Some(port) = &updater.port {
- endpoint.port = Some(*port);
+ if let Some(port) = updater.port {
+ endpoint.port = Some(port);
}
- if let Some(username) = &updater.username {
- endpoint.username = Some(username.into());
+ if let Some(username) = updater.username {
+ endpoint.username = Some(username);
}
- if let Some(mode) = &updater.mode {
- endpoint.mode = Some(*mode);
+ if let Some(mode) = updater.mode {
+ endpoint.mode = Some(mode);
}
- if let Some(password) = &private_endpoint_config_updater.password {
+ if let Some(password) = private_endpoint_config_updater.password {
super::set_private_config_entry(
config,
- &SmtpPrivateConfig {
+ SmtpPrivateConfig {
name: name.into(),
- password: Some(password.into()),
+ password: Some(password),
},
SMTP_TYPENAME,
name,
)?;
}
- if let Some(author) = &updater.author {
- endpoint.author = Some(author.into());
+ if let Some(author) = updater.author {
+ endpoint.author = Some(author);
}
- if let Some(comment) = &updater.comment {
- endpoint.comment = Some(comment.into());
+ if let Some(comment) = updater.comment {
+ endpoint.comment = Some(comment);
}
- if let Some(disable) = &updater.disable {
- endpoint.disable = Some(*disable);
+ if let Some(disable) = updater.disable {
+ endpoint.disable = Some(disable);
}
if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
@@ -209,7 +209,7 @@ pub mod tests {
pub fn add_smtp_endpoint_for_test(config: &mut Config, name: &str) -> Result<(), HttpError> {
add_endpoint(
config,
- &SmtpConfig {
+ SmtpConfig {
name: name.into(),
mailto: Some(vec!["user1@example.com".into()]),
mailto_user: None,
@@ -222,7 +222,7 @@ pub mod tests {
username: Some("username".into()),
..Default::default()
},
- &SmtpPrivateConfig {
+ SmtpPrivateConfig {
name: name.into(),
password: Some("password".into()),
},
@@ -252,8 +252,8 @@ pub mod tests {
assert!(update_endpoint(
&mut config,
"test",
- &Default::default(),
- &Default::default(),
+ Default::default(),
+ Default::default(),
None,
None,
)
@@ -270,8 +270,8 @@ pub mod tests {
assert!(update_endpoint(
&mut config,
"sendmail-endpoint",
- &Default::default(),
- &Default::default(),
+ Default::default(),
+ Default::default(),
None,
Some(&[0; 32]),
)
@@ -290,7 +290,7 @@ pub mod tests {
update_endpoint(
&mut config,
"smtp-endpoint",
- &SmtpConfigUpdater {
+ SmtpConfigUpdater {
mailto: Some(vec!["user2@example.com".into(), "user3@example.com".into()]),
mailto_user: Some(vec!["root@pam".into()]),
from_address: Some("root@example.com".into()),
@@ -302,7 +302,7 @@ pub mod tests {
username: Some("newusername".into()),
..Default::default()
},
- &Default::default(),
+ Default::default(),
None,
Some(&digest),
)?;
@@ -325,8 +325,8 @@ pub mod tests {
update_endpoint(
&mut config,
"smtp-endpoint",
- &Default::default(),
- &Default::default(),
+ Default::default(),
+ Default::default(),
Some(&[
DeleteableSmtpProperty::Author,
DeleteableSmtpProperty::MailtoUser,
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox 03/19] notify: convert Option<Vec<T>> -> Vec<T> in config structs
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 01/19] notify: switch to file-based templating system Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 02/19] notify: make api methods take config struct ownership Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 04/19] notify: don't make tests require pve-context Lukas Wagner
` (16 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@proxmox.com>
---
proxmox-notify/src/api/matcher.rs | 27 +++++++--------
proxmox-notify/src/api/mod.rs | 22 +++++-------
proxmox-notify/src/api/sendmail.rs | 22 ++++++------
proxmox-notify/src/api/smtp.rs | 24 ++++++-------
proxmox-notify/src/endpoints/common/mail.rs | 20 ++++-------
proxmox-notify/src/endpoints/sendmail.rs | 14 ++++----
proxmox-notify/src/endpoints/smtp.rs | 18 +++++-----
proxmox-notify/src/lib.rs | 10 +++---
proxmox-notify/src/matcher.rs | 38 ++++++++++++---------
9 files changed, 96 insertions(+), 99 deletions(-)
diff --git a/proxmox-notify/src/api/matcher.rs b/proxmox-notify/src/api/matcher.rs
index f0eabd9..63ec73d 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -38,10 +38,7 @@ pub fn get_matcher(config: &Config, name: &str) -> Result<MatcherConfig, HttpErr
/// - the configuration could not be saved (`500 Internal server error`)
pub fn add_matcher(config: &mut Config, matcher_config: MatcherConfig) -> Result<(), HttpError> {
super::ensure_unique(config, &matcher_config.name)?;
-
- if let Some(targets) = matcher_config.target.as_deref() {
- super::ensure_endpoints_exist(config, targets)?;
- }
+ super::ensure_endpoints_exist(config, &matcher_config.target)?;
config
.config
@@ -78,10 +75,10 @@ pub fn update_matcher(
if let Some(delete) = delete {
for deleteable_property in delete {
match deleteable_property {
- DeleteableMatcherProperty::MatchSeverity => matcher.match_severity = None,
- DeleteableMatcherProperty::MatchField => matcher.match_field = None,
- DeleteableMatcherProperty::MatchCalendar => matcher.match_calendar = None,
- DeleteableMatcherProperty::Target => matcher.target = None,
+ DeleteableMatcherProperty::MatchSeverity => matcher.match_severity.clear(),
+ DeleteableMatcherProperty::MatchField => matcher.match_field.clear(),
+ DeleteableMatcherProperty::MatchCalendar => matcher.match_calendar.clear(),
+ DeleteableMatcherProperty::Target => matcher.target.clear(),
DeleteableMatcherProperty::Mode => matcher.mode = None,
DeleteableMatcherProperty::InvertMatch => matcher.invert_match = None,
DeleteableMatcherProperty::Comment => matcher.comment = None,
@@ -91,15 +88,15 @@ pub fn update_matcher(
}
if let Some(match_severity) = matcher_updater.match_severity {
- matcher.match_severity = Some(match_severity);
+ matcher.match_severity = match_severity;
}
if let Some(match_field) = matcher_updater.match_field {
- matcher.match_field = Some(match_field);
+ matcher.match_field = match_field;
}
if let Some(match_calendar) = matcher_updater.match_calendar {
- matcher.match_calendar = Some(match_calendar);
+ matcher.match_calendar = match_calendar;
}
if let Some(mode) = matcher_updater.mode {
@@ -120,7 +117,7 @@ pub fn update_matcher(
if let Some(target) = matcher_updater.target {
super::ensure_endpoints_exist(config, target.as_slice())?;
- matcher.target = Some(target);
+ matcher.target = target;
}
config
@@ -244,9 +241,9 @@ matcher: matcher2
let matcher = get_matcher(&config, "matcher1")?;
assert_eq!(matcher.invert_match, None);
- assert!(matcher.match_severity.is_none());
- assert!(matches!(matcher.match_field, None));
- assert_eq!(matcher.target, None);
+ assert!(matcher.match_severity.is_empty());
+ assert!(matcher.match_field.is_empty());
+ assert!(matcher.target.is_empty());
assert!(matcher.mode.is_none());
assert_eq!(matcher.comment, None);
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index bb0371d..a2cf29e 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -102,10 +102,8 @@ fn get_referrers(config: &Config, entity: &str) -> Result<HashSet<String>, HttpE
let mut referrers = HashSet::new();
for matcher in matcher::get_matchers(config)? {
- if let Some(targets) = matcher.target {
- if targets.iter().any(|target| target == entity) {
- referrers.insert(matcher.name.clone());
- }
+ if matcher.target.iter().any(|target| target == entity) {
+ referrers.insert(matcher.name.clone());
}
}
@@ -149,11 +147,9 @@ fn get_referenced_entities(config: &Config, entity: &str) -> HashSet<String> {
let mut new = HashSet::new();
for entity in entities {
- if let Ok(group) = matcher::get_matcher(config, entity) {
- if let Some(targets) = group.target {
- for target in targets {
- new.insert(target.clone());
- }
+ if let Ok(matcher) = matcher::get_matcher(config, entity) {
+ for target in matcher.target {
+ new.insert(target.clone());
}
}
}
@@ -219,7 +215,7 @@ mod tests {
&mut config,
SendmailConfig {
name: "sendmail".to_string(),
- mailto: Some(vec!["foo@example.com".to_string()]),
+ mailto: vec!["foo@example.com".to_string()],
..Default::default()
},
)?;
@@ -228,7 +224,7 @@ mod tests {
&mut config,
SendmailConfig {
name: "builtin".to_string(),
- mailto: Some(vec!["foo@example.com".to_string()]),
+ mailto: vec!["foo@example.com".to_string()],
origin: Some(Origin::Builtin),
..Default::default()
},
@@ -251,11 +247,11 @@ mod tests {
&mut config,
MatcherConfig {
name: "matcher".to_string(),
- target: Some(vec![
+ target: vec![
"sendmail".to_string(),
"gotify".to_string(),
"builtin".to_string(),
- ]),
+ ],
..Default::default()
},
)?;
diff --git a/proxmox-notify/src/api/sendmail.rs b/proxmox-notify/src/api/sendmail.rs
index 3285ac7..c20a3e5 100644
--- a/proxmox-notify/src/api/sendmail.rs
+++ b/proxmox-notify/src/api/sendmail.rs
@@ -40,7 +40,7 @@ pub fn get_endpoint(config: &Config, name: &str) -> Result<SendmailConfig, HttpE
pub fn add_endpoint(config: &mut Config, endpoint: SendmailConfig) -> Result<(), HttpError> {
super::ensure_unique(config, &endpoint.name)?;
- if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
+ if endpoint.mailto.is_empty() && endpoint.mailto_user.is_empty() {
http_bail!(
BAD_REQUEST,
"must at least provide one recipient, either in mailto or in mailto-user"
@@ -83,19 +83,19 @@ pub fn update_endpoint(
DeleteableSendmailProperty::FromAddress => endpoint.from_address = None,
DeleteableSendmailProperty::Author => endpoint.author = None,
DeleteableSendmailProperty::Comment => endpoint.comment = None,
- DeleteableSendmailProperty::Mailto => endpoint.mailto = None,
- DeleteableSendmailProperty::MailtoUser => endpoint.mailto_user = None,
+ DeleteableSendmailProperty::Mailto => endpoint.mailto.clear(),
+ DeleteableSendmailProperty::MailtoUser => endpoint.mailto_user.clear(),
DeleteableSendmailProperty::Disable => endpoint.disable = None,
}
}
}
if let Some(mailto) = updater.mailto {
- endpoint.mailto = Some(mailto);
+ endpoint.mailto = mailto;
}
if let Some(mailto_user) = updater.mailto_user {
- endpoint.mailto_user = Some(mailto_user);
+ endpoint.mailto_user = mailto_user;
}
if let Some(from_address) = updater.from_address {
@@ -114,7 +114,7 @@ pub fn update_endpoint(
endpoint.disable = Some(disable);
}
- if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
+ if endpoint.mailto.is_empty() && endpoint.mailto_user.is_empty() {
http_bail!(
BAD_REQUEST,
"must at least provide one recipient, either in mailto or in mailto-user"
@@ -164,8 +164,8 @@ pub mod tests {
config,
SendmailConfig {
name: name.into(),
- mailto: Some(vec!["user1@example.com".into()]),
- mailto_user: None,
+ mailto: vec!["user1@example.com".into()],
+ mailto_user: vec![],
from_address: Some("from@example.com".into()),
author: Some("root".into()),
comment: Some("Comment".into()),
@@ -248,12 +248,12 @@ pub mod tests {
assert_eq!(
endpoint.mailto,
- Some(vec![
+ vec![
"user2@example.com".to_string(),
"user3@example.com".to_string()
- ])
+ ]
);
- assert_eq!(endpoint.mailto_user, Some(vec!["root@pam".to_string(),]));
+ assert_eq!(endpoint.mailto_user, vec!["root@pam".to_string(),]);
assert_eq!(endpoint.from_address, Some("root@example.com".to_string()));
assert_eq!(endpoint.author, Some("newauthor".to_string()));
assert_eq!(endpoint.comment, Some("new comment".to_string()));
diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp.rs
index 16d103c..7a58677 100644
--- a/proxmox-notify/src/api/smtp.rs
+++ b/proxmox-notify/src/api/smtp.rs
@@ -50,7 +50,7 @@ pub fn add_endpoint(
super::ensure_unique(config, &endpoint_config.name)?;
- if endpoint_config.mailto.is_none() && endpoint_config.mailto_user.is_none() {
+ if endpoint_config.mailto.is_empty() && endpoint_config.mailto_user.is_empty() {
http_bail!(
BAD_REQUEST,
"must at least provide one recipient, either in mailto or in mailto-user"
@@ -101,8 +101,8 @@ pub fn update_endpoint(
DeleteableSmtpProperty::Author => endpoint.author = None,
DeleteableSmtpProperty::Comment => endpoint.comment = None,
DeleteableSmtpProperty::Disable => endpoint.disable = None,
- DeleteableSmtpProperty::Mailto => endpoint.mailto = None,
- DeleteableSmtpProperty::MailtoUser => endpoint.mailto_user = None,
+ DeleteableSmtpProperty::Mailto => endpoint.mailto.clear(),
+ DeleteableSmtpProperty::MailtoUser => endpoint.mailto_user.clear(),
DeleteableSmtpProperty::Password => super::set_private_config_entry(
config,
SmtpPrivateConfig {
@@ -119,10 +119,10 @@ pub fn update_endpoint(
}
if let Some(mailto) = updater.mailto {
- endpoint.mailto = Some(mailto);
+ endpoint.mailto = mailto;
}
if let Some(mailto_user) = updater.mailto_user {
- endpoint.mailto_user = Some(mailto_user);
+ endpoint.mailto_user = mailto_user;
}
if let Some(from_address) = updater.from_address {
endpoint.from_address = from_address;
@@ -163,7 +163,7 @@ pub fn update_endpoint(
endpoint.disable = Some(disable);
}
- if endpoint.mailto.is_none() && endpoint.mailto_user.is_none() {
+ if endpoint.mailto.is_empty() && endpoint.mailto_user.is_empty() {
http_bail!(
BAD_REQUEST,
"must at least provide one recipient, either in mailto or in mailto-user"
@@ -211,8 +211,8 @@ pub mod tests {
config,
SmtpConfig {
name: name.into(),
- mailto: Some(vec!["user1@example.com".into()]),
- mailto_user: None,
+ mailto: vec!["user1@example.com".into()],
+ mailto_user: vec![],
from_address: "from@example.com".into(),
author: Some("root".into()),
comment: Some("Comment".into()),
@@ -311,12 +311,12 @@ pub mod tests {
assert_eq!(
endpoint.mailto,
- Some(vec![
+ vec![
"user2@example.com".to_string(),
"user3@example.com".to_string()
- ])
+ ]
);
- assert_eq!(endpoint.mailto_user, Some(vec!["root@pam".to_string(),]));
+ assert_eq!(endpoint.mailto_user, vec!["root@pam".to_string(),]);
assert_eq!(endpoint.from_address, "root@example.com".to_string());
assert_eq!(endpoint.author, Some("newauthor".to_string()));
assert_eq!(endpoint.comment, Some("new comment".to_string()));
@@ -343,7 +343,7 @@ pub mod tests {
assert_eq!(endpoint.comment, None);
assert_eq!(endpoint.port, None);
assert_eq!(endpoint.username, None);
- assert_eq!(endpoint.mailto_user, None);
+ assert!(endpoint.mailto_user.is_empty());
Ok(())
}
diff --git a/proxmox-notify/src/endpoints/common/mail.rs b/proxmox-notify/src/endpoints/common/mail.rs
index 0929d7c..b1c4823 100644
--- a/proxmox-notify/src/endpoints/common/mail.rs
+++ b/proxmox-notify/src/endpoints/common/mail.rs
@@ -2,22 +2,16 @@ use std::collections::HashSet;
use crate::context;
-pub(crate) fn get_recipients(
- email_addrs: Option<&[String]>,
- users: Option<&[String]>,
-) -> HashSet<String> {
+pub(crate) fn get_recipients(email_addrs: &[String], users: &[String]) -> HashSet<String> {
let mut recipients = HashSet::new();
- if let Some(mailto_addrs) = email_addrs {
- for addr in mailto_addrs {
- recipients.insert(addr.clone());
- }
+ for addr in email_addrs {
+ recipients.insert(addr.clone());
}
- if let Some(users) = users {
- for user in users {
- if let Some(addr) = context::context().lookup_email_for_user(user) {
- recipients.insert(addr);
- }
+
+ for user in users {
+ if let Some(addr) = context::context().lookup_email_for_user(user) {
+ recipients.insert(addr);
}
}
recipients
diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index 6178b5e..fa04002 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -44,11 +44,13 @@ pub struct SendmailConfig {
#[updater(skip)]
pub name: String,
/// Mail recipients
- #[serde(skip_serializing_if = "Option::is_none")]
- pub mailto: Option<Vec<String>>,
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub mailto: Vec<String>,
/// Mail recipients
- #[serde(skip_serializing_if = "Option::is_none")]
- pub mailto_user: Option<Vec<String>>,
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub mailto_user: Vec<String>,
/// `From` address for the mail
#[serde(skip_serializing_if = "Option::is_none")]
pub from_address: Option<String>,
@@ -90,8 +92,8 @@ pub struct SendmailEndpoint {
impl Endpoint for SendmailEndpoint {
fn send(&self, notification: &Notification) -> Result<(), Error> {
let recipients = mail::get_recipients(
- self.config.mailto.as_deref(),
- self.config.mailto_user.as_deref(),
+ self.config.mailto.as_slice(),
+ self.config.mailto_user.as_slice(),
);
let recipients_str: Vec<&str> = recipients.iter().map(String::as_str).collect();
diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
index f0c836a..ded5baf 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -44,8 +44,8 @@ pub enum SmtpMode {
},
mailto: {
type: Array,
- items: {
- schema: EMAIL_SCHEMA,
+ items: {
+ schema: EMAIL_SCHEMA,
},
optional: true,
},
@@ -80,11 +80,13 @@ pub struct SmtpConfig {
#[serde(skip_serializing_if = "Option::is_none")]
pub username: Option<String>,
/// Mail recipients
- #[serde(skip_serializing_if = "Option::is_none")]
- pub mailto: Option<Vec<String>>,
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub mailto: Vec<String>,
/// Mail recipients
- #[serde(skip_serializing_if = "Option::is_none")]
- pub mailto_user: Option<Vec<String>>,
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub mailto_user: Vec<String>,
/// `From` address for the mail
pub from_address: String,
/// Author of the mail
@@ -177,8 +179,8 @@ impl Endpoint for SmtpEndpoint {
let transport = transport_builder.build();
let recipients = mail::get_recipients(
- self.config.mailto.as_deref(),
- self.config.mailto_user.as_deref(),
+ self.config.mailto.as_slice(),
+ self.config.mailto_user.as_slice(),
);
let mail_from = self.config.from_address.clone();
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 9f8683f..35dcb17 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -611,7 +611,7 @@ mod tests {
bus.add_endpoint(Box::new(mock.clone()));
let matcher = MatcherConfig {
- target: Some(vec!["endpoint".into()]),
+ target: vec!["endpoint".into()],
..Default::default()
};
@@ -642,15 +642,15 @@ mod tests {
bus.add_matcher(MatcherConfig {
name: "matcher1".into(),
- match_severity: Some(vec!["warning,error".parse()?]),
- target: Some(vec!["mock1".into()]),
+ match_severity: vec!["warning,error".parse()?],
+ target: vec!["mock1".into()],
..Default::default()
});
bus.add_matcher(MatcherConfig {
name: "matcher2".into(),
- match_severity: Some(vec!["error".parse()?]),
- target: Some(vec!["mock2".into()]),
+ match_severity: vec!["error".parse()?],
+ target: vec!["mock2".into()],
..Default::default()
});
diff --git a/proxmox-notify/src/matcher.rs b/proxmox-notify/src/matcher.rs
index d6051ac..b387fef 100644
--- a/proxmox-notify/src/matcher.rs
+++ b/proxmox-notify/src/matcher.rs
@@ -113,17 +113,19 @@ pub struct MatcherConfig {
pub name: String,
/// List of matched metadata fields
- #[serde(skip_serializing_if = "Option::is_none")]
- pub match_field: Option<Vec<FieldMatcher>>,
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub match_field: Vec<FieldMatcher>,
/// List of matched severity levels
- #[serde(skip_serializing_if = "Option::is_none")]
- pub match_severity: Option<Vec<SeverityMatcher>>,
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub match_severity: Vec<SeverityMatcher>,
/// List of matched severity levels
- #[serde(skip_serializing_if = "Option::is_none")]
- pub match_calendar: Option<Vec<CalendarMatcher>>,
-
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub match_calendar: Vec<CalendarMatcher>,
/// Decide if 'all' or 'any' match statements must match
#[serde(skip_serializing_if = "Option::is_none")]
pub mode: Option<MatchModeOperator>,
@@ -133,8 +135,9 @@ pub struct MatcherConfig {
pub invert_match: Option<bool>,
/// Targets to notify
- #[serde(skip_serializing_if = "Option::is_none")]
- pub target: Option<Vec<String>>,
+ #[serde(default, skip_serializing_if = "Vec::is_empty")]
+ #[updater(serde(skip_serializing_if = "Option::is_none"))]
+ pub target: Vec<String>,
/// Comment
#[serde(skip_serializing_if = "Option::is_none")]
@@ -284,29 +287,32 @@ impl MatcherConfig {
// If there are no matching directives, the matcher will always match
let mut no_matchers = true;
- if let Some(severity_matchers) = self.match_severity.as_deref() {
+ if !self.match_severity.is_empty() {
no_matchers = false;
is_match = mode.apply(
is_match,
- self.check_matches(notification, severity_matchers)?,
+ self.check_matches(notification, &self.match_severity)?,
);
}
- if let Some(field_matchers) = self.match_field.as_deref() {
+ if !self.match_field.is_empty() {
no_matchers = false;
- is_match = mode.apply(is_match, self.check_matches(notification, field_matchers)?);
+ is_match = mode.apply(
+ is_match,
+ self.check_matches(notification, &self.match_field)?,
+ );
}
- if let Some(calendar_matchers) = self.match_calendar.as_deref() {
+ if !self.match_calendar.is_empty() {
no_matchers = false;
is_match = mode.apply(
is_match,
- self.check_matches(notification, calendar_matchers)?,
+ self.check_matches(notification, &self.match_calendar)?,
);
}
let invert_match = self.invert_match.unwrap_or_default();
Ok(if is_match != invert_match || no_matchers {
- Some(self.target.as_deref().unwrap_or_default())
+ Some(&self.target)
} else {
None
})
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox 04/19] notify: don't make tests require pve-context
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (2 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 03/19] notify: convert Option<Vec<T>> -> Vec<T> in config structs Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 05/19] notify: make the `mail-forwarder` feature depend on proxmox-sys Lukas Wagner
` (15 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
Tests now have their own context, so requiring pve-context is not
necessary any more.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@proxmox.com>
---
proxmox-notify/src/api/gotify.rs | 2 +-
proxmox-notify/src/api/matcher.rs | 2 +-
proxmox-notify/src/api/sendmail.rs | 2 +-
proxmox-notify/src/api/smtp.rs | 24 ++++++++++++------------
4 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/proxmox-notify/src/api/gotify.rs b/proxmox-notify/src/api/gotify.rs
index 15d94cb..92151f5 100644
--- a/proxmox-notify/src/api/gotify.rs
+++ b/proxmox-notify/src/api/gotify.rs
@@ -165,7 +165,7 @@ fn remove_private_config_entry(config: &mut Config, name: &str) -> Result<(), Ht
Ok(())
}
-#[cfg(all(feature = "pve-context", test))]
+#[cfg(test)]
mod tests {
use super::*;
use crate::api::test_helpers::empty_config;
diff --git a/proxmox-notify/src/api/matcher.rs b/proxmox-notify/src/api/matcher.rs
index 63ec73d..fa11633 100644
--- a/proxmox-notify/src/api/matcher.rs
+++ b/proxmox-notify/src/api/matcher.rs
@@ -148,7 +148,7 @@ pub fn delete_matcher(config: &mut Config, name: &str) -> Result<(), HttpError>
Ok(())
}
-#[cfg(all(test, feature = "sendmail", feature = "pve-context"))]
+#[cfg(all(test, feature = "sendmail"))]
mod tests {
use super::*;
use crate::matcher::MatchModeOperator;
diff --git a/proxmox-notify/src/api/sendmail.rs b/proxmox-notify/src/api/sendmail.rs
index c20a3e5..47588af 100644
--- a/proxmox-notify/src/api/sendmail.rs
+++ b/proxmox-notify/src/api/sendmail.rs
@@ -151,7 +151,7 @@ pub fn delete_endpoint(config: &mut Config, name: &str) -> Result<(), HttpError>
Ok(())
}
-#[cfg(all(feature = "pve-context", test))]
+#[cfg(test)]
pub mod tests {
use super::*;
use crate::api::test_helpers::*;
diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp.rs
index 7a58677..1b4700e 100644
--- a/proxmox-notify/src/api/smtp.rs
+++ b/proxmox-notify/src/api/smtp.rs
@@ -200,7 +200,7 @@ pub fn delete_endpoint(config: &mut Config, name: &str) -> Result<(), HttpError>
Ok(())
}
-#[cfg(all(feature = "pve-context", test))]
+#[cfg(test)]
pub mod tests {
use super::*;
use crate::api::test_helpers::*;
@@ -348,15 +348,15 @@ pub mod tests {
Ok(())
}
- // #[test]
- // fn test_delete() -> Result<(), HttpError> {
- // let mut config = empty_config();
- // add_smtp_endpoint_for_test(&mut config, "smtp-endpoint")?;
- //
- // delete_endpoint(&mut config, "smtp-endpoint")?;
- // assert!(delete_endpoint(&mut config, "smtp-endpoint").is_err());
- // assert_eq!(get_endpoints(&config)?.len(), 0);
- //
- // Ok(())
- // }
+ #[test]
+ fn test_delete() -> Result<(), HttpError> {
+ let mut config = empty_config();
+ add_smtp_endpoint_for_test(&mut config, "smtp-endpoint")?;
+
+ delete_endpoint(&mut config, "smtp-endpoint")?;
+ assert!(delete_endpoint(&mut config, "smtp-endpoint").is_err());
+ assert_eq!(get_endpoints(&config)?.len(), 0);
+
+ Ok(())
+ }
}
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox 05/19] notify: make the `mail-forwarder` feature depend on proxmox-sys
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (3 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 04/19] notify: don't make tests require pve-context Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-19 8:19 ` Fiona Ebner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 06/19] notify: give each notification a unique ID Lukas Wagner
` (14 subsequent siblings)
19 siblings, 1 reply; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
It uses proxmox_sys::nodename - the dep is needed, otherwise the code
does not compile in some feature flag permutations.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@proxmox.com>
---
proxmox-notify/Cargo.toml | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index 52a466e..185b50a 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -17,13 +17,13 @@ log.workspace = true
mail-parser = { workspace = true, optional = true }
openssl.workspace = true
regex.workspace = true
-serde = { workspace = true, features = ["derive"]}
+serde = { workspace = true, features = ["derive"] }
serde_json.workspace = true
proxmox-http = { workspace = true, features = ["client-sync"], optional = true }
proxmox-http-error.workspace = true
proxmox-human-byte.workspace = true
-proxmox-schema = { workspace = true, features = ["api-macro", "api-types"]}
+proxmox-schema = { workspace = true, features = ["api-macro", "api-types"] }
proxmox-section-config = { workspace = true }
proxmox-serde.workspace = true
proxmox-sys = { workspace = true, optional = true }
@@ -31,7 +31,7 @@ proxmox-time.workspace = true
[features]
default = ["sendmail", "gotify", "smtp"]
-mail-forwarder = ["dep:mail-parser"]
+mail-forwarder = ["dep:mail-parser", "dep:proxmox-sys"]
sendmail = ["dep:proxmox-sys"]
gotify = ["dep:proxmox-http"]
pve-context = ["dep:proxmox-sys"]
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 05/19] notify: make the `mail-forwarder` feature depend on proxmox-sys
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 05/19] notify: make the `mail-forwarder` feature depend on proxmox-sys Lukas Wagner
@ 2024-04-19 8:19 ` Fiona Ebner
0 siblings, 0 replies; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 8:19 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> It uses proxmox_sys::nodename - the dep is needed, otherwise the code
> does not compile in some feature flag permutations.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> Tested-by: Folke Gleumes <f.gleumes@proxmox.com>
> ---
> proxmox-notify/Cargo.toml | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> index 52a466e..185b50a 100644
> --- a/proxmox-notify/Cargo.toml
> +++ b/proxmox-notify/Cargo.toml
> @@ -17,13 +17,13 @@ log.workspace = true
> mail-parser = { workspace = true, optional = true }
> openssl.workspace = true
> regex.workspace = true
> -serde = { workspace = true, features = ["derive"]}
> +serde = { workspace = true, features = ["derive"] }
Nit: unrelated whitespace changes should be their own patch
> serde_json.workspace = true
>
> proxmox-http = { workspace = true, features = ["client-sync"], optional = true }
> proxmox-http-error.workspace = true
> proxmox-human-byte.workspace = true
> -proxmox-schema = { workspace = true, features = ["api-macro", "api-types"]}
> +proxmox-schema = { workspace = true, features = ["api-macro", "api-types"] }
> proxmox-section-config = { workspace = true }
> proxmox-serde.workspace = true
> proxmox-sys = { workspace = true, optional = true }
> @@ -31,7 +31,7 @@ proxmox-time.workspace = true
>
> [features]
> default = ["sendmail", "gotify", "smtp"]
> -mail-forwarder = ["dep:mail-parser"]
> +mail-forwarder = ["dep:mail-parser", "dep:proxmox-sys"]
> sendmail = ["dep:proxmox-sys"]
> gotify = ["dep:proxmox-http"]
> pve-context = ["dep:proxmox-sys"]
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [pve-devel] [PATCH proxmox 06/19] notify: give each notification a unique ID
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (4 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 05/19] notify: make the `mail-forwarder` feature depend on proxmox-sys Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets Lukas Wagner
` (13 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
We need this for queuing notifications on PBS from the unprivileged
proxy process.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notify/Cargo.toml | 1 +
proxmox-notify/src/lib.rs | 11 +++++++++++
2 files changed, 12 insertions(+)
diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
index 185b50a..797b1ac 100644
--- a/proxmox-notify/Cargo.toml
+++ b/proxmox-notify/Cargo.toml
@@ -28,6 +28,7 @@ proxmox-section-config = { workspace = true }
proxmox-serde.workspace = true
proxmox-sys = { workspace = true, optional = true }
proxmox-time.workspace = true
+proxmox-uuid = { workspace = true, features = ["serde"] }
[features]
default = ["sendmail", "gotify", "smtp"]
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 35dcb17..91c0b61 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -10,6 +10,7 @@ use serde_json::Value;
use proxmox_schema::api;
use proxmox_section_config::SectionConfigData;
+use proxmox_uuid::Uuid;
pub mod matcher;
use crate::config::CONFIG;
@@ -198,6 +199,8 @@ pub struct Notification {
content: Content,
/// Metadata
metadata: Metadata,
+ /// Unique ID
+ id: Uuid,
}
impl Notification {
@@ -217,6 +220,7 @@ impl Notification {
template_name: template_name.as_ref().to_string(),
data: template_data,
},
+ id: Uuid::generate(),
}
}
#[cfg(feature = "mail-forwarder")]
@@ -246,8 +250,14 @@ impl Notification {
additional_fields,
timestamp: proxmox_time::epoch_i64(),
},
+ id: Uuid::generate(),
})
}
+
+ /// Return the unique ID of this notification.
+ pub fn id(&self) -> &Uuid {
+ &self.id
+ }
}
/// Notification configuration
@@ -548,6 +558,7 @@ impl Bus {
template_name: "test".to_string(),
data: json!({ "target": target }),
},
+ id: Uuid::generate(),
};
if let Some(endpoint) = self.endpoints.get(target) {
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (5 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 06/19] notify: give each notification a unique ID Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-19 8:34 ` Fiona Ebner
2024-04-19 8:37 ` Fiona Ebner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 08/19] notify: derive `api` for Deleteable*Property Lukas Wagner
` (12 subsequent siblings)
19 siblings, 2 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
This method allows us to get a list of all notification targets.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notify/src/api/mod.rs | 75 +++++++++++++++++++++++++++++++++++
1 file changed, 75 insertions(+)
diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs
index a2cf29e..9a4ce8b 100644
--- a/proxmox-notify/src/api/mod.rs
+++ b/proxmox-notify/src/api/mod.rs
@@ -3,6 +3,7 @@ use std::collections::HashSet;
use serde::{Deserialize, Serialize};
use proxmox_http_error::HttpError;
+use proxmox_schema::api;
use crate::{Config, Origin};
@@ -39,6 +40,80 @@ macro_rules! http_bail {
pub use http_bail;
pub use http_err;
+#[api]
+#[derive(Clone, Debug, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd)]
+#[serde(rename_all = "kebab-case")]
+/// Type of the endpoint.
+pub enum EndpointType {
+ /// Sendmail endpoint
+ #[cfg(feature = "sendmail")]
+ Sendmail,
+ /// SMTP endpoint
+ #[cfg(feature = "smtp")]
+ Smtp,
+ /// Gotify endpoint
+ #[cfg(feature = "gotify")]
+ Gotify,
+}
+
+#[api]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd)]
+#[serde(rename_all = "kebab-case")]
+/// Target information
+pub struct Target {
+ /// Name of the endpoint
+ name: String,
+ /// Origin of the endpoint
+ origin: Origin,
+ /// Type of the endpoint
+ #[serde(rename = "type")]
+ endpoint_type: EndpointType,
+ /// Target is disabled
+ disable: Option<bool>,
+ /// Comment
+ comment: Option<String>,
+}
+
+/// Get a list of all notification targets.
+pub fn get_targets(config: &Config) -> Result<Vec<Target>, HttpError> {
+ let mut targets = Vec::new();
+
+ #[cfg(feature = "gotify")]
+ for endpoint in gotify::get_endpoints(config)? {
+ targets.push(Target {
+ name: endpoint.name,
+ origin: endpoint.origin.unwrap_or(Origin::UserCreated),
+ endpoint_type: EndpointType::Gotify,
+ disable: endpoint.disable,
+ comment: endpoint.comment,
+ })
+ }
+
+ #[cfg(feature = "sendmail")]
+ for endpoint in sendmail::get_endpoints(config)? {
+ targets.push(Target {
+ name: endpoint.name,
+ origin: endpoint.origin.unwrap_or(Origin::UserCreated),
+ endpoint_type: EndpointType::Sendmail,
+ disable: endpoint.disable,
+ comment: endpoint.comment,
+ })
+ }
+
+ #[cfg(feature = "smtp")]
+ for endpoint in smtp::get_endpoints(config)? {
+ targets.push(Target {
+ name: endpoint.name,
+ origin: endpoint.origin.unwrap_or(Origin::UserCreated),
+ endpoint_type: EndpointType::Smtp,
+ disable: endpoint.disable,
+ comment: endpoint.comment,
+ })
+ }
+
+ Ok(targets)
+}
+
fn verify_digest(config: &Config, digest: Option<&[u8]>) -> Result<(), HttpError> {
if let Some(digest) = digest {
if config.digest != *digest {
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets Lukas Wagner
@ 2024-04-19 8:34 ` Fiona Ebner
2024-04-19 12:54 ` Lukas Wagner
2024-04-19 8:37 ` Fiona Ebner
1 sibling, 1 reply; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 8:34 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> +/// Get a list of all notification targets.
> +pub fn get_targets(config: &Config) -> Result<Vec<Target>, HttpError> {
> + let mut targets = Vec::new();
> +
> + #[cfg(feature = "gotify")]
> + for endpoint in gotify::get_endpoints(config)? {
> + targets.push(Target {
> + name: endpoint.name,
> + origin: endpoint.origin.unwrap_or(Origin::UserCreated),
> + endpoint_type: EndpointType::Gotify,
> + disable: endpoint.disable,
> + comment: endpoint.comment,
> + })
Would it make sense to have into() functions for
{Gotify,Sendmail,Smtp}Config -> Target ?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets
2024-04-19 8:34 ` Fiona Ebner
@ 2024-04-19 12:54 ` Lukas Wagner
0 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-19 12:54 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2024-04-19 10:34, Fiona Ebner wrote:
> Am 09.04.24 um 15:25 schrieb Lukas Wagner:
>> +/// Get a list of all notification targets.
>> +pub fn get_targets(config: &Config) -> Result<Vec<Target>, HttpError> {
>> + let mut targets = Vec::new();
>> +
>> + #[cfg(feature = "gotify")]
>> + for endpoint in gotify::get_endpoints(config)? {
>> + targets.push(Target {
>> + name: endpoint.name,
>> + origin: endpoint.origin.unwrap_or(Origin::UserCreated),
>> + endpoint_type: EndpointType::Gotify,
>> + disable: endpoint.disable,
>> + comment: endpoint.comment,
>> + })
>
> Would it make sense to have into() functions for
> {Gotify,Sendmail,Smtp}Config -> Target ?
That would indeed be a bit nicer - but I'll do that in a follow-up, since this is
completely internal to proxmox-notify and is more 'style' than an actual issue :)
Thanks!
--
- Lukas
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets Lukas Wagner
2024-04-19 8:34 ` Fiona Ebner
@ 2024-04-19 8:37 ` Fiona Ebner
1 sibling, 0 replies; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 8:37 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> +#[api]
> +#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd)]
> +#[serde(rename_all = "kebab-case")]
> +/// Target information
> +pub struct Target {
> + /// Name of the endpoint
> + name: String,
> + /// Origin of the endpoint
> + origin: Origin,
> + /// Type of the endpoint
> + #[serde(rename = "type")]
> + endpoint_type: EndpointType,
> + /// Target is disabled
Oh, and don't we want:
#[serde(skip_serializing_if = "Option::is_none")]
here?
> + disable: Option<bool>,
> + /// Comment
> + comment: Option<String>,
> +}
> +
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [pve-devel] [PATCH proxmox 08/19] notify: derive `api` for Deleteable*Property
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (6 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 07/19] notify: api: add get_targets Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 09/19] notify: derive Deserialize/Serialize for Notification struct Lukas Wagner
` (11 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
The API endpoints in Proxmox Backup Server require ApiType to be
implemented for any deserialized parameter.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notify/src/endpoints/gotify.rs | 3 +++
proxmox-notify/src/endpoints/sendmail.rs | 7 +++++++
proxmox-notify/src/endpoints/smtp.rs | 9 +++++++++
proxmox-notify/src/matcher.rs | 9 +++++++++
4 files changed, 28 insertions(+)
diff --git a/proxmox-notify/src/endpoints/gotify.rs b/proxmox-notify/src/endpoints/gotify.rs
index 4c1f9e0..70675c8 100644
--- a/proxmox-notify/src/endpoints/gotify.rs
+++ b/proxmox-notify/src/endpoints/gotify.rs
@@ -81,10 +81,13 @@ pub struct GotifyEndpoint {
pub private_config: GotifyPrivateConfig,
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableGotifyProperty {
+ /// Delete `comment`
Comment,
+ /// Delete `disable`
Disable,
}
diff --git a/proxmox-notify/src/endpoints/sendmail.rs b/proxmox-notify/src/endpoints/sendmail.rs
index fa04002..47901ef 100644
--- a/proxmox-notify/src/endpoints/sendmail.rs
+++ b/proxmox-notify/src/endpoints/sendmail.rs
@@ -73,14 +73,21 @@ pub struct SendmailConfig {
pub origin: Option<Origin>,
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableSendmailProperty {
+ /// Delete `author`
Author,
+ /// Delete `comment`
Comment,
+ /// Delete `disable`
Disable,
+ /// Delete `from-address`
FromAddress,
+ /// Delete `mailto`
Mailto,
+ /// Delete `mailto-user`
MailtoUser,
}
diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
index ded5baf..f04583a 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -104,16 +104,25 @@ pub struct SmtpConfig {
pub origin: Option<Origin>,
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableSmtpProperty {
+ /// Delete `author`
Author,
+ /// Delete `comment`
Comment,
+ /// Delete `disable`
Disable,
+ /// Delete `mailto`
Mailto,
+ /// Delete `mailto-user`
MailtoUser,
+ /// Delete `password`
Password,
+ /// Delete `port`
Port,
+ /// Delete `username`
Username,
}
diff --git a/proxmox-notify/src/matcher.rs b/proxmox-notify/src/matcher.rs
index b387fef..2d30378 100644
--- a/proxmox-notify/src/matcher.rs
+++ b/proxmox-notify/src/matcher.rs
@@ -412,16 +412,25 @@ impl FromStr for CalendarMatcher {
}
}
+#[api]
#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
pub enum DeleteableMatcherProperty {
+ /// Delete `comment`
Comment,
+ /// Delete `disable`
Disable,
+ /// Delete `invert-match`
InvertMatch,
+ /// Delete `match-calendar`
MatchCalendar,
+ /// Delete `match-field`
MatchField,
+ /// Delete `match-severity`
MatchSeverity,
+ /// Delete `mode`
Mode,
+ /// Delete `target`
Target,
}
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox 09/19] notify: derive Deserialize/Serialize for Notification struct
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (7 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 08/19] notify: derive `api` for Deleteable*Property Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-19 8:45 ` Fiona Ebner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 10/19] notify: pbs context: include nodename in default sendmail author Lukas Wagner
` (10 subsequent siblings)
19 siblings, 1 reply; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notify/src/lib.rs | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
index 91c0b61..8d4dc63 100644
--- a/proxmox-notify/src/lib.rs
+++ b/proxmox-notify/src/lib.rs
@@ -159,11 +159,13 @@ pub trait Endpoint {
fn disabled(&self) -> bool;
}
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
pub enum Content {
/// Title and body will be rendered as a template
Template {
/// Name of the used template
+ #[serde(rename = "template-name")]
template_name: String,
/// Data that can be used for template rendering.
data: Value,
@@ -182,7 +184,8 @@ pub enum Content {
},
}
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
pub struct Metadata {
/// Notification severity
severity: Severity,
@@ -192,7 +195,8 @@ pub struct Metadata {
additional_fields: HashMap<String, String>,
}
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case")]
/// Notification which can be sent
pub struct Notification {
/// Notification content
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 09/19] notify: derive Deserialize/Serialize for Notification struct
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 09/19] notify: derive Deserialize/Serialize for Notification struct Lukas Wagner
@ 2024-04-19 8:45 ` Fiona Ebner
2024-04-19 12:46 ` Lukas Wagner
0 siblings, 1 reply; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 8:45 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Nit: I always like a quick sentence for who needs it for such changes.
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> proxmox-notify/src/lib.rs | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 91c0b61..8d4dc63 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -159,11 +159,13 @@ pub trait Endpoint {
> fn disabled(&self) -> bool;
> }
>
> -#[derive(Debug, Clone)]
> +#[derive(Debug, Clone, Serialize, Deserialize)]
> +#[serde(rename_all = "kebab-case")]
> pub enum Content {
> /// Title and body will be rendered as a template
> Template {
> /// Name of the used template
> + #[serde(rename = "template-name")]
So I guess this is here, because the rename_all above is not recursive?
Should we put rename_all on top of Template and ForwardedMail (if that
even works), so we are sure not to forget it for potential future fields?
> template_name: String,
> /// Data that can be used for template rendering.
> data: Value,
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH proxmox 09/19] notify: derive Deserialize/Serialize for Notification struct
2024-04-19 8:45 ` Fiona Ebner
@ 2024-04-19 12:46 ` Lukas Wagner
0 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-19 12:46 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2024-04-19 10:45, Fiona Ebner wrote:
> Nit: I always like a quick sentence for who needs it for such changes.
>
> Am 09.04.24 um 15:25 schrieb Lukas Wagner:
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>> proxmox-notify/src/lib.rs | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
>> index 91c0b61..8d4dc63 100644
>> --- a/proxmox-notify/src/lib.rs
>> +++ b/proxmox-notify/src/lib.rs
>> @@ -159,11 +159,13 @@ pub trait Endpoint {
>> fn disabled(&self) -> bool;
>> }
>>
>> -#[derive(Debug, Clone)]
>> +#[derive(Debug, Clone, Serialize, Deserialize)]
>> +#[serde(rename_all = "kebab-case")]
>> pub enum Content {
>> /// Title and body will be rendered as a template
>> Template {
>> /// Name of the used template
>> + #[serde(rename = "template-name")]
>
> So I guess this is here, because the rename_all above is not recursive?
> Should we put rename_all on top of Template and ForwardedMail (if that
> even works), so we are sure not to forget it for potential future fields?
>
Yup, rename_all is not recursive. Added a rename_all for Template and ForwardedMail,
this makes more sense.
Thanks!
--
- Lukas
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [pve-devel] [PATCH proxmox 10/19] notify: pbs context: include nodename in default sendmail author
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (8 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 09/19] notify: derive Deserialize/Serialize for Notification struct Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 11/19] notify: renderer: add relative-percentage helper from PBS Lukas Wagner
` (9 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
The old notification stack in proxmox-backup includes the nodename, so
we include it here as well.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notify/src/context/pbs.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
index 70e993f..299f685 100644
--- a/proxmox-notify/src/context/pbs.rs
+++ b/proxmox-notify/src/context/pbs.rs
@@ -82,7 +82,7 @@ impl Context for PBSContext {
}
fn default_sendmail_author(&self) -> String {
- "Proxmox Backup Server".into()
+ format!("Proxmox Backup Server - {}", proxmox_sys::nodename())
}
fn default_sendmail_from(&self) -> String {
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox 11/19] notify: renderer: add relative-percentage helper from PBS
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (9 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 10/19] notify: pbs context: include nodename in default sendmail author Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox-perl-rs 12/19] notify: use file based notification templates Lukas Wagner
` (8 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
proxmox-notify/src/renderer/mod.rs | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/proxmox-notify/src/renderer/mod.rs b/proxmox-notify/src/renderer/mod.rs
index a51ece6..ddb241d 100644
--- a/proxmox-notify/src/renderer/mod.rs
+++ b/proxmox-notify/src/renderer/mod.rs
@@ -73,6 +73,30 @@ fn value_to_timestamp(val: &Value) -> Option<String> {
proxmox_time::strftime_local("%F %H:%M:%S", timestamp).ok()
}
+fn handlebars_relative_percentage_helper(
+ h: &Helper,
+ _: &Handlebars,
+ _: &Context,
+ _rc: &mut RenderContext,
+ out: &mut dyn Output,
+) -> HelperResult {
+ let param0 = h
+ .param(0)
+ .and_then(|v| v.value().as_f64())
+ .ok_or_else(|| HandlebarsRenderError::new("relative-percentage: param0 not found"))?;
+ let param1 = h
+ .param(1)
+ .and_then(|v| v.value().as_f64())
+ .ok_or_else(|| HandlebarsRenderError::new("relative-percentage: param1 not found"))?;
+
+ if param1 == 0.0 {
+ out.write("-")?;
+ } else {
+ out.write(&format!("{:.2}%", (param0 * 100.0) / param1))?;
+ }
+ Ok(())
+}
+
/// Available render functions for `serde_json::Values``
///
/// May be used as a handlebars helper, e.g.
@@ -237,6 +261,11 @@ fn render_template_impl(
ValueRenderFunction::register_helpers(&mut handlebars);
+ handlebars.register_helper(
+ "relative-percentage",
+ Box::new(handlebars_relative_percentage_helper),
+ );
+
let rendered_template = handlebars
.render_template(template, data)
.map_err(|err| Error::RenderError(err.into()))?;
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox-perl-rs 12/19] notify: use file based notification templates
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (10 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox 11/19] notify: renderer: add relative-percentage helper from PBS Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox-perl-rs 13/19] notify: don't pass config structs by reference Lukas Wagner
` (7 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 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>
Tested-by: Folke Gleumes <f.gleumes@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
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox-perl-rs 13/19] notify: don't pass config structs by reference
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (11 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox-perl-rs 12/19] notify: use file based notification templates Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox-perl-rs 14/19] notify: adapt to Option<Vec<T>> to Vec<T> changes in proxmox_notify Lukas Wagner
` (6 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 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>
Tested-by: Folke Gleumes <f.gleumes@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
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH proxmox-perl-rs 14/19] notify: adapt to Option<Vec<T>> to Vec<T> changes in proxmox_notify
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (12 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox-perl-rs 13/19] notify: don't pass config structs by reference Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH cluster 15/19] notify: use named template instead of passing template strings Lukas Wagner
` (5 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@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
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH cluster 15/19] notify: use named template instead of passing template strings
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (13 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH proxmox-perl-rs 14/19] notify: adapt to Option<Vec<T>> to Vec<T> changes in proxmox_notify Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-19 9:29 ` Fiona Ebner
2024-04-09 13:25 ` [pve-devel] [PATCH pve-ha-manager 16/19] env: notify: use named templates " Lukas Wagner
` (4 subsequent siblings)
19 siblings, 1 reply; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 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>
Tested-by: Folke Gleumes <f.gleumes@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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH cluster 15/19] notify: use named template instead of passing template strings
2024-04-09 13:25 ` [pve-devel] [PATCH cluster 15/19] notify: use named template instead of passing template strings Lukas Wagner
@ 2024-04-19 9:29 ` Fiona Ebner
0 siblings, 0 replies; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 9:29 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> 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>
> Tested-by: Folke Gleumes <f.gleumes@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) = @_;
And there's another versioned breaks required for packages with the
callers of all these functions here. Old version of package with caller
will be broken with new pve-cluster.
> $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
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [pve-devel] [PATCH pve-ha-manager 16/19] env: notify: use named templates instead of passing template strings
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (14 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH cluster 15/19] notify: use named template instead of passing template strings Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH manager 17/19] gitignore: ignore any test artifacts Lukas Wagner
` (3 subsequent siblings)
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@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..9784d84 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/proxmox-ve/templates/default/fencing-body.html.hbs
+/usr/share/proxmox-ve/templates/default/fencing-body.txt.hbs
+/usr/share/proxmox-ve/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..145adbc
--- /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/proxmox-ve/templates/default
+ $(foreach i,$(NOTIFICATION_TEMPLATES), \
+ install -m644 $(i) $(DESTDIR)/usr/share/proxmox-ve/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
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH manager 17/19] gitignore: ignore any test artifacts
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (15 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH pve-ha-manager 16/19] env: notify: use named templates " Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-19 9:46 ` Fiona Ebner
2024-04-09 13:25 ` [pve-devel] [PATCH manager 18/19] tests: remove vzdump_notification test Lukas Wagner
` (2 subsequent siblings)
19 siblings, 1 reply; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 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..48975d55 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/*.log.tmp
--
2.39.2
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH manager 18/19] tests: remove vzdump_notification test
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (16 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH manager 17/19] gitignore: ignore any test artifacts Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-09 13:25 ` [pve-devel] [PATCH manager 19/19] notifications: use named templates instead of in-code templates Lukas Wagner
2024-04-19 10:09 ` [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Fiona Ebner
19 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 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
^ permalink raw reply [flat|nested] 38+ messages in thread* [pve-devel] [PATCH manager 19/19] notifications: use named templates instead of in-code templates
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (17 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH manager 18/19] tests: remove vzdump_notification test Lukas Wagner
@ 2024-04-09 13:25 ` Lukas Wagner
2024-04-19 9:59 ` Fiona Ebner
2024-04-19 10:09 ` [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Fiona Ebner
19 siblings, 1 reply; 38+ messages in thread
From: Lukas Wagner @ 2024-04-09 13:25 UTC (permalink / raw)
To: pve-devel
This commit adapts notification sending for
- package update
- replication
- backups
to use named templates (installed in /usr/share/proxmox-ve/templates)
instead of passing template strings defined in code to the
notification stack.
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
Tested-by: Folke Gleumes <f.gleumes@proxmox.com>
---
Makefile | 2 +-
PVE/API2/APT.pm | 9 +------
PVE/API2/Replication.pm | 20 +---------------
PVE/VZDump.pm | 6 ++---
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(+), 32 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 54121ec2..b4a24c3e 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 152eb3e5..2ea626f0 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -544,8 +544,7 @@ sub send_notification {
PVE::Notify::notify(
$severity,
- $subject_template,
- $body_template,
+ "vzdump",
$notification_props,
$fields,
$notification_config
@@ -556,8 +555,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..b0add81e
--- /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/proxmox-ve/templates/default
+ $(foreach i,$(NOTIFICATION_TEMPLATES), \
+ install -m644 $(i) $(DESTDIR)/usr/share/proxmox-ve/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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH manager 19/19] notifications: use named templates instead of in-code templates
2024-04-09 13:25 ` [pve-devel] [PATCH manager 19/19] notifications: use named templates instead of in-code templates Lukas Wagner
@ 2024-04-19 9:59 ` Fiona Ebner
2024-04-19 10:42 ` Lukas Wagner
0 siblings, 1 reply; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 9:59 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index 152eb3e5..2ea626f0 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
The existing $subject_template and $body_template could be removed now
like for the others
> diff --git a/templates/Makefile b/templates/Makefile
> new file mode 100644
> index 00000000..b0add81e
> --- /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 \
> +
Nit: backslashes are not aligned
> 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}}
Nit: is there a special reason for not indenting this?
> + </pre>
> + </body>
> +</html>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH manager 19/19] notifications: use named templates instead of in-code templates
2024-04-19 9:59 ` Fiona Ebner
@ 2024-04-19 10:42 ` Lukas Wagner
0 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-19 10:42 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2024-04-19 11:59, Fiona Ebner wrote:
> Am 09.04.24 um 15:25 schrieb Lukas Wagner:
>> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
>> index 152eb3e5..2ea626f0 100644
>> --- a/PVE/VZDump.pm
>> +++ b/PVE/VZDump.pm
>
> The existing $subject_template and $body_template could be removed now
> like for the others
>
>> diff --git a/templates/Makefile b/templates/Makefile
>> new file mode 100644
>> index 00000000..b0add81e
>> --- /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 \
>> +
>
> Nit: backslashes are not aligned
>
>> 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}}
>
> Nit: is there a special reason for not indenting this?
Yes, since it's in a <pre> block, the first line of the logs would be displayed indented otherwise.
--
- Lukas
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations
2024-04-09 13:25 [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Lukas Wagner
` (18 preceding siblings ...)
2024-04-09 13:25 ` [pve-devel] [PATCH manager 19/19] notifications: use named templates instead of in-code templates Lukas Wagner
@ 2024-04-19 10:09 ` Fiona Ebner
2024-04-19 11:22 ` Fabian Grünbichler
2024-04-19 14:08 ` Fiona Ebner
19 siblings, 2 replies; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 10:09 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner; +Cc: Thomas Lamprecht
Am 09.04.24 um 15:25 schrieb Lukas Wagner:
> 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/proxmox-ve/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.
>
> The patches for `proxmox` also include some preparatory patches for
> the upcoming integration of the notification system into PBS. They
> are not needed for PVE, but I included them here since Folke and I
> tested the PVE changes with them applied. IMO they should just be
> applied with the same version bump.
> The list of optional, preparatory patches is:
> notify: give each notification a unique ID
> notify: api: add get_targets
> notify: derive `api` for Deleteable*Property
> notify: derive Deserialize/Serialize for Notification struct
> notify: pbs context: include nodename in default sendmail author
> notify: renderer: add relative-percentage helper from PBS
>
> Folke kindly did some off-list testing before this was posted, hence
> his T-bs were included.
>
> Bumps/dependencies:
> - proxmox_notify
> - libproxmox-rs-perl/libpve-rs-perl (needs bumped proxmox_notify)
> - libpve-notify-perl (needs bumped libproxmox-rs-perl/libpve-rs-perl)
> - pve-ha-manager (needs bumped libpve-notify-perl)
> - pve-manager (needs bumped libpve-notify-perl)
> - proxmox-mail-forward (optional. should not be affected by any changes, but I think
> it should be also be bumped for any larger proxmox_notify changes to avoid any
> weird hidden regressions due to mismatches of proxmox_notify
>
Versioned breaks required:
- new pve-cluster will break old pve-manager and old pve-ha-manager
- new libproxmox-rs-perl/libpve-rs-perl will break old pve-cluster
Still not too sure about putting the templates in
/usr/share/proxmox-ve/, but maybe @Thomas or @Fabian can chime in on that.
While I'm rather unfamiliar with the code and not much into Rust,
everything looked good to me (just a few nits). So, with a grain of salt:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations
2024-04-19 10:09 ` [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Fiona Ebner
@ 2024-04-19 11:22 ` Fabian Grünbichler
2024-04-19 11:29 ` Lukas Wagner
2024-04-19 14:08 ` Fiona Ebner
1 sibling, 1 reply; 38+ messages in thread
From: Fabian Grünbichler @ 2024-04-19 11:22 UTC (permalink / raw)
To: Fiona Ebner, Lukas Wagner, Proxmox VE development discussion
Cc: Thomas Lamprecht
On April 19, 2024 12:09 pm, Fiona Ebner wrote:
> Am 09.04.24 um 15:25 schrieb Lukas Wagner:
>> Bumps/dependencies:
>> - proxmox_notify
>> - libproxmox-rs-perl/libpve-rs-perl (needs bumped proxmox_notify)
>> - libpve-notify-perl (needs bumped libproxmox-rs-perl/libpve-rs-perl)
>> - pve-ha-manager (needs bumped libpve-notify-perl)
>> - pve-manager (needs bumped libpve-notify-perl)
>> - proxmox-mail-forward (optional. should not be affected by any changes, but I think
>> it should be also be bumped for any larger proxmox_notify changes to avoid any
>> weird hidden regressions due to mismatches of proxmox_notify
>>
>
> Versioned breaks required:
> - new pve-cluster will break old pve-manager and old pve-ha-manager
> - new libproxmox-rs-perl/libpve-rs-perl will break old pve-cluster
>
> Still not too sure about putting the templates in
> /usr/share/proxmox-ve/, but maybe @Thomas or @Fabian can chime in on that.
without in-depth look at all the changes in this series, I think it
would make most sense for the package shipping (most?) templates to
"own" the dir where they are shipped. that seems to be pve-manager, so
maybe /usr/share/pve-manager would be an okay fit?
or, if we want to avoid having "pve-manager" there, we could of course
also create /usr/share/pve or /usr/lib/pve or one of those with
"proxmox" instead of "pve", and have that owned by pve-manager..
I dislike proxmox-ve
- it might not be the case that it is always installed, which means
potential pitfalls for such non-standard systems or if we ever make it
optional like we did for PMG/PBS (granted, not very likely, but
still..)
- normally the /usr/share/$package dir should only be used by $package
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations
2024-04-19 11:22 ` Fabian Grünbichler
@ 2024-04-19 11:29 ` Lukas Wagner
0 siblings, 0 replies; 38+ messages in thread
From: Lukas Wagner @ 2024-04-19 11:29 UTC (permalink / raw)
To: Fabian Grünbichler, Fiona Ebner, Proxmox VE development discussion
Cc: Thomas Lamprecht
On 2024-04-19 13:22, Fabian Grünbichler wrote:
> On April 19, 2024 12:09 pm, Fiona Ebner wrote:
>> Am 09.04.24 um 15:25 schrieb Lukas Wagner:
>>> Bumps/dependencies:
>>> - proxmox_notify
>>> - libproxmox-rs-perl/libpve-rs-perl (needs bumped proxmox_notify)
>>> - libpve-notify-perl (needs bumped libproxmox-rs-perl/libpve-rs-perl)
>>> - pve-ha-manager (needs bumped libpve-notify-perl)
>>> - pve-manager (needs bumped libpve-notify-perl)
>>> - proxmox-mail-forward (optional. should not be affected by any changes, but I think
>>> it should be also be bumped for any larger proxmox_notify changes to avoid any
>>> weird hidden regressions due to mismatches of proxmox_notify
>>>
>>
>> Versioned breaks required:
>> - new pve-cluster will break old pve-manager and old pve-ha-manager
>> - new libproxmox-rs-perl/libpve-rs-perl will break old pve-cluster
>>
>> Still not too sure about putting the templates in
>> /usr/share/proxmox-ve/, but maybe @Thomas or @Fabian can chime in on that.
>
> without in-depth look at all the changes in this series, I think it
> would make most sense for the package shipping (most?) templates to
> "own" the dir where they are shipped. that seems to be pve-manager, so
> maybe /usr/share/pve-manager would be an okay fit?
>
Okay, I think I'll just use pve-manager then.
Thanks!
--
- Lukas
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations
2024-04-19 10:09 ` [pve-devel] [PATCH many 00/19] notifications: move template strings to template files; PBS preparations Fiona Ebner
2024-04-19 11:22 ` Fabian Grünbichler
@ 2024-04-19 14:08 ` Fiona Ebner
1 sibling, 0 replies; 38+ messages in thread
From: Fiona Ebner @ 2024-04-19 14:08 UTC (permalink / raw)
To: Proxmox VE development discussion, Lukas Wagner; +Cc: Thomas Lamprecht
Am 19.04.24 um 12:09 schrieb Fiona Ebner:
>>
>
> Versioned breaks required:
> - new pve-cluster will break old pve-manager and old pve-ha-manager
> - new libproxmox-rs-perl/libpve-rs-perl will break old pve-cluster
>
Since I got myself confused for a moment when re-installing and it never
hurts to avoid confusion: the relevant package is libpve-notify-perl
(which lives in the pve-cluster repo)
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 38+ messages in thread