Subject: Re: [bug#32771] [PATCH 2/2] services: Add Varnish service.

From: Marius Bakke <[email-masked]>
Date: Wed, 26 Sep 2018 00:52:51 +0200

Christopher Baines <[email-masked]> writes:

> Hey Marius,
>
> I've not got much experience with Varnish, but this patch applies, and
> the system test passes :)
>
> Marius Bakke <[email-masked]> writes:
>
>> +@subsubheading Varnish Cache
>> +@cindex Varnish
>> +Varnish is a fast cache server that sits in between web applications
>> +and end users.  It proxies requests from clients and caches the
>> +accessed URLs such that multiple requests for the same resource only
>> +creates one request to the back-end.
>> +
>> +@defvr {Scheme Variable} varnish-service-type
>> +A service type for the Varnish daemon.
>> +@end defvr
>
> Given there are not other service types for Varnish in Guix, "The
> service type ..." would probably be clearer here, or just "Service type
> ...".

Just "Service type ..." is better, thanks!

>> +@deftp {Data Type} varnish-configuration
>> +Data type representing the @code{varnish} service configuration.
>> +This type has the following parameters:
>> +
>> +@table @asis
>> +@item @code{package} (default: @code{varnish})
>> +The Varnish package to use.
>> +
>> +@item @code{name} (default: @code{"default"})
>> +A name for this Varnish instance.  Varnish will create a directory in
>> +@file{/var/varnish/} with this name and keep temporary files there.  If
>> +the name starts with a forward slash, it is interpreted as an absolute
>> +directory name.
>
> Most services in Guix use /var/lib for data, would this work for
> Varnish?

Probably, although I didn't bother trying it.  Those files are temporary
anyway, perhaps /var/cache/varnish would be better?  But, I think the
upstream default is OK.

> Also, I wonder if you'd considered supporing running multiple instances
> of varnishd, I guess the "name" might come in useful then.

If you read the system test closely, you'll notice it does actually run
multiple instances, one proxying the other, and testing different
aspects of the service definition :-)

>> +@c Varnish does not support HTTPS, so keep this URL to avoid confusion.
>> +For example, to mirror @url{http://www.gnu.org,www.gnu.org} with VCL you
>> +can do something along these lines:
>
> Does "@c" mean a comment?

Yes.

>> +(define %varnish-accounts
>> +  (list (user-group
>> +         (name "varnish")
>> +         (system? #t))
>> +        (user-account
>> +         (name "varnish")
>> +         (group "varnish")
>> +         (system? #t)
>> +         (comment "Varnish Cache User")
>> +         (home-directory "/var/varnish")
>> +         (shell (file-append shadow "/sbin/nologin")))))
>> +
>> +(define %varnish-activation
>> +  #~(begin
>> +      (use-modules (guix build utils))
>> +      (let ((home-dir "/var/varnish")
>> +            (user (getpwnam "varnish")))
>> +        (mkdir-p home-dir)
>> +        (chown home-dir (passwd:uid user) (passwd:gid user))
>> +        (chmod home-dir #o755))))
>
> Is this necessary, as I think the users home directory might be
> automatically created?

You are correct!  I removed the activation script.

> Anyway, this looks pretty much good to me.

Thank you very much for reviewing :-)

Pushed as 3b97a1779f3b65d582b8edc8c154b6414314b946.