From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id A12001FF136 for ; Mon, 23 Mar 2026 13:27:10 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 32B90161FA; Mon, 23 Mar 2026 13:26:56 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Mon, 23 Mar 2026 13:26:20 +0100 Message-Id: Subject: Re: [PATCH proxmox-perl-rs 1/1] notify (smtp): add oauth2 parameters to bindings From: "Lukas Wagner" To: "Arthur Bied-Charreton" , Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260213160415.609868-1-a.bied-charreton@proxmox.com> <20260213160415.609868-9-a.bied-charreton@proxmox.com> In-Reply-To: <20260213160415.609868-9-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774268734764 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: O7USZSAQHSTDCG6DYIWBOES37LAXZO74 X-Message-ID-Hash: O7USZSAQHSTDCG6DYIWBOES37LAXZO74 X-MailFrom: l.wagner@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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri Feb 13, 2026 at 5:04 PM CET, Arthur Bied-Charreton wrote: > Update SMTP bindings to take the Smtp(Private)?Config structs directly, > and additionally the OAuth2 refresh token. The commit message should probably go into more detail on *why* we now use the struct itself instead of enumerating all parameters in the function signature. The addition of 'trigger_state_refresh' should probably its own commit. The change of parameters for the existing functions is a breaking change, so I think it would be nice to have these separated from the addition of this new function. > > Signed-off-by: Arthur Bied-Charreton > --- > common/src/bindings/notify.rs | 82 ++++++++++++----------------------- > 1 file changed, 28 insertions(+), 54 deletions(-) > > diff --git a/common/src/bindings/notify.rs b/common/src/bindings/notify.r= s > index 409270a..137cc79 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; > =20 > - use anyhow::{Error, bail}; > + use anyhow::{bail, Error}; > use serde_json::Value as JSONValue; > =20 > use perlmod::Value; > @@ -26,7 +26,7 @@ pub mod proxmox_rs_notify { > DeleteableSendmailProperty, SendmailConfig, SendmailConfigUpdate= r, > }; > use proxmox_notify::endpoints::smtp::{ > - DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpMode,= SmtpPrivateConfig, > + DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPriva= teConfig, > SmtpPrivateConfigUpdater, > }; > use proxmox_notify::endpoints::webhook::{ > @@ -36,7 +36,7 @@ pub mod proxmox_rs_notify { > CalendarMatcher, DeleteableMatcherProperty, FieldMatcher, MatchM= odeOperator, MatcherConfig, > MatcherConfigUpdater, SeverityMatcher, > }; > - use proxmox_notify::{Config, Notification, Severity, api}; > + use proxmox_notify::{api, Config, Notification, Severity}; > =20 > /// A notification catalog instance. > /// > @@ -141,6 +141,19 @@ pub mod proxmox_rs_notify { > api::common::send(&config, ¬ification) > } > =20 > + /// Method: Refresh the state for all endpoints. > + /// > + /// This iterates through all configured targets, refreshing their s= tate if needed. > + /// > + /// See [`api::common::refresh_targets`] > + #[export(serialize_error)] > + pub fn trigger_state_refresh( > + #[try_from_ref] this: &NotificationConfig, > + ) -> Result<(), HttpError> { > + let config =3D this.config.lock().unwrap(); > + api::common::trigger_state_refresh(&config) > + } > + > /// Method: Get a list of all notification targets. > /// > /// See [`api::get_targets`]. > @@ -390,37 +403,16 @@ pub mod proxmox_rs_notify { > #[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, > + oauth2_refresh_token: Option, > ) -> Result<(), HttpError> { > let mut config =3D 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 }, > + smtp_config, > + smtp_private_config, > + oauth2_refresh_token, > ) > } > =20 > @@ -432,17 +424,9 @@ pub mod proxmox_rs_notify { > 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, > + smtp_config_updater: SmtpConfigUpdater, > + smtp_private_config_updater: SmtpPrivateConfigUpdater, > + oauth2_refresh_token: Option, > delete: Option>, > digest: Option<&str>, > ) -> Result<(), HttpError> { > @@ -452,19 +436,9 @@ 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 }, > + smtp_config_updater, > + smtp_private_config_updater, > + oauth2_refresh_token, > delete.as_deref(), > digest.as_deref(), > ) I wondered (and we shortly discussed off-list) whether it would make sense to keep the existing functions as is and then add two *new* functions which offer the 'new' struct/hash-based API. It would have the benefit of being a non-breaking change which would ease life for maintainers a bit, but then on the other hand it's also a bit weird since we'd want to remove the old functions pretty soon anyway -- we have only one caller to worry about anyway. When removing the old variants we'd have to bump the version contraints anyway, so in reality, it does not help that much. So after some thought, I think the current approach is fine.