From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id DC1021FF0E4 for ; Tue, 28 Jul 2026 15:09:36 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id AA8862142F; Tue, 28 Jul 2026 15:09:36 +0200 (CEST) Date: Tue, 28 Jul 2026 15:09:30 +0200 From: Wolfgang Bumiller To: Lukas Wagner Subject: Re: [PATCH proxmox-perl-rs 23/29] notify: matcher: pass matcher config / updater directly Message-ID: References: <20260709115716.299836-1-l.wagner@proxmox.com> <20260709115716.299836-24-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260709115716.299836-24-l.wagner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785244133764 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.204 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: GTVMJDJ7PP23QSBV5EAIDF7B3YCDHP7W X-Message-ID-Hash: GTVMJDJ7PP23QSBV5EAIDF7B3YCDHP7W X-MailFrom: w.bumiller@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com, pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Noticed another thing worth mentioning for future perl-rs changes: On Thu, Jul 09, 2026 at 01:57:10PM +0200, Lukas Wagner wrote: > Instead of enumerating all config properties individually as paramters, > we now pass the config struct or updater as a parameter. This allows us > to simplify the callers and also we don't have to change the binding > code if new parameters are added -- a simple rebuild against > proxmox-notify suffices. > > Signed-off-by: Lukas Wagner > --- > common/src/bindings/notify.rs | 200 +++++----------------------------- > 1 file changed, 26 insertions(+), 174 deletions(-) > > diff --git a/common/src/bindings/notify.rs b/common/src/bindings/notify.rs > index 409270a..80ccf39 100644 > --- a/common/src/bindings/notify.rs > +++ b/common/src/bindings/notify.rs > @@ -12,7 +12,7 @@ pub mod proxmox_rs_notify { > use std::collections::HashMap; > use std::sync::Mutex; > > - use anyhow::{Error, bail}; > + use anyhow::{bail, Error}; > use serde_json::Value as JSONValue; > > use perlmod::Value; > @@ -26,17 +26,14 @@ pub mod proxmox_rs_notify { > DeleteableSendmailProperty, SendmailConfig, SendmailConfigUpdater, > }; > use proxmox_notify::endpoints::smtp::{ > - DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpMode, SmtpPrivateConfig, > + DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPrivateConfig, > SmtpPrivateConfigUpdater, > }; > use proxmox_notify::endpoints::webhook::{ > DeleteableWebhookProperty, WebhookConfig, WebhookConfigUpdater, > }; > - use proxmox_notify::matcher::{ > - CalendarMatcher, DeleteableMatcherProperty, FieldMatcher, MatchModeOperator, MatcherConfig, > - MatcherConfigUpdater, SeverityMatcher, > - }; > - use proxmox_notify::{Config, Notification, Severity, api}; > + use proxmox_notify::matcher::{DeleteableMatcherProperty, MatcherConfig, MatcherConfigUpdater}; > + use proxmox_notify::{api, Config, Notification, Severity}; > > /// A notification catalog instance. > /// > @@ -189,49 +186,23 @@ pub mod proxmox_rs_notify { > /// > /// See [`api::sendmail::add_endpoint`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn add_sendmail_endpoint( > #[try_from_ref] this: &NotificationConfig, > - name: String, > - mailto: Option>, > - mailto_user: Option>, > - from_address: Option, > - author: Option, > - comment: Option, > - disable: Option, > + sendmail_config: SendmailConfig, As you mentioned in the cover letter, dependency version bumps are needed. Since recently, perlmod supports `@` parameters via `#[list]`. It is technically possible to support both ways of calling this function (temporarily), bu making its signature: fn( #[try_from_ref] this: &NotificationConfig, #[list] params: Vec, ) -> … And then something like the following: let config = match params.len() { 1 => perlmod::de::from_ref_value(¶ms[0]), // new API 7 => SendmailConfig { name: perlmod::de::from_ref_value(¶ms[0]), mailto: perlmod::de::from_ref_value(¶ms[1]), … } n => bail!("unexpected parameter count"), }; I'm mostly mentioning this as it'll allow splitting up series a bit more easily this way, especially in code in the `common/` dir as for that we'll need to remember to also bump the PMG side, although this particular code IIRC is not yet used by PMG anyway. > ) -> Result<(), HttpError> { > let mut config = this.config.lock().unwrap(); > > - api::sendmail::add_endpoint( > - &mut config, > - SendmailConfig { > - name, > - mailto: mailto.unwrap_or_default(), > - mailto_user: mailto_user.unwrap_or_default(), > - from_address, > - author, > - comment, > - disable, > - filter: None, > - origin: None, > - }, > - ) > + api::sendmail::add_endpoint(&mut config, sendmail_config) > } > > /// Method: Update a sendmail endpoint. > /// > /// See [`api::sendmail::update_endpoint`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn update_sendmail_endpoint( > #[try_from_ref] this: &NotificationConfig, > name: &str, > - mailto: Option>, > - mailto_user: Option>, > - from_address: Option, > - author: Option, > - comment: Option, > - disable: Option, > + updater: SendmailConfigUpdater, > delete: Option>, > digest: Option<&str>, > ) -> Result<(), HttpError> { > @@ -241,14 +212,7 @@ pub mod proxmox_rs_notify { > api::sendmail::update_endpoint( > &mut config, > name, > - SendmailConfigUpdater { > - mailto, > - mailto_user, > - from_address, > - author, > - comment, > - disable, > - }, > + updater, > delete.as_deref(), > digest.as_deref(), > ) > @@ -295,39 +259,22 @@ pub mod proxmox_rs_notify { > #[export(serialize_error)] > pub fn add_gotify_endpoint( > #[try_from_ref] this: &NotificationConfig, > - name: String, > - server: String, > - token: String, > - comment: Option, > - disable: Option, > + gotify_config: GotifyConfig, > + gotify_private_config: GotifyPrivateConfig, > ) -> Result<(), HttpError> { > let mut config = this.config.lock().unwrap(); > - api::gotify::add_endpoint( > - &mut config, > - GotifyConfig { > - name: name.clone(), > - server, > - comment, > - disable, > - filter: None, > - origin: None, > - }, > - GotifyPrivateConfig { name, token }, > - ) > + api::gotify::add_endpoint(&mut config, gotify_config, gotify_private_config) > } > > /// Method: Update a 'gotify' endpoint. > /// > /// See [`api::gotify::update_endpoint`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn update_gotify_endpoint( > #[try_from_ref] this: &NotificationConfig, > name: &str, > - server: Option, > - token: Option, > - comment: Option, > - disable: Option, > + updater: GotifyConfigUpdater, > + private_updater: GotifyPrivateConfigUpdater, > delete: Option>, > digest: Option<&str>, > ) -> Result<(), HttpError> { > @@ -337,12 +284,8 @@ pub mod proxmox_rs_notify { > api::gotify::update_endpoint( > &mut config, > name, > - GotifyConfigUpdater { > - server, > - comment, > - disable, > - }, > - GotifyPrivateConfigUpdater { token }, > + updater, > + private_updater, > delete.as_deref(), > digest.as_deref(), > ) > @@ -387,62 +330,24 @@ pub mod proxmox_rs_notify { > /// > /// See [`api::smtp::add_endpoint`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn add_smtp_endpoint( > #[try_from_ref] this: &NotificationConfig, > - name: String, > - server: String, > - port: Option, > - mode: Option, > - username: Option, > - password: Option, > - mailto: Option>, > - mailto_user: Option>, > - from_address: String, > - author: Option, > - comment: Option, > - disable: Option, > + smtp_config: SmtpConfig, > + smtp_private_config: SmtpPrivateConfig, > ) -> Result<(), HttpError> { > let mut config = this.config.lock().unwrap(); > - api::smtp::add_endpoint( > - &mut config, > - SmtpConfig { > - name: name.clone(), > - server, > - port, > - mode, > - username, > - mailto: mailto.unwrap_or_default(), > - mailto_user: mailto_user.unwrap_or_default(), > - from_address, > - author, > - comment, > - disable, > - origin: None, > - }, > - SmtpPrivateConfig { name, password }, > - ) > + api::smtp::add_endpoint(&mut config, smtp_config, smtp_private_config) > } > > /// Method: Update an SMTP endpoint. > /// > /// See [`api::smtp::update_endpoint`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn update_smtp_endpoint( > #[try_from_ref] this: &NotificationConfig, > name: &str, > - server: Option, > - port: Option, > - mode: Option, > - username: Option, > - password: Option, > - mailto: Option>, > - mailto_user: Option>, > - from_address: Option, > - author: Option, > - comment: Option, > - disable: Option, > + updater: SmtpConfigUpdater, > + private_updater: SmtpPrivateConfigUpdater, > delete: Option>, > digest: Option<&str>, > ) -> Result<(), HttpError> { > @@ -452,19 +357,8 @@ pub mod proxmox_rs_notify { > api::smtp::update_endpoint( > &mut config, > name, > - SmtpConfigUpdater { > - server, > - port, > - mode, > - username, > - mailto, > - mailto_user, > - from_address, > - author, > - comment, > - disable, > - }, > - SmtpPrivateConfigUpdater { password }, > + updater, > + private_updater, > delete.as_deref(), > digest.as_deref(), > ) > @@ -509,7 +403,6 @@ pub mod proxmox_rs_notify { > /// > /// See [`api::webhook::add_endpoint`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn add_webhook_endpoint( > #[try_from_ref] this: &NotificationConfig, > endpoint_config: WebhookConfig, > @@ -522,7 +415,6 @@ pub mod proxmox_rs_notify { > /// > /// See [`api::webhook::update_endpoint`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn update_webhook_endpoint( > #[try_from_ref] this: &NotificationConfig, > name: &str, > @@ -581,53 +473,22 @@ pub mod proxmox_rs_notify { > /// > /// See [`api::matcher::add_matcher`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn add_matcher( > #[try_from_ref] this: &NotificationConfig, > - name: String, > - target: Option>, > - match_severity: Option>, > - match_field: Option>, > - match_calendar: Option>, > - mode: Option, > - invert_match: Option, > - comment: Option, > - disable: Option, > + matcher_config: MatcherConfig, > ) -> Result<(), HttpError> { > let mut config = this.config.lock().unwrap(); > - api::matcher::add_matcher( > - &mut config, > - MatcherConfig { > - name, > - 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, > - disable, > - origin: None, > - }, > - ) > + api::matcher::add_matcher(&mut config, matcher_config) > } > > /// Method: Update a matcher. > /// > /// See [`api::matcher::update_matcher`]. > #[export(serialize_error)] > - #[allow(clippy::too_many_arguments)] > pub fn update_matcher( > #[try_from_ref] this: &NotificationConfig, > name: &str, > - target: Option>, > - match_severity: Option>, > - match_field: Option>, > - match_calendar: Option>, > - mode: Option, > - invert_match: Option, > - comment: Option, > - disable: Option, > + updater: MatcherConfigUpdater, > delete: Option>, > digest: Option<&str>, > ) -> Result<(), HttpError> { > @@ -637,16 +498,7 @@ pub mod proxmox_rs_notify { > api::matcher::update_matcher( > &mut config, > name, > - MatcherConfigUpdater { > - match_severity, > - match_field, > - match_calendar, > - target, > - mode, > - invert_match, > - comment, > - disable, > - }, > + updater, > delete.as_deref(), > digest.as_deref(), > ) > -- > 2.47.3 > > > > > --