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

[paul]

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