Quick review before the stand-up :)

Diff comments:

> diff --git a/charm/launchpad-copy-archive-publisher/layer.yaml 
> b/charm/launchpad-copy-archive-publisher/layer.yaml
> index c314294..da8e726 100644
> --- a/charm/launchpad-copy-archive-publisher/layer.yaml
> +++ b/charm/launchpad-copy-archive-publisher/layer.yaml
> @@ -1,12 +1,14 @@
>  includes:
>    - layer:launchpad-db
>    - layer:launchpad-publisher-parts
> +  - interface:apache-website
>  repo: https://git.launchpad.net/launchpad
>  options:
>    apt:
>      packages:
>        - launchpad-soyuz-dependencies
>        - procmail # Needed for the 'lockfile' command.
> +      - libapache2-mod-wsgi-py3

I'm not 100% if this is needed. The PPA publisher used it because it required 
some wsgi auth config

>    ols-pg:
>      databases:
>        db:
> diff --git 
> a/charm/launchpad-copy-archive-publisher/templates/derived.vhost.conf.j2 
> b/charm/launchpad-copy-archive-publisher/templates/derived.vhost.conf.j2
> new file mode 100644
> index 0000000..cbd1539
> --- /dev/null
> +++ b/charm/launchpad-copy-archive-publisher/templates/derived.vhost.conf.j2
> @@ -0,0 +1,15 @@
> +<VirtualHost *:80>
> +     ServerName derived.archive.canonical.com

I'd expect this to come from a variable so that it can have a different value 
in each environment. As a variable it could also be used for the `CustomLog` 
and `ErrorLog` variables below

> +
> +     CustomLog /var/log/apache2/derived.archive.canonical.com-access.log 
> combined
> +     ErrorLog /var/log/apache2/derived.archive.canonical.com-error.log
> +
> +     DocumentRoot /srv/launchpad.net/archives/

I think this should be `{{ data_dir }}/archives/` or create a separate `{{ 
archives_dir }} in the reactive code, since it's also used below

> +
> +     <Directory /srv/launchpad.net/archives>
> +             Options Indexes FollowSymLinks
> +             AllowOverride None
> +             Require all granted
> +     </Directory>
> +
> +</VirtualHost>
> diff --git 
> a/charm/launchpad-copy-archive-publisher/templates/rebuild-test.vhost.conf.j2 
> b/charm/launchpad-copy-archive-publisher/templates/rebuild-test.vhost.conf.j2
> new file mode 100644
> index 0000000..d4ef0a8
> --- /dev/null
> +++ 
> b/charm/launchpad-copy-archive-publisher/templates/rebuild-test.vhost.conf.j2
> @@ -0,0 +1,19 @@
> +<VirtualHost *:80>
> +        #NOWEBSTATS
> +        ServerName rebuild-test.internal

This file doesn't seem used? Should be also configures in the reactive code?

If it's supposed to be used, then same as above regarding having these as 
variables in this file

> +        ServerAlias toolchain.ubuntu.com
> +        ServerAlias rebuild-test.ubuntu.com
> +
> +        CustomLog /var/log/apache2/rebuild-test.internal.access.log combined
> +        ErrorLog /var/log/apache2/rebuild-test.internal.error.log
> +
> +        DocumentRoot /srv/rebuild-test.internal/ftp
> +        Alias /ubuntu "/srv/rebuild-test.internal/ftp"
> +
> +        <Directory "/srv/rebuild-test.internal/">
> +          IndexOptions NameWidth=* +SuppressDescription
> +          Options +Indexes +FollowSymLinks
> +          IndexIgnore favicon.ico
> +          Require all granted
> +        </Directory>
> +</VirtualHost>


-- 
https://code.launchpad.net/~jugmac00/launchpad/+git/launchpad/+merge/453215
Your team Launchpad code reviewers is requested to review the proposed merge of 
~jugmac00/launchpad:add-apache-config-to-copy-archive-publisher into 
launchpad:master.


_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help   : https://help.launchpad.net/ListHelp

Reply via email to