launchpad-reviewers team mailing list archive

[Ines Almeida <mp+446073@xxxxxxxxxxxxxxxxxx>]

LGTM, only a few minor comments.
I don't feel comfortable being the sole reviewer though, so I'll leave the approval for someone else :)

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"

There seems to be a few new commits since this one, but most of our charms still use this commit.
What is the process regarding using a newer commit if it's not needed per se?

> +    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",
> +        ]

Missing a `,` here (linting is failing because of it)

> +    }
> +)
> +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(

Should this be "@when_all" instead?

> +    "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")

Should the state be "service.configured" only after configuring celery and the crontab?

> +    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")
> diff --git a/charm/launchpad-scripts-bzrsyncd/templates/launchpad-scripts-bzrsyncd-lazr.conf.j2 b/charm/launchpad-scripts-bzrsyncd/templates/launchpad-scripts-bzrsyncd-lazr.conf.j2
> new file mode 100644
> index 0000000..955c9df
> --- /dev/null
> +++ b/charm/launchpad-scripts-bzrsyncd/templates/launchpad-scripts-bzrsyncd-lazr.conf.j2
> @@ -0,0 +1,19 @@
> +# Public configuration data.  The contents of this file may be freely shared
> +# with developers if needed for debugging.
> +
> +# A schema's sections, keys, and values are automatically inherited, except
> +# for '.optional' sections.  Update this config to override key values.
> +# Values are strings, except for numbers that look like ints.  The tokens
> +# true, false, and none are treated as True, False, and None.
> +
> +{% from "macros.j2" import opt -%}

Is there a need for this line if `opt` is not used in this file?

> +
> +[meta]
> +extends: ../launchpad-db-lazr.conf
> +
> +[codehosting]
> +internal_branch_by_id_root: http://bazaar.lp.internal/
> +
> +[memcache]
> +servers: {{ memcache_servers }}
> +


-- 
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.