Review: Needs Fixing
See inline-comment, and please try to take your time to self-review your code a
bit more :-)
Diff comments:
> diff --git a/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
> b/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
> index 0f1cc34..0765efc 100644
> --- a/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
> +++ b/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
> @@ -250,6 +250,25 @@ def update_lxd_dropins(arch, ip, n):
> reload()
>
>
> +def remove_old_lxd_dropins(target_lxd_config):
> + (
> + _,
> + lxd_unit_object_paths,
> + _,
> + ) = get_units()
> + all_lxd_arches = set(
> + list(lxd_unit_object_paths.keys()) + list(target_lxd_config.keys())
> + )
> + for arch in all_lxd_arches:
> + ips = target_lxd_config[arch].keys()
> + for lxd_path in lxd_unit_object_paths:
> + if "autopkgtest@" in lxd_path and [
The trailing `[` opens a new list, which is closed two lines below. That might
be remains of the previous version.
Please take your time when doing that kind of change, especially in a
complicated logic test, because if the reviewer fails to catch that, you can
basically end up with testing this: `bool([False])`, which is always True,
because the list is not empty, leading in that case to always remove all units,
and that can be really bad.
A good idea in general, is to always read the `git diff`, then `git add -p`,
then re-read the `git diff --cached` before commit. Basically review yourself,
and do that **before** publishing any of your code, to avoid hitting people
with updates that may still need amending.
In that case, I caught that because I was lacking the context, and needed to
show the diff between the two revisions of this MP, so I ran `git diff
befd787a3993..2bca011d4e4a`:
```
diff --git a/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
b/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
index 3c900f0c8d15..62d01b9606d0 100644
--- a/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
+++ b/charms/focal/autopkgtest-cloud-worker/lib/systemd.py
@@ -262,8 +262,9 @@ def remove_old_lxd_dropins(target_lxd_config):
ips = target_lxd_config[arch].keys()
for lxd_path in lxd_unit_object_paths:
if "autopkgtest@" in lxd_path and [
- ip for ip in ips if ip not in lxd_path
+ not any([ip in lxd_path for ip in ips])
]:
+ print(f"Removing {lxd_path} as it's not an intended remote.")
os.remove(lxd_path)
```
With that diff, the typo is pretty obvious, because it removes the inside of
the list comprehension **and** adds a new one inside, which sounds odd. That's
completely something I could have missed if I already had the context in mind
and didn't need that particular diff.
Btw, I really wish Launchpad could provide that in the UI.
> + not any([ip in lxd_path for ip in ips])
> + ]:
> + print(f"Removing {lxd_path} as it's not an intended remote.")
> + os.remove(lxd_path)
> +
> +
> def enable_timer(region, arch, releases):
> unit_names = [
> "build-adt-image@{}-{}-{}.timer".format(release, region, arch)
--
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/456773
Your team Canonical's Ubuntu QA is requested to review the proposed merge of
~andersson123/autopkgtest-cloud:lxd-cleanup-srv-files into
autopkgtest-cloud:master.
--
Mailing list: https://launchpad.net/~canonical-ubuntu-qa
Post to : canonical-ubuntu-qa@lists.launchpad.net
Unsubscribe : https://launchpad.net/~canonical-ubuntu-qa
More help : https://help.launchpad.net/ListHelp
