launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

It sounds like the direction we're going is indeed going to be to provide external access for the whole duration of the build, but could you please get in touch with Martin Albisetti to confirm that that's definitely what we have to do rather than separating out the pull phase and providing external access only to that as we'd previously intended?

Diff comments:

> === modified file 'lpbuildd/snap.py'
> --- lpbuildd/snap.py	2015-07-31 11:54:07 +0000
> +++ lpbuildd/snap.py	2015-10-04 22:12:07 +0000
> @@ -52,6 +56,15 @@
>              "--build-id", self._buildid,
>              "--arch", self.arch_tag,
>              ]
> +        if self.proxy_token:

Maybe worth checking here that all the necessary arguments were passed, just in case.

> +            proxy_env = (
> +                "http_proxy=http://{username}:{password}";
> +                "@{host}:{port}".format(
> +                    username=self.build_cookie,
> +                    password=self.proxy_token,
> +                    host=self.proxy_host,
> +                    port=self.proxy_port))
> +            args.insert(0, proxy_env)

Rather than relying on shell parsing of environment variable prefixes to the command, could you please instead add an env=None keyword argument to runSubProcess, defaulting to os.environ if None, so that you can then do something like:

  env = dict(os.environ)
  if condition:
      env["http_proxy"] = blah
  ...
  self.runSubProcess(self.build_snap_path, args, env=env)

>          if self.branch is not None:
>              args.extend(["--branch", self.branch])
>          if self.git_repository is not None:


-- 
https://code.launchpad.net/~blr/launchpad-buildd/snap_http_proxy/+merge/273355
Your team Launchpad code reviewers is subscribed to branch lp:launchpad-buildd.