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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp