Re: [PATCH] improve nginx-service

[Julien Lepiller]

On Wed, 19 Oct 2016 23:04:14 +0200
address@hidden (Ludovic Courtès) wrote:

> Hi Julien,
> 
> This looks like a great improvement to me!  Sounds nicer than fiddling
> with config files.
> 
> I suppose we could make ‘nginx-service-type’ extensible (info "(guix)
> Service Types and Services") so that people can write services that
> define new vhosts?

You mean something like udev-service-type where you could extend it
with a list of vhosts?

Except for that comment, I modified the patch following your advice.

> 
> Julien Lepiller <address@hidden> skribis:
> 
> > From 613d5db739d4010be2037fd2fcfc70baca4625aa Mon Sep 17 00:00:00
> > 2001 From: Julien Lepiller <address@hidden>
> > Date: Mon, 26 Sep 2016 23:55:58 +0200
> > Subject: [PATCH] services: improve nginx-service configuration
> >
> > * gnu/services/web.scm (<nginx-vhost-configuration>): New record
> > type.
> > * doc/guix.texi (Web Services): Document
> > 'nginx-vhost-configuration'.  
> 
> Please mention the other changes in web.scm: new procedures, changes
> in existing procedures, etc.
> 
> > +As an alternative to using a @var{config-file}, @var{vhost-list}
> > can be +used to specify the list of vhosts required on the host.
> > For this to work,  
> 
> “the list of @dfn{virtual hosts} or @dfn{vhosts}”
> 
> > address@hidden {Data Type} nginx-vhost-configuration
> > +Data type representing the configuration of an nginx virtual
> > host.  
> 
> @dfn{virtual host}
> 
> > +This type has the following parameters:
> > address@hidden @asis
> > address@hidden @code{http-port} (default: 80)  
> 
> @code{80}; likewise for all the other default values.
> 
> > +Nginx will listen for http connection on this port. Set it at #f
> > if nginx should +not listen for http (non secure) connection for
> > this vhost.  
> 
> s/http/HTTP/ and s/#f/@code{#f}/
> Please leave two spaces after an end-of-sentence period.
> 
> > +(define (list-names names)
> > + (match names
> > +  ('() "")
> > +  ((head tail ...) (string-append (if (eq? head 'default) "_" head)
> > +                                  " "
> > +                                  (list-names tail)))))  
> 
> Maybe call it ‘config-value-strings’?  Please add a docstring.  I
> think it’d be more readable like this:
> 
>   (define (config-value-strings names)
>     "Return a string denoting the nginx config representation of
> NAMES, a list of foobars…"
>     (string-concatenate
>      (map (match-lambda
>             ('default "_")
>             ((? string? str) str))
>           names)))
> 
> Could you send an updated patch?
> 
> Unless David, who initially wrote this service has objections, this
> patch looks good to me with the changes as suggested above.
> 
> Thanks!
> 
> Ludo’.

0001-services-improve-nginx-service-configuration.patch
Description: Text Data