launchpad-reviewers team mailing list archive

[Ines Almeida <mp+447663@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/charm/launchpad-ppa-publisher/templates/crontab.j2 b/charm/launchpad-ppa-publisher/templates/crontab.j2
> new file mode 100644
> index 0000000..cc69fb6
> --- /dev/null
> +++ b/charm/launchpad-ppa-publisher/templates/crontab.j2
> @@ -0,0 +1,17 @@
> +# m h  dom mon dow   command
> +MAILTO={{ cron_mailto }}
> +LPCONFIG=launchpad-ppa-publisher
> +
> +{% 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
> +
> +59 05  * * 0 {{ code_dir }}/cronscripts/publishing/cron.daily-ppa >> {{ logs_dir }}/cron.ppa.log 2>&1
> +0 */6 * * * nice -n 17 {{ code_dir }}/scripts/process-death-row.py -d ubuntu --ppa -q --log-file=INFO:{{ logs_dir }}/process-death-row.log
> +0 * * * * nice -n 17 {{ code_dir }}/scripts/process-death-row.py --all-derived --ppa -q --log-file=INFO:{{ logs_dir }}/derived-process-death-row.log
> +*/20 * * * * nice -n 12 ionice -c 2 -n 7 {{ code_dir }}/cronscripts/ppa-generate-keys.py -q --log-file=INFO:{{ logs_dir }}/ppa-generate-keys.log
> +* * * * * {{ code_dir }}/cronscripts/generate-ppa-htaccess.py >> {{ logs_dir }}/generate-ppa-htaccess-wrapper.log 2>&1

Note here, the original lines pointed at a `generate-ppa-htaccess-wrapper.sh` which I haven't been able to find anywhere. Was this a fix of some sorts added to production directly?

> +
> +# OOPS amqp
> +*/15 * * * * {{ code_dir }}/bin/datedir2amqp --exchange oopses --host {{ rabbitmq_host }} --username {{ rabbitmq_username }} --password {{ rabbitmq_password }} --vhost {{ rabbitmq_vhost }} --repo {{ oopses_dir }} --key ""
> +{% endif %}
> +
> diff --git a/charm/launchpad-ppa-uploader/reactive/launchpad-ppa-uploader.py b/charm/launchpad-ppa-uploader/reactive/launchpad-ppa-uploader.py
> new file mode 100644
> index 0000000..50c9c8a
> --- /dev/null
> +++ b/charm/launchpad-ppa-uploader/reactive/launchpad-ppa-uploader.py
> @@ -0,0 +1,115 @@
> +# Copyright 2023 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import os
> +
> +from charmhelpers.core import hookenv, host, templating
> +from charms.launchpad.base import configure_email, get_service_config
> +from charms.launchpad.payload import configure_cron, configure_lazr
> +from charms.reactive import (
> +    clear_flag,
> +    endpoint_from_flag,
> +    hook,
> +    remove_state,
> +    set_flag,
> +    when,
> +    when_not,
> +    when_not_all,
> +)
> +from ols import base
> +
> +
> +def configure_logrotate(config):
> +    hookenv.log("Writing logrotate configuration.")
> +    templating.render(
> +        "logrotate.conf.j2",
> +        "/etc/logrotate.d/launchpad-ppa-uploader",
> +        config,
> +        perms=0o644,
> +    )
> +
> +
> +@when(
> +    "launchpad.db.configured",
> +    "memcache.available",
> +)
> +@when_not("service.configured")
> +def configure():
> +    hookenv.log("Configuring ppa uploader")
> +    config = get_service_config()
> +    config["ppa_archive_root"] = os.path.join(base.base_dir(), "ppa-archive")
> +    config["ppa_archive_private_root"] = os.path.join(
> +        base.base_dir(), "private-ppa-archive"
> +    )
> +    config["ppa_keys_root"] = os.path.join(base.base_dir(), "ppa-signing-keys")
> +
> +    # Create PPA file directories
> +    ppa_queue_dir = os.path.join(base.base_dir(), "ppa-queue")
> +    host.mkdir(
> +        ppa_queue_dir, owner=base.user(), group=base.user(), perms=0o775
> +    )
> +    config["ppa_queue_dir"] = ppa_queue_dir
> +    for folder in ["accepted", "failed", "rejected"]:
> +        dir = os.path.join(ppa_queue_dir, folder)
> +        host.mkdir(dir, owner=base.user(), group=base.user(), perms=0o755)
> +
> +    hookenv.log("Setting up memcache for ppa uploader")
> +    memcache = endpoint_from_flag("memcache.available")
> +    config["memcache_servers"] = ",".join(
> +        sorted(
> +            f"({host}:{port},1)"
> +            for host, port in memcache.memcache_hosts_ports()
> +        )
> +    )
> +    configure_lazr(
> +        config,
> +        "launchpad-ppa-uploader-lazr.conf.j2",
> +        "launchpad-ppa-uploader/launchpad-lazr.conf",
> +    )
> +    configure_lazr(
> +        config,
> +        "launchpad-ppa-auth-lazr.conf.j2",
> +        "launchpad-ppa-auth/launchpad-lazr.conf",
> +        secret=True,
> +    )
> +    configure_email(config, "launchpad-ppa-uploader")
> +    configure_logrotate(config)
> +    configure_cron(config, "crontab.j2")
> +    set_flag("service.configured")
> +
> +
> +@when("service.configured", "upload-queue-processor.available")
> +@when_not("service.txpkgupload-configured")
> +def configure_txpkgupload():
> +    hookenv.log("Configuring txpkgupload")
> +
> +    fsroot = os.path.join(base.base_dir(), "ppa-queue", "incoming")
> +    host.mkdir(fsroot, owner=base.user(), group=base.user(), perms=0o777)

For some reason, setting this to 775 and adding the `txpkgupload` use to `launchpad` didn't seem to be enough for it to have permission to move files to `fsroot`. I made sure that the users (launchpad and txpkgupload) were in each other's groups, and that group permissions were `rwx` and still got permission errors until I set the permission to `777`. Perhaps there is another user that I haven't considered?

> +    txpkgupload = endpoint_from_flag("upload-queue-processor.available")
> +    txpkgupload.set_config(fsroot=fsroot)
> +    host.add_user_to_group(base.user(), "txpkgupload")
> +    host.add_user_to_group("txpkgupload", base.user())
> +    set_flag("service.txpkgupload-configured")
> +
> +
> +@when("service.txpkgupload-configured")
> +@when_not_all("service.configured", "upload-queue-processor.available")
> +def txpkgupload_deconfigured():
> +    hookenv.status_set("blocked", "Txpkgupload not yet configured")
> +    clear_flag("service.txpkgupload-configured")
> +
> +
> +@when("service.configured", "service.txpkgupload-configured")
> +def check_is_running():
> +    hookenv.status_set("active", "Ready")
> +
> +
> +@when("service.configured")
> +@when_not_all("launchpad.db.configured", "memcache.available")
> +def deconfigure():
> +    remove_state("service.configured")
> +
> +
> +@hook("{requires:memcache}-relation-{joined,changed,broken,departed}")
> +def memcache_relation_changed(memcache):
> +    remove_state("service.configured")
> diff --git a/cronscripts/publishing/cron.base-ppa.sh b/cronscripts/publishing/cron.base-ppa.sh
> index e663fac..a772d39 100644
> --- a/cronscripts/publishing/cron.base-ppa.sh
> +++ b/cronscripts/publishing/cron.base-ppa.sh
> @@ -5,10 +5,17 @@
>  # Initial setup for PPA cronscripts.
>  
>  # DO NOT set LPCONFIG here, it should come from the crontab or the shell.
> +if [ -d "/srv/launchpad.net" ] 

Thoughts here?

I am considering improving this so that BASE_DIR would be an arg that has value `/srv/launchpad.net` by default. But this isn't being called by the cronscript, it's being `source`d by other scripts, so it would add extra baggage. Feels like perhaps that's not a battle to battle here

> +then
> +  BASE_DIR=/srv/launchpad.net
> +else
> +  BASE_DIR=/srv/launchpad
> +fi
> +
>  # Define common variables (also used by cron.daily-ppa).
> -PPAROOT=/srv/launchpad.net/ppa-archive
> +PPAROOT=$BASE_DIR/ppa-archive
>  # shellcheck disable=SC2034  # not used here, but used by cron.daily-ppa
> -P3AROOT=/srv/launchpad.net/private-ppa-archive
> +P3AROOT=$BASE_DIR/private-ppa-archive
>  LOCKFILE=$PPAROOT/.lock
>  # Default lockfile options, retry once if it's locked.
>  if [ "$LOCKFILEOPTIONS" == "" ]; then


-- 
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/447663
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:add-ppa-publisher-charm into launchpad:master.