[bug#66160] [PATCH] gnu: Add oci-container-service-type.

[paul via Guix-patches via <guix-patches@gnu.org>]

Hi,

On 10/5/23 16:30, Ludovic Courtès wrote:
> Hi,
>
> Giacomo Leidi <goodoldpaul@autistici.org> skribis:
>
>> * gnu/services/docker.scm (oci-container-configuration): New variable;
>> (oci-container-shepherd-service): new variable;
>> (oci-container-service-type): new variable.
>> * doc/guix.texi: Document it.
> Neat!
>
>> +@cindex OCI-backed, Shepherd services
>> +@subsubheading OCI backed services
>> +
>> +Should you wish to manage your Docker containers with the same consistent
>> +interface you use for your other Shepherd services,
>> +@var{oci-container-service-type} is the tool to use.
> Perhaps expound a bit, like:
>
>    … is the tool to use: given an @acronym{Open Container Initiative,
>    OCI} container image, it will run it in a Shepherd service.  One
>    example where this is useful: it lets you run services that are
>    available as Docker/OCI images but not yet packaged for Guix.
nice thank you, fixed.
>
>> +@defvar oci-container-service-type
>> +
>> +This is a thin wrapper around Docker's CLI that wraps OCI images backed
>> +processes as Shepherd Services.
>> +
>> +@lisp
>> +(simple-service 'oci-grafana-service
>> +                (list
>> +                 (oci-container-configuration
> The second argument to ‘simple-service’ is missing.
Good catch, fixed.
>
>> +                  (image "prom/prometheus")
>> +                  (network "host")
>> +                  (ports
>> +                    '(("9000" . "9000")
>> +                      ("9090" . "9090"))))))
>> +                 (oci-container-configuration
>> +                  (image "grafana/grafana:10.0.1")
>> +                  (network "host")
>> +                  (volumes
>> +                    '("/var/lib/grafana:/var/lib/grafana"))))))
>> +@end lisp
> Please explain the example in one or two sentences.
>
> Personally, I’d like to know how the image names are resolved; would be
> nice to mention it in the doc.
[ ... ]
>
>> +@table @asis
>> +@item @code{command} (default: @code{()}) (type: list-of-strings)
>> +Overwrite the default CMD of the image.
> “… the default command (@code{CMD}) of the image.”
[ ... ]
>
>> +@item @code{entrypoint} (default: @code{""}) (type: string)
>> +Overwrite the default ENTRYPOINT of the image.
> Likewise.
Fixed, thank you.
>
>> +@item @code{environment} (default: @code{()}) (type: list)
>> +Set environment variables. This can be a list of pairs or strings, even mixed:
>> +
>> +@lisp
>> +(list '("LANGUAGE" . "eo:ca:eu")
>> +      "JAVA_HOME=/opt/java")
> I would choose one or the other, but not both.
I would like to allow some kind of escape (the same way the nice Guix 
configuration records provide an extra-content field which is literally 
appended to the config) in case there's some something I didn't foresee 
with this implementation. It may be paranoia, I don't have a strong 
opinion. are you strongly against supporting the two formats?
>
>> +@item @code{ports} (default: @code{()}) (type: list)
>> +Set the port or port ranges to expose from the spawned container. This can be a
>> +list of pairs or strings, even mixed:
>> +
>> +@lisp
>> +(list '("8080" . "80")
>> +      "10443:443")
> Likewise.
>
>> +(define (oci-sanitize-pair pair delimiter)
>> +  (cond ((file-like? (car pair))
>> +         (file-append (car pair) delimiter (cdr pair)))
> Please use ‘match’ instead of car/cdr (info "(guix) Data Types and
> Pattern Matching").
Thank you, fixed.
>
>> +         (error
>> +          (format #f "pair members must only contain gexps, file-like objects and strings but ~a was found" (car pair))))))
> Should be (raise (formatted-message (G_ …))).  That way we get i18n
> support and the message is presented like other error messages.

[ ... ]
>
>> +            (error
>> +             (format #f "~a members must be either a string or a pair but ~a was found!" name el)))))
> Ditto.

[ ... ]
>
>> +    (shepherd-service (provision `(,(string->symbol name)))
>> +                      (requirement '(dockerd))
> Actually: (requirement '(dockerd user-processes)).

[ ... ]
>
>> +                (description
>> +                 "This service provides allows the management of Docker
>> +containers as Shepherd services.")))
> “Docker and OCI containers”
Fixed.
> Could you send an updated patch?

I should have addressed all of your comments besides the one on the 
key-value format. I'm sending an updated patch.


Thank you for your time and effort,


giacomo