Re: [PATCH] improve nginx-service

[Hartmut Goebel]

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.

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

    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))

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.

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