launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32573
Re: [Merge] ~alvarocs/launchpad-buildd:fetch-service-charm-builds into launchpad-buildd:master
comments addressed, thanks for the review!
Diff comments:
> diff --git a/lpbuildd/charm.py b/lpbuildd/charm.py
> index e79aacb..6101d96 100644
> --- a/lpbuildd/charm.py
> +++ b/lpbuildd/charm.py
> @@ -58,6 +60,14 @@ class CharmBuildManager(BuildManagerProxyMixin, DebianBuildManager):
> args.extend(["--build-path", self.build_path])
> if self.craft_platform:
> args.extend(["--craft-platform", self.craft_platform])
> + if self.use_fetch_service:
> + args.append("--use-fetch-service")
No reason, I would also include it inside the extend block but for other crafts it's done like this (append and then extend)
> + args.extend(
>From the snap script in https://git.launchpad.net/launchpad-buildd/tree/lpbuildd/snap.py#n111 it seems we don't need additional checks as if the certificate is not present the fetch service won't work anyway
> + [
> + "--fetch-service-mitm-certificate",
> + self.secrets["fetch_service_mitm_certificate"],
> + ]
> + )
> args.append(self.name)
> self.runTargetSubProcess("build-charm", *args)
>
> diff --git a/lpbuildd/target/tests/test_build_charm.py b/lpbuildd/target/tests/test_build_charm.py
> index 6c3c04a..42e3154 100644
> --- a/lpbuildd/target/tests/test_build_charm.py
> +++ b/lpbuildd/target/tests/test_build_charm.py
> @@ -265,6 +265,267 @@ class TestBuildCharm(TestCase):
> ],
> )
>
> + def test_install_certificate(self):
> + args = [
> + "build-charm",
> + "--backend=fake",
> + "--series=xenial",
> + "--arch=amd64",
> + "1",
> + "--git-repository",
> + "lp:foo",
> + "--proxy-url",
> + "http://proxy.example:3128/";,
> + "test-image",
> + "--use-fetch-service",
> + "--fetch-service-mitm-certificate",
> + # Base64 content_of_cert
> + "Y29udGVudF9vZl9jZXJ0",
> + ]
> + build_charm = parse_args(args=args).operation
> + build_charm.bin = "/builderbin"
> + self.useFixture(FakeFilesystem()).add("/builderbin")
> + os.mkdir("/builderbin")
> + with open("/builderbin/lpbuildd-git-proxy", "w") as proxy_script:
> + proxy_script.write("proxy script\n")
> + os.fchmod(proxy_script.fileno(), 0o755)
> + build_charm.install()
> + self.assertThat(
> + build_charm.backend.run.calls,
> + MatchesListwise(
> + [
> + RanAptGet(
> + "install",
> + "python3",
> + "socat",
> + "git",
> + "python3-pip",
> + "python3-setuptools",
> + ),
> + RanSnap("install", "--classic", "charmcraft"),
> + RanCommand(["rm", "-rf", "/var/lib/apt/lists"]),
> + RanCommand(["update-ca-certificates"]),
> + RanCommand(
> + [
> + "snap",
> + "set",
> + "system",
> + "proxy.http=http://proxy.example:3128/";,
> + ]
> + ),
> + RanCommand(
> + [
> + "snap",
> + "set",
> + "system",
> + "proxy.https=http://proxy.example:3128/";,
> + ]
> + ),
> + RanAptGet("update"),
> + RanCommand(
> + [
> + "systemctl",
> + "restart",
> + "snapd",
> + ]
> + ),
> + RanCommand(["mkdir", "-p", "/home/buildd"]),
> + ]
> + ),
> + )
> + self.assertEqual(
> + (b"proxy script\n", stat.S_IFREG | 0o755),
> + build_charm.backend.backend_fs["/usr/local/bin/lpbuildd-git-proxy"],
> + )
> + self.assertEqual(
> + (
> + b"content_of_cert",
> + stat.S_IFREG | 0o644,
> + ),
> + build_charm.backend.backend_fs[
> + "/usr/local/share/ca-certificates/local-ca.crt"
> + ],
> + )
> + self.assertEqual(
> + (
> + dedent(
> + """\
> + Acquire::http::Proxy "http://proxy.example:3128/";;
> + Acquire::https::Proxy "http://proxy.example:3128/";;
> +
> + """
> + ).encode("UTF-8"),
> + stat.S_IFREG | 0o644,
> + ),
> + build_charm.backend.backend_fs["/etc/apt/apt.conf.d/99proxy"],
> + )
> +
> + def test_install_snapd_proxy(self):
> + args = [
> + "build-charm",
> + "--backend=fake",
> + "--series=xenial",
> + "--arch=amd64",
> + "1",
> + "--git-repository",
> + "lp:foo",
> + "--proxy-url",
> + "http://proxy.example:3128/";,
> + "test-image",
> + "--use-fetch-service",
> + "--fetch-service-mitm-certificate",
> + # Base64 content_of_cert
> + "Y29udGVudF9vZl9jZXJ0",
> + ]
> + build_charm = parse_args(args=args).operation
> + build_charm.bin = "/builderbin"
> + self.useFixture(FakeFilesystem()).add("/builderbin")
> + os.mkdir("/builderbin")
> + with open("/builderbin/lpbuildd-git-proxy", "w") as proxy_script:
> + proxy_script.write("proxy script\n")
> + os.fchmod(proxy_script.fileno(), 0o755)
> + build_charm.install()
> + self.assertThat(
> + build_charm.backend.run.calls,
> + MatchesListwise(
> + [
> + RanAptGet(
> + "install",
> + "python3",
> + "socat",
> + "git",
> + "python3-pip",
> + "python3-setuptools",
> + ),
> + RanSnap("install", "--classic", "charmcraft"),
> + RanCommand(["rm", "-rf", "/var/lib/apt/lists"]),
> + RanCommand(["update-ca-certificates"]),
> + RanCommand(
> + [
> + "snap",
> + "set",
> + "system",
> + "proxy.http=http://proxy.example:3128/";,
> + ]
> + ),
> + RanCommand(
> + [
> + "snap",
> + "set",
> + "system",
> + "proxy.https=http://proxy.example:3128/";,
> + ]
> + ),
> + RanAptGet("update"),
> + RanCommand(
> + [
> + "systemctl",
> + "restart",
> + "snapd",
> + ]
> + ),
> + RanCommand(["mkdir", "-p", "/home/buildd"]),
> + ]
> + ),
> + )
> + self.assertEqual(
> + (b"proxy script\n", stat.S_IFREG | 0o755),
> + build_charm.backend.backend_fs["/usr/local/bin/lpbuildd-git-proxy"],
> + )
> + self.assertEqual(
> + (
> + dedent(
> + """\
> + Acquire::http::Proxy "http://proxy.example:3128/";;
> + Acquire::https::Proxy "http://proxy.example:3128/";;
> +
> + """
> + ).encode("UTF-8"),
> + stat.S_IFREG | 0o644,
> + ),
> + build_charm.backend.backend_fs["/etc/apt/apt.conf.d/99proxy"],
> + )
> +
> + def test_install_fetch_service(self):
> + args = [
> + "build-charm",
> + "--backend=fake",
> + "--series=xenial",
> + "--arch=amd64",
> + "1",
> + "--git-repository",
> + "lp:foo",
> + "--proxy-url",
> + "http://proxy.example:3128/";,
> + "test-image",
> + "--use-fetch-service",
> + "--fetch-service-mitm-certificate",
> + # Base64 content_of_cert
> + "Y29udGVudF9vZl9jZXJ0",
> + ]
> + build_charm = parse_args(args=args).operation
> + build_charm.bin = "/builderbin"
> + self.useFixture(FakeFilesystem()).add("/builderbin")
> + os.mkdir("/builderbin")
> + with open("/builderbin/lpbuildd-git-proxy", "w") as proxy_script:
> + proxy_script.write("proxy script\n")
> + os.fchmod(proxy_script.fileno(), 0o755)
> + build_charm.install()
> + self.assertThat(
> + build_charm.backend.run.calls,
> + MatchesAll(
> + Not(
> + AnyMatch(
> + RanCommand(
> + [
> + "git",
This test simply checks that command is not set when use fetch service is enabled on any series but focal. Then, there is a test below for Focal in which we enforce git v2 protocol. We could add more asserts in here but those are already in the test_install_certificate() and test_install_snapd_proxy() tests, so it will be redundant. I kept it like this to be consistent with the tests of the other crafts. The difference between focal and the other series comes from this https://git.launchpad.net/launchpad-buildd/tree/lpbuildd/target/proxy.py#n143 which seems to enforce git v2 protocol for focal (I guess because v1 or v0 is the default), opposite to the other serie for which v2 is the default (not sure why this difference though).
> + "config",
> + "--global",
> + "protocol.version",
> + "2",
> + ]
> + )
> + )
> + ),
> + ),
> + )
> +
> + def test_install_fetch_service_focal(self):
> + args = [
> + "build-charm",
> + "--backend=fake",
> + "--series=focal",
> + "--arch=amd64",
> + "1",
> + "--git-repository",
> + "lp:foo",
> + "--proxy-url",
> + "http://proxy.example:3128/";,
> + "test-image",
> + "--use-fetch-service",
> + "--fetch-service-mitm-certificate",
> + # Base64 content_of_cert
> + "Y29udGVudF9vZl9jZXJ0",
> + ]
> + build_charm = parse_args(args=args).operation
> + build_charm.bin = "/builderbin"
> + self.useFixture(FakeFilesystem()).add("/builderbin")
> + os.mkdir("/builderbin")
> + with open("/builderbin/lpbuildd-git-proxy", "w") as proxy_script:
> + proxy_script.write("proxy script\n")
> + os.fchmod(proxy_script.fileno(), 0o755)
> + build_charm.install()
> + self.assertThat(
> + build_charm.backend.run.calls,
> + MatchesAll(
> + AnyMatch(
> + RanCommand(
answered on the previous comment
> + ["git", "config", "--global", "protocol.version", "2"]
> + )
> + ),
> + ),
> + )
> +
> def test_repo_bzr(self):
> args = [
> "build-charm",
> diff --git a/lpbuildd/tests/test_charm.py b/lpbuildd/tests/test_charm.py
> index b8a5183..3944ab4 100644
> --- a/lpbuildd/tests/test_charm.py
> +++ b/lpbuildd/tests/test_charm.py
> @@ -243,6 +243,21 @@ class TestCharmBuildManagerIteration(TestCase):
> self.buildmanager.iterate, self.buildmanager.iterators[-1]
> )
> self.assertFalse(self.builder.wasCalled("buildFail"))
> +
> + @defer.inlineCallbacks
> + def test_iterate_use_fetch_service(self):
> + # The build manager can be told to use the fetch service as its proxy.
> + # This requires also a ca certificate passed in via secrets.
> + args = {
> + "use_fetch_service": True,
> + "secrets": {"fetch_service_mitm_certificate": "content_of_cert"},
> + }
> + expected_options = [
> + "--use-fetch-service",
> + "--fetch-service-mitm-certificate",
> + "content_of_cert",
> + ]
> + yield self.startBuild(args, expected_options)
The objective of this test is to verify that the extra args for the fetch service that we have added are converted into the CLI flags for the builder to execute. The asserts are in the startBuild() method.
>
> @defer.inlineCallbacks
> def test_iterate_craft_platform(self):
--
https://code.launchpad.net/~alvarocs/launchpad-buildd/+git/launchpad-buildd/+merge/486058
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/launchpad-buildd:fetch-service-charm-builds into launchpad-buildd:master.
References
