Re: [PATCH] improve nginx-service

[Julien Lepiller]

On Fri, 4 Nov 2016 22:28:07 +0100
Hartmut Goebel <address@hidden> wrote:

> Hi Julien
> 
> thanks for these patches. I applied them to current master, but I did
> not work out how to use the new features. I'd appreciate a more
> complete example.
> 
> 0001-Make-nginx-service-extensible.patch
> > address@hidden {Scheme Variable} nginx-service-type +This is the type for
> > the nginx web server. + +This service can be extended to add more
> > vhosts than the default one. + address@hidden +(simple-service
> > 'my-extra-vhost nginx-service-type + (list
> > (nginx-vhost-configuration (https-port #f)
> > + (root "/var/www/extra-website"))))  
> 
> I do not understand this. Why would I want to to this? Why not just
> add the vhost declaration when defining the service in the system
> declaration? Is this for vhost-types which have preset values for some
> configuration files? Then a different example might be useful, and/or
> some more explanation.

There is no real difference between the two. This is mostly usefull for
implementing new web services in the distro, but it is also good for
separating nginx configuration from web service configuration. I guess
that's a matter of taste.

> 
> It's a bit more obvious with patch 3, but even that example is not
> that obvious to me and could use some more explanation.
> 
> I've build a simple service like the gtk-doc-vhost in your second
> example, but it not manage to make it work: First I got "error: no
> target of type 'nginx'", so I re-added "(nginx-service)" to the system
> declaration. Then I got

Yes, the example adds a service that uses the nginx service, but does
not create it.

> 
>     gnu/services/web.scm:118:34: In procedure
> default-nginx-vhost-config: gnu/services/web.scm:118:34: In procedure
> struct_vtable: Wrong type argument in position 1 (expecting struct):
>     (nginx-vhost-configuration (root taler-landing-page))

What is your configuration exactly, and what is taler-landing-page?

> 
> Si I'd really appreciate seeing a more complete example either in the
> documentation of in gn/systems/examples/
> 
> > (system* (string-append #$nginx "/sbin/nginx") - "-c" #$config-file
> > "-t"))))) + "-c" #$(or config-file + (default-nginx-config
> > log-directory + run-directory vhosts)) + "-t")))))  
> 
> Nitpicking: I'd mode the "-t" to the front, since this is the
> important difference.
> 
> > (define nginx-shepherd-service (match-lambda - (($
> > <nginx-configuration> nginx log-directory run-directory
> > config-file) + (($ <nginx-configuration> nginx log-directory
> > run-directory vhosts config-file) (let* ((nginx-binary (file-append
> > nginx "/sbin/nginx")) (nginx-action (lambda args #~(lambda _ (zero?
> > - (system* #$nginx-binary "-c" #$config-file address@hidden)))))) +
> > (system* #$nginx-binary "-c" #$(or config-file +
> > (default-nginx-config + log-directory + run-directory + vhosts))+
> > address@hidden))))))  
> 
> To avoid duplicate code I suggest moving the "#$(or …)" part into a
> private function - if this is possible.
> 
> 
> 
> 0003-services-Accept-gexps-as-nginx-configuration-value.patch
> > address@hidden +(simple-service 'gtk-doc-vhost nginx-service-type + (list
> > (nginx-vhost-configuration  
> 
> git am says: trailing whitespace
> 
> > + " root " #$(nginx-vhost-configuration-root vhost) ";\n" + " index
> > " #$(config-index-strings (nginx-vhost-configuration-index vhost))
> > ";\n"
> > + " server_tokens " #$(if (nginx-vhost-configuration-server-tokens?
> > vhost) + "on" "off") ";\n"  
> 
> Could you please (maybe in another patch) add a way to include
> additional config lines? Both for the main server and each vhost.I'm
> gioing to need this for adding locations, backends and such.

I'd like to get these patches accepted first, but I'm already working
on adding more configuration lines.

Thanks for your review, I will fix that and come with a new patchset
quickly (with better documentation).

> 
> -- Regards Hartmut Goebel | Hartmut Goebel |
> address@hidden | | www.crazy-compilers.com | compilers
> which you thought are impossible |
> 
> 
>