Re: [PATCH pve-manager 1/5] notifications: Add OAuth2 parameters to schema and add/update endpoints

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

On Wed, Feb 11, 2026 at 09:55:23AM +0100, Lukas Wagner wrote:
> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> > Add auth-method, as well as optional
> > oauth2-{client-id,client-secret,tenant-id,refresh-token} parameters to
> > prepare for OAuth2 support.
> >
> > The auth-method parameter was previously implicit and inferred by
> > proxmox-notify based on the presence of a password. It is now made
> > explicit, however still kept optional and explicitly inferred in the
> > {update,create}_endpoint handlers to avoid breaking the API.
> >
> > Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> > ---
> >  PVE/API2/Cluster/Notifications.pm | 55 +++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> > [...]
> >          eval {
> >              PVE::Notify::lock_config(sub {
> >                  my $config = PVE::Notify::read_config();
> > @@ -1208,6 +1258,11 @@ __PACKAGE__->register_method({
> >                      $mode,
> >                      $username,
> >                      $password,
> > +                    $auth_method,
> > +                    $oauth2_client_id,
> > +                    $oauth2_client_secret,
> > +                    $oauth2_tenant_id,
> > +                    $oauth2_refresh_token,
> >                      $mailto,
> >                      $mailto_user,
> >                      $from_address,
> 
> As already explained off-list, I think it's time to switch from a flat
> list of parameters to passing hashes for the parameters for the
> `add_smtp_target` and `update_smtp_target` methods. This means, the
> bindings in proxmox-perl-rs would then directly take
> SmtpConfig/SmtpPrivateConfig and
> SmtpConfigUpdater/SmtpPrivateConfigUpdater. Then the call could look
> something like (not tested)
> 
> $config->add_smtp_endpoint(
>                     $name,
>                     {
>                          server => $server,
>                          port => $port,
>                           ...
>                     },
>                     {
>                         password => $password,
>                         ...
>                     }
>                 );
> 
> This makes it much harder to introduce bugs due to parameter ordering.
> Long-term we should do the same for the other endpoints, but no need to
> do it in this series, I think.
> 

That makes a lot of sense, will update it

> For changes like these and in general it's pretty important to mention
> any breaking changes in the cover letter and maybe patch description,
> since the changes done in this series affect multiple packages that
> *could* be updated independently by our users. For instance, in the
> cover-letter you could write something like:
>    
>    The patch series requires the following version requirement bumps:
> 
>      pve-manager requires bumped proxmox-perl-rs
>      proxmox-perl-rs requires bumped proxmox-notify*
> 
> 
> *.) although for this one it's not that critical, since its only a
> build-dependency, so there is no chance of customer systems breaking due
> to partial system updates
> 
> This way the maintainer knows that the version requirements in
> debian/control must be adapted at some point after applying the patches.

I was wondering how this would work, thanks for the explanation!