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,
> > + },
> > };
> > }
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
