Package: guix-patches;
Reported by: Felix Lechner <felix.lechner <at> lease-up.com>
Date: Mon, 27 Nov 2023 20:24:01 UTC
Severity: normal
Tags: help, moreinfo, patch
Message #60 received at 67497 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Felix Lechner <felix.lechner <at> lease-up.com> Cc: 67497 <at> debbugs.gnu.org, Carlo Zancanaro <carlo <at> zancanaro.id.au>, Bruno Victal <mirai <at> makinata.eu> Subject: Re: [PATCH v2 4/4] In certbot's client configuration, offer multiple deploy-hooks. Date: Thu, 01 May 2025 23:47:33 +0900
Hi,
Felix Lechner <felix.lechner <at> lease-up.com> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.