launchpad-reviewers team mailing list archive

[Colin Watson <mp+446073@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/charm/launchpad-scripts-bzrsyncd/charmcraft.yaml b/charm/launchpad-scripts-bzrsyncd/charmcraft.yaml
> new file mode 100644
> index 0000000..9b28632
> --- /dev/null
> +++ b/charm/launchpad-scripts-bzrsyncd/charmcraft.yaml
> @@ -0,0 +1,63 @@
> +type: charm
> +bases:
> +  - build-on:
> +    - name: ubuntu
> +      channel: "20.04"
> +      architectures: [amd64]
> +    run-on:
> +    - name: ubuntu
> +      channel: "20.04"
> +      architectures: [amd64]
> +parts:
> +  charm-wheels:
> +    source: https://git.launchpad.net/~ubuntuone-hackers/ols-charm-deps/+git/wheels
> +    source-commit: "42c89d9c66dbe137139b047fd54aed49b66d1a5e"
> +    source-submodules: []
> +    source-type: git
> +    plugin: dump
> +    organize:
> +      "*": charm-wheels/
> +    prime:
> +      - "-charm-wheels"
> +  ols-layers:
> +    source: https://git.launchpad.net/ols-charm-deps
> +    source-commit: "f63ae0386275bf9089b30c8abae252a0ea523633"
> +    source-submodules: []
> +    source-type: git
> +    plugin: dump
> +    organize:
> +      "*": layers/
> +    stage:
> +      - layers
> +    prime:
> +      - "-layers"
> +  launchpad-layers:
> +    after:
> +      - ols-layers
> +    source: https://git.launchpad.net/launchpad-layers
> +    source-commit: "42a4b4c4f62936b1d050c775e84f7364dfb5efc0"

I don't want to be _too_ prescriptive about this, but my general advice is that it's going to be easier to understand what's going on if all the charms in a given repository use the same revision for common layers.  To that end, I'd prefer that upgrading layer revisions be done in a separate commit from other changes (except for those needed as a result of upgrading layers) and that it be done across all charms in a repository at the same time.

> +    source-submodules: []
> +    source-type: git
> +    plugin: dump
> +    organize:
> +      launchpad-base: layers/layer/launchpad-base
> +      launchpad-db: layers/layer/launchpad-db
> +      launchpad-payload: layers/layer/launchpad-payload
> +    stage:
> +      - layers
> +    prime:
> +      - "-layers"
> +  launchpad-scripts-bzrsyncd:
> +    after:
> +      - charm-wheels
> +      - launchpad-layers
> +    source: .
> +    plugin: reactive
> +    build-snaps: [charm]
> +    build-packages: [libpq-dev, python3-dev]
> +    build-environment:
> +      - CHARM_LAYERS_DIR: $CRAFT_STAGE/layers/layer
> +      - CHARM_INTERFACES_DIR: $CRAFT_STAGE/layers/interface
> +      - PIP_NO_INDEX: "true"
> +      - PIP_FIND_LINKS: $CRAFT_STAGE/charm-wheels
> +    reactive-charm-build-arguments: [--binary-wheels-from-source]
> diff --git a/charm/launchpad-scripts-bzrsyncd/reactive/launchpad-scripts-bzrsyncd.py b/charm/launchpad-scripts-bzrsyncd/reactive/launchpad-scripts-bzrsyncd.py
> new file mode 100644
> index 0000000..6e85f10
> --- /dev/null
> +++ b/charm/launchpad-scripts-bzrsyncd/reactive/launchpad-scripts-bzrsyncd.py
> @@ -0,0 +1,104 @@
> +# Copyright 2023 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import subprocess
> +
> +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 (
> +    endpoint_from_flag,
> +    remove_state,
> +    set_state,
> +    when,
> +    when_not,
> +    when_not_all,
> +)
> +
> +
> +def configure_logrotate(config):
> +    hookenv.log("Writing logrotate configuration.")
> +    templating.render(
> +        "logrotate.conf.j2",
> +        "/etc/logrotate.d/launchpad",
> +        config,
> +        perms=0o644,
> +    )
> +
> +
> +@host.restart_on_change(
> +    {
> +        "/lib/systemd/system/celerybeat-bzrsyncd.service": [
> +            "celerybeat-bzrsyncd",
> +        ],
> +        "/lib/systemd/system/celeryd_bzrsyncd_job.service": [
> +            "celeryd_bzrsyncd_job",
> +        ],
> +        "/lib/systemd/system/celeryd_bzrsyncd_job_slow.service": [
> +            "celeryd_bzrsyncd_job_slow",
> +        ]
> +    }
> +)
> +def configure_celery(config):
> +    hookenv.log("Writing celery systemd service files.")
> +    destination_dir = "/lib/systemd/system"
> +    service_files = (
> +        "celerybeat_bzrsyncd.service",
> +        "celeryd_bzrsyncd_job.service",
> +        "celeryd_bzrsyncd_job_slow.service",
> +    )
> +    for service_file in service_files:
> +        templating.render(
> +            f"{service_file}.j2",
> +            f"{destination_dir}/{service_file}",
> +            config,
> +        )
> +    subprocess.check_call(["systemctl", "daemon-reload"])
> +    for service_file in service_files:
> +        subprocess.check_call(["systemctl", "enable", service_file])
> +
> +
> +@when(
> +    "launchpad.db.configured",
> +    "memcache.available",
> +)
> +@when_not("service.configured")
> +def configure():
> +    config = get_service_config()
> +    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-scripts-bzrsyncd-lazr.conf.j2",
> +        "launchpad-scripts-bzrsyncd/launchpad-lazr.conf",
> +    )
> +    configure_lazr(
> +        config,
> +        "launchpad-scripts-bzrsyncd-secrets-lazr.conf.j2",
> +        "launchpad-scripts-bzrsyncd-secrets-lazr.conf",
> +        secret=True,
> +    )
> +    configure_email(config, "launchpad-scripts")
> +    configure_logrotate(config)
> +    set_state("service.configured")

Strictly speaking it doesn't matter because changes to flags are reset when a handler crashes, so this is effectively transactional - the flag change will only be persisted to future handlers if the rest of this handler succeeds.  But I generally think it's less confusing to put flag changes at the end of the handler, so this line should move to after `configure_cron` and `configure_celery`.

(Guruprasad, you seem to have acknowledged Inês's point but then not made the change.  Was that an oversight or has there been a misunderstanding here?)

> +    configure_cron(config, "crontab.j2")
> +    configure_celery(config)
> +
> +
> +@when("service.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")


-- 
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/446073
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:charm-launchpad-scripts-bzrsyncd into launchpad:master.