Mostly LGTM, left a few comments that you can address and then I can approve.
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")
Why not make this part of the extend below?
> + args.extend(
Shouldn't we check something like
if fetch-service-mitm-certificate: then add related args
> + [
> + "--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",
Is the assert on git command enough to test fetch_service installation?
> + "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(
Here as well, is the git config check enough?
> + ["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)
hmm, no asserts here ?
Can you briefly explain this test?
>
> @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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
