Re: [pbs-devel] [PATCH proxmox-backup 02/10] notifications: add type for GC notification template data

[Lukas Wagner <l.wagner@proxmox.com>]

On  2025-03-21 14:28, Wolfgang Bumiller wrote:
> On Fri, Mar 21, 2025 at 01:25:13PM +0100, Lukas Wagner wrote:
>> This commit adds a separate type for the data passed to this type of
>> notification template. Also we make sure that we do not expose any
>> non-primitive types to the template renderer, any data
>> needed in the template is mapped into the new dedicated
>> template data type.
>> This ensures that any changes in types defined in other places
>> do not leak into the template rendering process by accident.
>>
>> This commit also tries to unify the style and naming of template
>> variables.
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>  src/server/notifications/mod.rs           |  50 ++++----
>>  src/server/notifications/template_data.rs | 132 ++++++++++++++++++++++
>>  templates/default/gc-err-body.txt.hbs     |   2 +-
>>  templates/default/gc-err-subject.txt.hbs  |   2 +-
>>  templates/default/gc-ok-body.txt.hbs      |  22 ++--
>>  templates/default/gc-ok-subject.txt.hbs   |   2 +-
>>  6 files changed, 170 insertions(+), 40 deletions(-)
>>  create mode 100644 src/server/notifications/template_data.rs
>>
>> diff --git a/src/server/notifications/mod.rs b/src/server/notifications/mod.rs
>> index eea55202..182af213 100644
>> --- a/src/server/notifications/mod.rs
>> +++ b/src/server/notifications/mod.rs
>> @@ -21,6 +21,10 @@ use proxmox_notify::{Endpoint, Notification, Severity};
>>  
>>  const SPOOL_DIR: &str = concatcp!(pbs_buildcfg::PROXMOX_BACKUP_STATE_DIR, "/notifications");
>>  
>> +mod template_data;
>> +
>> +use template_data::{GcErrTemplateData, GcOkTemplateData};
>> +
>>  /// Initialize the notification system by setting context in proxmox_notify
>>  pub fn init() -> Result<(), Error> {
>>      proxmox_notify::context::set_context(&PBS_CONTEXT);
>> @@ -146,38 +150,32 @@ pub fn send_gc_status(
>>      status: &GarbageCollectionStatus,
>>      result: &Result<(), Error>,
>>  ) -> Result<(), Error> {
>> -    let (fqdn, port) = get_server_url();
>> -    let mut data = json!({
>> -        "datastore": datastore,
>> -        "fqdn": fqdn,
>> -        "port": port,
>> -    });
>> -
>> -    let (severity, template) = match result {
>> -        Ok(()) => {
>> -            let deduplication_factor = if status.disk_bytes > 0 {
>> -                (status.index_data_bytes as f64) / (status.disk_bytes as f64)
>> -            } else {
>> -                1.0
>> -            };
>> -
>> -            data["status"] = json!(status);
>> -            data["deduplication-factor"] = format!("{:.2}", deduplication_factor).into();
>> -
>> -            (Severity::Info, "gc-ok")
>> -        }
>> -        Err(err) => {
>> -            data["error"] = err.to_string().into();
>> -            (Severity::Error, "gc-err")
>> -        }
>> -    };
>>      let metadata = HashMap::from([
>>          ("datastore".into(), datastore.into()),
>>          ("hostname".into(), proxmox_sys::nodename().into()),
>>          ("type".into(), "gc".into()),
>>      ]);
>>  
>> -    let notification = Notification::from_template(severity, template, data, metadata);
>> +    let notification = match result {
>> +        Ok(()) => {
>> +            let template_data = GcOkTemplateData::new(datastore.to_string(), status);
>> +            Notification::from_template(
>> +                Severity::Info,
>> +                "gc-ok",
>> +                serde_json::to_value(template_data)?,
>> +                metadata,
>> +            )
>> +        }
>> +        Err(err) => {
>> +            let template_data = GcErrTemplateData::new(datastore.to_string(), err.to_string());
> 
> ^ While at it, we could consider switching this error to use
> `format!{"{err:#}")` which will include the context as a short form.
> This goes for all the templates/commits - but can just be a follow up.
> Noticing this since Chris got me looking at anyhow's display
> representations[1]
> 
> [1] https://docs.rs/anyhow/latest/anyhow/struct.Error.html#display-representations
> 

Good point, I'll check it out. Will do that in a followup if nothing else comes up.

Thanks!

-- 
- Lukas



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