launchpad-reviewers team mailing list archive

[Guruprasad <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"

Afaics, most of the new changes in the `launchpad-layers` repository have been related to turnip or other things not affecting things used by this charm. So there is no need to upgrade for the time being. Till now, Colin has upgraded the various charms to newer revisions, as needed. But if there are fixes in that repo needed by a specific charm, I don't see why we cannot test and perform the update as well.

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

Sorry for letting this slip through. For some reason, my local pre-commit setup was broken yesterday and so I relied on the same checks running here in CI. Will fix in the next revision.

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

The `when()` decorator is an alias for `when_all()` as documented at https://charmsreactive.readthedocs.io/en/latest/charms.reactive.decorators.html#charms.reactive.decorators.when. Most of our charms use the former and that is why I used it too.

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

Yes. This is a pattern that we follow in many of our charms.

> +    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 -%}

It is needed because I have to use it for the `internal_branch_by_id_root` parameter that I missed doing. Will address this in the next revision.

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