launchpad-reviewers team mailing list archive

[Ines Almeida <mp+453215@xxxxxxxxxxxxxxxxxx>]

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.