guix-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] improve nginx-service


From: Ludovic Courtès
Subject: Re: [PATCH] improve nginx-service
Date: Wed, 19 Oct 2016 23:04:14 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)

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?

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’.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]