launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+480927@xxxxxxxxxxxxxxxxxx>]

Great job, I left two comments <3

Diff comments:

> diff --git a/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py b/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
> index 2b8b7af..af17792 100644
> --- a/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
> +++ b/charm/launchpad-ppa-publisher/reactive/launchpad-ppa-publisher.py
> @@ -64,6 +65,7 @@ def configure():
>      config["ppa_archive_private_root"] = ppa_archive_private()
>      config["ppa_signing_keys_root"] = ppa_keys_root
>      config["oval_data_root"] = oval_data_root
> +    config["excluded_ppas"] = yaml.safe_load(config["excluded_ppas"])

Maybe just me, but I would prefer the parsing logic there to have more control on that, something like :


config["excluded_ppas"] = yaml.safe_load(config["excluded_ppas"])

if config["excluded_ppas"]:
    config["excluded_ppas_options"] = " --exclude ".join(config["excluded_ppas"])
else:
    config["excluded_ppas_options"] = ""

>  
>      host.mkdir(data_dir, owner=base.user(), group=base.user(), perms=0o775)
>      host.mkdir(
> diff --git a/charm/launchpad-ppa-publisher/templates/crontab.j2 b/charm/launchpad-ppa-publisher/templates/crontab.j2
> index dbe2998..2f13a55 100644
> --- a/charm/launchpad-ppa-publisher/templates/crontab.j2
> +++ b/charm/launchpad-ppa-publisher/templates/crontab.j2
> @@ -9,7 +9,13 @@ PPAROOT={{ ppa_archive_root }}
>  P3AROOT={{ ppa_archive_private_root }}
>  
>  {% if active -%}
> -* * * * * umask 022; nice -n 5 ionice -c 2 -n 7 {{ code_dir }}/cronscripts/publishing/cron.publish-ppa "-d ubuntu" "--all-derived" "-d debian" >> {{ logs_dir }}/cron.ppa.log 2>&1
> +
> +{% if excluded_ppas %}
> +{% set excluded_ppas_options = excluded_ppas | join(' --exclude ') %} 
> +{% set excluded_ppas_options = "--exclude " ~ excluded_ppas_options %}

Just to be clear: do we want this output ?

` "--exclude --exclude PPA1 --exclude PPA2 ... " `

> +{% endif %}
> +
> +* * * * * umask 022; nice -n 5 ionice -c 2 -n 7 {{ code_dir }}/cronscripts/publishing/cron.publish-ppa "-d ubuntu {{ excluded_ppas_options }}" "--all-derived {{ excluded_ppas_options }}" "-d debian {{ excluded_ppas_options }}" >> {{ logs_dir }}/cron.ppa.log 2>&1
>  
>  17,47 * * * * nice -n 15 {{ code_dir }}/cronscripts/parse-ppa-apache-access-logs.py -q --log-file=INFO:{{ logs_dir }}/parse-ppa-apache-access-logs.log
>  


-- 
https://code.launchpad.net/~tushar5526/launchpad/+git/launchpad/+merge/480927
Your team Launchpad code reviewers is requested to review the proposed merge of ~tushar5526/launchpad:update-charm-to-exclude-ppas-from-secrets into launchpad:master.