From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id EDFD91FF136 for ; Mon, 23 Mar 2026 17:44:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0842A327C8; Mon, 23 Mar 2026 17:44:22 +0100 (CET) Date: Mon, 23 Mar 2026 17:44:15 +0100 From: Arthur Bied-Charreton To: Lukas Wagner Subject: Re: [PATCH proxmox-perl-rs 1/1] notify (smtp): add oauth2 parameters to bindings Message-ID: References: <20260213160415.609868-1-a.bied-charreton@proxmox.com> <20260213160415.609868-9-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774284210835 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.830 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: 27JX5T7MF3YP3P4D4XVGG277GCHQXDRX X-Message-ID-Hash: 27JX5T7MF3YP3P4D4XVGG277GCHQXDRX X-MailFrom: a.bied-charreton@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: 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: On Mon, Mar 23, 2026 at 01:26:20PM +0100, Lukas Wagner wrote: > 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. > ACK > 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. > ACK, will split this commit > > > > 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.rs > > 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; > > > > - use anyhow::{Error, bail}; > > + use anyhow::{bail, Error}; > > use serde_json::Value as JSONValue; > > > > use perlmod::Value; > > @@ -26,7 +26,7 @@ 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::{ > > @@ -36,7 +36,7 @@ pub mod proxmox_rs_notify { > > CalendarMatcher, DeleteableMatcherProperty, FieldMatcher, MatchModeOperator, MatcherConfig, > > MatcherConfigUpdater, SeverityMatcher, > > }; > > - use proxmox_notify::{Config, Notification, Severity, api}; > > + use proxmox_notify::{api, Config, Notification, Severity}; > > > > /// A notification catalog instance. > > /// > > @@ -141,6 +141,19 @@ pub mod proxmox_rs_notify { > > api::common::send(&config, ¬ification) > > } > > > > + /// Method: Refresh the state for all endpoints. > > + /// > > + /// This iterates through all configured targets, refreshing their state if needed. > > + /// > > + /// See [`api::common::refresh_targets`] > > + #[export(serialize_error)] > > + pub fn trigger_state_refresh( > > + #[try_from_ref] this: &NotificationConfig, > > + ) -> Result<(), HttpError> { > > + let config = 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 = 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, > > ) > > } > > > > @@ -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. Yes, I agree, as discussed off-list, the naming for those new functions would be quite awkward ^^ I will make the breaking change clear in the commit message for v2