Subject: Re: bug#26306: exim service patches (including mail-aliases-service-type)

From: [email-masked] (Ludovic Courtès)
Date: Fri, 07 Apr 2017 23:25:23 +0200

Hi Carlo,

Carlo Zancanaro <[email-masked]> skribis:

> I've wanted to fix up the exim service given Ludo's comments on my last
> few patches (see #25789), so here are some patches to introduce a
> mail-aliases-service-type, to change exim-service-type to use it
> (instead of doing its own thing), and to add a system test for
> exim-service-type.
>
> I haven't yet written the updates necessary to the documentation, but I
> thought I'd send these now to get feedback before I do the work to fix
> up the docs.

This all LGTM.

Minor comments:

> From 11a5223f4a9487d3a9a17925488e18e80baec843 Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <[email-masked]>
> Date: Thu, 30 Mar 2017 15:25:58 +1100
> Subject: [PATCH 1/3] services: Add mail-aliases-service-type.
>
> * gnu/services/mail.scm (mail-aliases-etc): New procedure.
> (mail-aliases-service-type): New variable.

[...]

> +(define (mail-aliases-etc aliases)
> +  `(("aliases" ,(plain-file "aliases"
> +                            ;; Ideally we'd use a format string like
> +                            ;; "~:{~a: ~{~a~^,~}\n~}", but it gives a
> +                            ;; warning that I can't figure out how to fix,
> +                            ;; so we'll just use string-join below instead.
> +                            (format #f "~:{~a: ~a\n~}"
> +                                    (map (lambda (entry)
> +                                           (list (car entry)
> +                                                 (string-join (cdr entry) ",")))

Please avoid car/cdr:

  (match-lambda
    ((user aliases ...)
     (list user (string-joint aliases ","))))

> +(define mail-aliases-service-type
> +  (service-type
> +   (name 'mail-aliases)
> +   (extensions
> +    (list (service-extension etc-service-type mail-aliases-etc)))
> +   (compose concatenate)
> +   (extend append)))

If you add it to guix.texi (which would be nice) please leave a note as
to what the values are (a list of user/addresses lists, right?).

> From 8ac4f5fba3420ba5525cd7dff93d30f7fed8b0ae Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <[email-masked]>
> Date: Thu, 30 Mar 2017 15:28:26 +1100
> Subject: [PATCH 2/3] services: Make exim-service-type use
>  mail-aliases-service-type
>
> * gnu/services/mail.scm (exim-configuration)[aliases]: Remove field.
> (exim-activation, exim-shepherd-service): Remove alias from matches.
> (exim-etc): Remove procedure.
> (exim-service-type): Extend mail-aliases-service-type instead of
> etc-service-type.

OK.

> From 984298f4cea4ac3bff530a4a767bf96567ec284f Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <[email-masked]>
> Date: Thu, 30 Mar 2017 15:13:56 +1100
> Subject: [PATCH 3/3] tests: mail: Add test for exim
>
> * gnu/tests/mail.scm (%exim-os, %test-exim): New variables.
> (run-exim-test): New procedure.

Nice test!

> +(define %exim-os
> +  (operating-system
> +    (host-name "komputilo")

Due to 892d9089a88abaa2ef1127f16308d03f4f08a4ce, this should use
‘simple-operating-system’.  Apologies!

With a guix.texi update, it’ll be perfect.

Could you send updated patches?

Thanks!

Ludo’.