On Wed, Dec 06, 2023 at 02:58:19PM +0000, Bruno Victal wrote:
> Hi Saku,
> 
> Some comments:
> 
> > +(define (directory-tree? xs)
> > +  (match xs
> > +    (((file-name file-like) ...)
> > +     (and (every string? file-name)
> > +          (every file-like? file-like)))
> > +    (_ #f)))
> 
> You can express this more compactly as:
> 
> --8<---------------cut here---------------start------------->8---
> (define directory-tree?
>   (match-lambda
>     ((((? string?) (? file-like?)) ...) #t)
>     (_ #f)))
> --8<---------------cut here---------------end--------------->8---

Done in v4.

> 
> > +  (user
> > +   (string "rspamd")
> > +   "The user to run rspamd as.")
> > +  (group
> > +   (string "rspamd")
> > +   "The group to run rspamd as.")
> 
> How about using user-account and user-group records instead? (see
> vnstat-service-type for an example)

Done in v4.

> 
> > +  (pid-file
> > +   (string "/var/run/rspamd/rspamd.pid")
> > +   "Where to store the PID file.")
> 
> Is it useful to expose this?

I don't know. It was there when I picked up this patch but I can't come
up with a case in which one would want to change it. Removed in v4.

> 
> 
> > +  (insecure?
> > +   (boolean #f)
> > +   "Ignore running workers as privileged users (insecure).")
> 
> To me it seems redundant to restate “(insecure)” in the description.

True. Removed in v4.

> 
> > +                     (make-forkexec-constructor
> > +                      (list #$rspamd "-c" #$config-file
> 
> I'd prefer the long-name --config over the shorter ones here.

Done in v4.

> > +                            "--var" (string-append "LOCAL_CONFDIR=" #$local-confdir)
> 
> Curiously I don't see this listed in the 'rspamd' manpage although
> it is on the 'rspamadm' one. Can you confirm whether this works
> and if so, report to upstream that their docs are missing this?

It does work; I've used it since before I submitted this patch. The
`--var` option is listed on `rspamd --help`. Unfortunately, Rspamd
tracks their issues on Github and I'd prefer not registering an account
there.

> > +     (service-extension profile-service-type
> > +                        (compose list rspamd-configuration-package))
> 
> What's the motivation for adding the rspamd package to the profile?

That was also there when I picked up this patch. I assume it is added to
the profile so that the `rspamadm` and `rspamc` programs are available
and compatible with the daemon. I don't have strong feelings about this
in either direction.

> > +(define %rspamd-os
> > +  (simple-operating-system
> > +   (service dhcp-client-service-type)
> > +   (service rspamd-service-type)))
> 
> Is 'dhcp-client-service-type' needed for this system test?
> I haven't tested it but it looks unnecessary to me.

It provides 'networking for the http test. Apparently the test wasn't
working yet anyway (I had no experience in Guix tests when I sent my
versions of the patch and just assumed that they were working in Thomas'
version). The tests are now fixed in v4.

> > +          ;; Check that we can access the web ui
> > +          (test-equal "http-get"
> > +            200
> > +            (begin
> > +              (let-values (((response text)
> > +                            (http-get "http://localhost:22668/"
> > +                                      #:decode-body? #t)))
> > +                (response-code response))))
> 
> IMO if you're only interested in the HTTP response code a http-head
> is the better option, unless the program handles those requests
> differently. Also, since 'text' isn't used you can simplify this to:
> 
> --8<---------------cut here---------------start------------->8---
> ;; Don't forget to remove the unused (srfi srfi-11) import.
> 
> (test-equal "Web UI is accessible"
>   200
>   (response-code (http-head "http://localhost:22668/")))
> --8<---------------cut here---------------end--------------->8---

Done in v4.

> > +          (test-assert "rspamd pid ready"
> > +            (marionette-eval
> > +             '(file-exists? "/var/run/rspamd/rspamd.pid")
> > +             marionette))
> 
> There's a procedure dedicated for this:
> 
> --8<---------------cut here---------------start------------->8---
> (test-assert "rspamd pid ready"
>   (wait-for-file #$(rspamd-configuration-pid-file (rspamd-configuration)) marionette)))
> --8<---------------cut here---------------end--------------->8---

Done in v4.

> > +(define %test-rspamd
> > +  (system-test
> > +   (name "rspamd")
> > +   (description "Send an email to a running rspamd server.")
> > +   (value (run-rspamd-test))))
> 
> I'd change the description to something like "Basic rspamd service test."
> as the current one is misleading.

Done in v4.