Re: [pve-devel] [PATCH access-control 1/2] api: domains: add off-by-default `check-connection` parameter

[Christoph Heiss <c.heiss@proxmox.com>]

Thanks for the review!

On Fri, Jul 28, 2023 at 10:29:26AM +0200, Lukas Wagner wrote:
>
> On Thu Jul 27, 2023 at 3:33 PM CEST, Christoph Heiss wrote:
> > [..]
>
> I think it would be enough to have the 'check-connection' parameter only for
> the API call itself, I wouldn't store it in the domains.cfg configuration
> file.
>
> That would imply that the 'Check configuration' checkbox that you introduce in
> the next patch could *always* be ticked, even for old realms. So whenever you
> create/update an LDAP/AD realm configuration you have to explicitly tell it
> "hey, I do not want the check right now, my LDAP server is down currently".
>
> My main point in making the check behavior opt-in rather was so that
> scripts/API consumers continue to work as before. For the GUI however, it
> should be fine to just always check by default, unless the behavior is
> explicitly turned off.
My thought here was that if you once explicitly turn it off, it should
stay off (if that's needed for /some/ reason). But OTOH, making it
always opt-out for the GUI definitely makes sense too.

I'll give the series a new spin with your suggestions implemented, seems
sensible enough to me. Most importantly, we can get rid of the regex in
the end :^)

>
> > +	},
> > +	'check-connection' => {
> > +	    description => 'Check bind connection to LDAP server.',
> > +	    type => 'boolean',
> > +	    optional => 1,
> > +	    # TODO: Make it enabled-by-default with PVE 9.0?
>                 ^ This wouldn't be necessary any more.
> > +	    default => 0,
> > +	},
> >      };
> >  }
>