launchpad-reviewers team mailing list archive

[Guruprasad <mp+494587@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lpbuildd/target/run_ci.py b/lpbuildd/target/run_ci.py
> index 54aca19..fde95b7 100644
> --- a/lpbuildd/target/run_ci.py
> +++ b/lpbuildd/target/run_ci.py
> @@ -65,6 +66,16 @@ class RunCIPrepare(
>          self.backend.run(["apt-get", "-y", "install"] + deps)
>          if self.backend.supports_snapd:
>              self.snap_store_set_proxy()
> +        result = subprocess.run(

Can we add a test for this workaround? While this might be a temporary or permanent workaround, it is easier to make sense of such changes if we have accompanying tests.

> +            ["sudo", "service", "apparmor", "restart"],

I think `sudo` is unnecessary because this code runs as `root`. You can see evidence of this a few lines above where we are just invoking `apt-get install` without `sudo`.

> +            capture_output=True,
> +            text=True,
> +            check=False

Why are choosing to not care about whether this command succeeded or failed?

> +        )
> +        print("Command output:")
> +        print(result.stdout)
> +        print("Command errors (if any):")
> +        print(result.stderr)

Do we need the print statements here? If yes, can you convert them to logging statements instead?

>          for snap_name, channel in sorted(self.args.channels.items()):
>              if snap_name not in ("lxd", "lpci"):
>                  self.backend.run(


-- 
https://code.launchpad.net/~clinton-fung/launchpad-buildd/+git/launchpad-buildd/+merge/494587
Your team Launchpad code reviewers is requested to review the proposed merge of ~clinton-fung/launchpad-buildd:restart-apparmor-after-upgrades into launchpad-buildd:master.