canonical-ubuntu-qa team mailing list archive

[Skia <mp+456773@xxxxxxxxxxxxxxxxxx>]

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.