Hi Saku,

On 2023-12-08 08:17, Saku Laesvuori wrote:
> On Wed, Dec 06, 2023 at 02:58:19PM +0000, Bruno Victal wrote:
>> On 2023-09-16 21:10, Saku Laesvuori wrote:
>>> +                            "--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.

Forwarded with [1].

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

I think it's better to omit this, users who are interested in the tools
can use 'guix shell rspamd'.

> +(define (list-of-symbols? x)
> +  (and (list? x)
> +       (every symbol? x)))

list-of-symbols? is already defined in (gnu services configuration),
you can omit this.

> +        (shepherd-action
> +         (name 'reopenlog)
> +         (documentation "Reopen log files.")

Missed this in my previous reply, I'd prefer naming this action as
'reopen instead.

> +(define %rspamd-os
> +  (simple-operating-system
> +   (service dhcp-client-service-type)

[…]

> +   (service rspamd-service-type
> +            (rspamd-configuration
> +              (shepherd-requirements '(networking))
> +              (local.d-files `(("worker-controller.inc"
> +                                ,(plain-file
> +                                   "rspamd-public-web-controller.conf"
> +                                   "bind_socket = \"0.0.0.0:11334\";"))))))))

I wonder if you could remove dhcp-client-service-type and use the
loopback device for this test instead, by binding to '[::1]' or '127.0.0.1'.
(You don't need to add %loopback-static-networking here since it is already
included in %base-services.)

> +(define (run-rspamd-test)
> +  "Return a test of an OS running Rspamd service."
> +
> +  (define rspamd-ports
> +    '((22668 . 11334)))    ;; web controller

[…]

> +
> +  (define vm
> +    (virtual-machine
> +     (operating-system (marionette-operating-system
> +                        %rspamd-os
> +                        #:imported-modules '((gnu services herd))))
> +     (port-forwardings rspamd-ports)))

[…]

> +          ;; Check that we can access the web ui
> +
> +          (test-equal "http-get"
> +            200
> +            (response-code (http-get "http://localhost:22668/"))) ; HEAD is unsupported

Actually I've realized that these port-forwards are unnecessary
and it would be better to instead do:

--8<---------------cut here---------------start------------->8---
;; Note: remove (web client) and (web response) in the imports above
;; i.e. after the #~(begin (use-modules …

(test-equal "http-get"
  200
  (marionette-eval
   '(begin
      (use-modules (web client)
                   (web response))
      (response-code (http-head "http://localhost:11334/")))
   marionette))
--8<---------------cut here---------------end--------------->8---


[1]: <https://github.com/rspamd/rspamd/issues/4736>

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

Cheers,
Bruno.