launchpad-reviewers team mailing list archive

[Andy Whitcroft <mp+495065@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
> index 5d957ae..0c6c2e5 100644
> --- a/lpbuildd/target/build_snap.py
> +++ b/lpbuildd/target/build_snap.py
> @@ -150,6 +151,13 @@ class BuildSnap(
>          self.backend.run(["apt-get", "-y", "install"] + deps)
>          if self.backend.supports_snapd:
>              self.snap_store_set_proxy()
> +        result = subprocess.run(
> +            ["sudo", "service", "apparmor", "restart"],
> +            capture_output=True,
> +            check=True

You are asking this to raise an exception if the payload exits badly, the time it would normally emit stderr.  Should you be capturing that and at least emitting stdout/stderr before re-raising?

Or more likely not asking for check, and checking yourself after the two logger.info commands?

> +        )
> +        logger.info("Command output: %s" % result.stdout)
> +        logger.info("Command errors (if any): %s" % result.stderr)
>          for snap_name, channel in sorted(self.args.channels.items()):
>              # snapcraft is handled separately, since it requires --classic.
>              if snap_name != "snapcraft":


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