[bug#67497] [PATCH] Multiple deploy hooks in certbot service

[Felix Lechner]

Hi,

Thank you both for reviewing this patch! I have to respond to several
reviews and will start with this one, because it weighed the heaviest on
me.

On Sat, Dec 16 2023, Bruno Victal wrote:

> As Arun pointed out, I don't think multiple deploy hooks would be
> adding value here.

Your blanket opposition to this patch is incomprehensible to me from
several angles:

1. A meaningful name for a hook near the certificate declaration is more
administrator-friendly. Someone who manages several certificates, like
my twenty-one certificates [1], can see right away which services are
being restarted.

2. Arun's solution requires an extra procedure and makes the
configuration file longer without without conveying extra meaning.

3. Anyone parsing the code has to look up the definition of the hook in
order to see what it does---and probably also the definition for
'invoke', which is not standard Guile, in the Guix manual. In my view,
your code is not easy to read.

4. The bundling into one script brings no economy, because different
services generally share no code for their reloading. That was already
recognized by Certbot's upstream when the feature for multiple hooks was
added. After all, the concerns can also be combined, as you prefer, in
Certbot's own hooks, but that was apparently unpopular.

5. As a more serious downside, in your cases changing the combined hook
might inadventently reload a certificate for a service does not use
it. A grep is required to check where the cmombined hook is being
used. An extra step is required, and the propensity for errors rises.

6. In your preferred setups, the most elegant way to provide different
hooks is probably '%certbot-hook-1' and 'certbot-hook-2'. Those scripts
will then share code---likely to restart a HTTP server---for no good
reason!

7. User-friendliness is regarded as a worthwhile goal at another, more
popular Linux distribution. [2]

8. Most significantly, your use case isn't affected by this patch! The
use of combined hooks, which you prefer, is still possible should this
patch be accepted.

In summary, I do not understand what motivated you to object to this
patch, but I recognize that the opinions of reasonable people can
differ.

As a side note, I have contributed upstream, but not to the feature we
are discussing here. [3]

> What would be interesting though is adding service-extensions support
> for certbot-service-type. Roughly speaking, two plausible ways to
> achieve this would be:
>
> * Single deploy-hook and ungexp-splicing, i.e.:
>
> [...]
>
> * Multiple --deploy-hook … behind the scenes (the deploy-hook
> field in <certificate-configuration> still accepts only a single hook)

While I very much respect Bruno's opinion and guidance on Guix services
(and genuinely appreciated this review) I do not understand what those
sentences mean.

I guess it's shame on me.

I can, however, say that I likewise fail to see an advantage in more
complexity when my patch does nearly the same thing in three lines.

Thank you!

Kind regards
Felix

[1] 
https://codeberg.org/lechner/system-config/src/commit/b566b08a982a12f896cd6e6666f7849dbac0ce2e/host/wallace-server/operating-system.scm#L1097-L1193
[2] point 4, https://www.debian.org/social_contract.html
[3] https://github.com/certbot/certbot/blob/master/AUTHORS.md