[bug#61740] [PATCH v3] services: Add rspamd-service-type. (was [bug#61740] [PATCH] services: Add rspamd-service-type.)

[Bruno Victal]

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---

> +  (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)

> +  (pid-file
> +   (string "/var/run/rspamd/rspamd.pid")
> +   "Where to store the PID file.")

Is it useful to expose this?


> +  (insecure?
> +   (boolean #f)
> +   "Ignore running workers as privileged users (insecure).")

To me it seems redundant to restate “(insecure)” in the description.

> +                     (make-forkexec-constructor
> +                      (list #$rspamd "-c" #$config-file

I'd prefer the long-name --config over the shorter ones here.

> +                            "--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?

> +     (service-extension profile-service-type
> +                        (compose list rspamd-configuration-package))

What's the motivation for adding the rspamd package to the profile?

> +(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.

> +          ;; 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---

> +          (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---

Note: I used (rspamd-configuration) since it you're using the default
rspamd-configuration value here.

> +(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.

-- 
Furthermore, I consider that nonfree software must be eradicated.

Cheers,
Bruno.

OpenPGP_signature.asc
Description: OpenPGP digital signature