Ludovic Courtès <ludo@gnu.org> writes:

> Hello,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> * gnu/services/web.scm (<apache-httpd-load-module>,
>>   <apache-httpd-config-file>, <apache-httpd-virtualhost>
>>   <apache-httpd-configuration>): New record types.
>>   (%default-apache-httpd-modules, %apache-httpd-accounts,
>>   apache-httpd-service-type): New variables.
>>   (apache-httpd-shepherd-services, apache-httpd-activation,
>>   apache-httpd-process-extensions): New procedures.
>> * gnu/tests/web.scm (run-apache-httpd-test): New procedure.
>>   (%apache-httpd-os, %tests-apache-httpd): New variables.
>
> Nice!  Documentation will be welcome.  :-)

Great, I've sent a couple of updated patches, which now include
documentation.

>> +  #:export (<apache-httpd-configuration>
>> +            apache-httpd-configuration
>> +            apache-httpd-configuration?
>> +            apache-httpd-configuration-package
>> +            apache-httpd-configuration-pid-file
>> +            apache-httpd-configuration-config
>
> In this context I think ‘httpd-’ would be good enough as a prefix.

Ok, I've changed the prefix to 'httpd-'.

>> +(define %default-apache-httpd-modules
>> +  (map (match-lambda
>> +         ((name file)
>> +          (apache-httpd-load-module
>> +           (name name)
>> +           (file file))))
>> +       '(("authn_file_module" "modules/mod_authn_file.so")
>> +         ("authn_core_module" "modules/mod_authn_core.so")
>> +         ("authz_host_module" "modules/mod_authz_host.so")
>
> I think having this list here is the right approach.  However could you
> write where it comes from?  I guess it’s equal to the current default in
> httpd, right?
>
> How often do you expect this to be updated?

I got this from the etc/httpd/httpd.conf file in the httpd package, and
I've now added a comment in to the code saying this. I don't expect it
will need updating often.

>> +(define-record-type* <apache-httpd-config-file>
>> +  apache-httpd-config-file make-apache-httpd-config-file
>> +  apache-httpd-config-file?
>> +  (load-modules   apache-httpd-config-file-load-modules
>> +                  (default %default-apache-httpd-modules))
>
> Or “loaded-modules” or just “modules”?

I've gone with modules, and changed this field and the corresponding
record type.

>> +(define* (run-apache-httpd-test #:optional (http-port 8080))
>> +  "Run tests in %APACHE-HTTPD-OS, which has apache-httpd running and listening on
>> +HTTP-PORT."
>
> I wonder if we could abstract ‘run-nginx-test’ just enough so it can be
> used for both.  It’d need to take a service name, log file, and PID file
> as arguments.

That sounds good to me. I've now updated the first patch to generalise
the test, so that it can be used for both the nginx and httpd service.