Re: [PATCH proxmox-perl-rs 1/1] notify (smtp): add oauth2 parameters to bindings

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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 <a.bied-charreton@proxmox.com>
> > ---
> >  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, &notification)
> >      }
> >  
> > +    /// 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<u16>,
> > -        mode: Option<SmtpMode>,
> > -        username: Option<String>,
> > -        password: Option<String>,
> > -        mailto: Option<Vec<String>>,
> > -        mailto_user: Option<Vec<String>>,
> > -        from_address: String,
> > -        author: Option<String>,
> > -        comment: Option<String>,
> > -        disable: Option<bool>,
> > +        smtp_config: SmtpConfig,
> > +        smtp_private_config: SmtpPrivateConfig,
> > +        oauth2_refresh_token: Option<String>,
> >      ) -> 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<String>,
> > -        port: Option<u16>,
> > -        mode: Option<SmtpMode>,
> > -        username: Option<String>,
> > -        password: Option<String>,
> > -        mailto: Option<Vec<String>>,
> > -        mailto_user: Option<Vec<String>>,
> > -        from_address: Option<String>,
> > -        author: Option<String>,
> > -        comment: Option<String>,
> > -        disable: Option<bool>,
> > +        smtp_config_updater: SmtpConfigUpdater,
> > +        smtp_private_config_updater: SmtpPrivateConfigUpdater,
> > +        oauth2_refresh_token: Option<String>,
> >          delete: Option<Vec<DeleteableSmtpProperty>>,
> >          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