Subject: [bug#67497] [PATCH v2 4/4] In certbot's client configuration, offer multiple deploy-hooks.

From: Maxim Cournoyer <[email-masked]>
Date: Thu, 01 May 2025 23:47:33 +0900

Hi,

Felix Lechner <[email-masked]> writes:

> The certbot program can accept multiple deploy hooks by repeating the relevant
> option on the command line. This commit makes that capability available to
> users.
>
> Certificates are often used to secure multiple services. It is helpful to have
> separate hooks for each service. It makes those hooks easier to maintain. It's
> also easier that way to re-use a hook for another certificate that may not
> serve to secure the same combination of services.

For this commit and the previous one, you can keep your nice explanatory
text, but a GNU ChangeLog must be added below, per our conventions.  I
can be terse and to the point, touching only the *changes*, especially
since you already have an explanatory text.

> Change-Id: I3a293daee47030d9bee7f366605aa63a14e98e38
> ---
>  doc/guix.texi            | 11 ++++++-----
>  gnu/services/certbot.scm | 18 ++++++++++++++++--
>  2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 1b0fa4f2a3a..deb1f76d353 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -35378,7 +35378,7 @@ Certificate Services
>             (list
>              (certificate-configuration
>               (domains '("example.net" "www.example.net"))
> -             (deploy-hook %nginx-deploy-hook))
> +             (deploy-hooks '(%nginx-deploy-hook)))
>              (certificate-configuration
>               (domains '("bar.example.net")))))))
>  @end lisp
> @@ -35483,14 +35483,15 @@ Certificate Services
>  additionally @code{$CERTBOT_AUTH_OUTPUT} will contain the standard output
>  of the @code{auth-hook} script.
>  
> -@item @code{deploy-hook} (default: @code{#f})
> -Command to be run in a shell once for each successfully issued
> -certificate.  For this command, the environment variable
> +@item @code{deploy-hooks} (default: @code{'()})
> +Commands to be run in a shell once for each successfully issued
> +certificate.  For these commands, the environment variable
>  @code{$RENEWED_LINEAGE} will point to the config live subdirectory (for
>  example, @samp{"/etc/letsencrypt/live/example.com"}) containing the new
>  certificates and keys; the environment variable @code{$RENEWED_DOMAINS} will
>  contain a space-delimited list of renewed certificate domains (for
> -example, @samp{"example.com www.example.com"}.
> +example, @samp{"example.com www.example.com"}. Please note that the singular
> +field @code{deploy-hook} was replaced by this field in the plural.

Need two space before the new sentence starts.

>  
>  @item @code{start-self-signed?} (default: @code{#t})
>  Whether to generate an initial self-signed certificate during system
> diff --git a/gnu/services/certbot.scm b/gnu/services/certbot.scm
> index 08a480ed3b1..7a67b9bd7cb 100644
> --- a/gnu/services/certbot.scm
> +++ b/gnu/services/certbot.scm
> @@ -30,6 +30,7 @@ (define-module (gnu services certbot)
>    #:use-module (gnu services web)
>    #:use-module (gnu system shadow)
>    #:use-module (gnu packages tls)
> +  #:use-module (guix deprecation)
>    #:use-module (guix i18n)
>    #:use-module (guix records)
>    #:use-module (guix gexp)
> @@ -63,8 +64,11 @@ (define-record-type* <certificate-configuration>
>                         (default #f))
>    (cleanup-hook        certificate-cleanup-hook
>                         (default #f))
> +  ;; TODO: remove singular deploy-hook; is deprecated

For standalone comments, please use complete sentences, as in:

     ;; TODO: Remove singular deploy-hook, which is deprecated.

Note that it's not enough to simply document it as deprecated, you must
introduce a deprecation warning when people are using it for the
'deprecation' to count as such.  Since this record is using a plain Guix
record, this is usually done using a sanitizer with a maybe value that
warns when the value is set.

>    (deploy-hook         certificate-configuration-deploy-hook
>                         (default #f))
> +  (deploy-hooks        certificate-configuration-deploy-hooks
> +                       (default '()))
>    (start-self-signed?  certificate-configuration-start-self-signed?
>                         (default #t)))
>  
> @@ -140,7 +144,8 @@ (define certbot-command
>                (match-lambda
>                  (($ <certificate-configuration> custom-name domains challenge
>                                                  csr authentication-hook
> -                                                cleanup-hook deploy-hook)
> +                                                cleanup-hook
> +                                                deploy-hook deploy-hooks)
>                   (let ((name (or custom-name (car domains))))
>                     (append
>                      (list name
> @@ -168,7 +173,16 @@ (define certbot-command
>                          (list "--register-unsafely-without-email"))
>                      (if server (list "--server" server) '())
>                      (if rsa-key-size (list "--rsa-key-size" rsa-key-size) '())
> -                    (if deploy-hook (list "--deploy-hook" deploy-hook) '())))))
> +
> +                    (if deploy-hook
> +                        (begin
> +                          (warn-about-deprecation 'deploy-hook #f
> +                                                  #:replacement 'deploy-hooks)

Ah, I see you warned here, but that's going to happen at the time the
service is executed, right?  Which is not as good: we'd like the
deprecation to be printed as early as possible, typically when the user
reconfigures their system.  A sanitizer on the record field would
achieve that.

-- 
Thanks,
Maxim