launchpad-reviewers team mailing list archive

[Yuliy Schwartzburg <mp+473839@xxxxxxxxxxxxxxxxxx>]

Yuliy Schwartzburg has proposed merging ~ikoruk/launchpad-buildd:git-protocol-v2 into launchpad-buildd:master.

Commit message:
Fix setting git protocol v2 for focal

The previous approach with env variables didn't work, this reverts that and uses `git config --global protocol.version 2`


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ikoruk/launchpad-buildd/+git/launchpad-buildd/+merge/473839
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ikoruk/launchpad-buildd:git-protocol-v2 into launchpad-buildd:master.

diff --git a/lpbuildd/target/build_snap.py b/lpbuildd/target/build_snap.py
index cfdf5ad..4e8962f 100644
--- a/lpbuildd/target/build_snap.py
+++ b/lpbuildd/target/build_snap.py
@@ -171,6 +171,7 @@ class BuildSnap(
             self.install_snapd_proxy(proxy_url=self.args.proxy_url)
             self.backend.run(["apt-get", "-y", "update"])
             self.restart_snapd()
+            self.configure_git_protocol_v2()
 
     def repo(self):
         """Collect git or bzr branch."""
diff --git a/lpbuildd/target/proxy.py b/lpbuildd/target/proxy.py
index 94de1c4..1201566 100644
--- a/lpbuildd/target/proxy.py
+++ b/lpbuildd/target/proxy.py
@@ -106,15 +106,11 @@ class BuilderProxyOperationMixin:
             # Avoid needing to keep track of snap store CDNs in proxy
             # configuration.
             full_env["SNAPPY_STORE_NO_CDN"] = "1"
-        if use_fetch_service:
-            # Remove this when fetch service supports git protocol v1
-            # This enables git protocol v2 for focal, versions of ubuntu
-            # before focal do not support protocol v2
-            full_env["GIT_PROTOCOL"] = "version=2"
         # Avoid circular import using __class__.__name__
         if use_fetch_service and self.__class__.__name__ == "BuildRock":
             full_env["CARGO_HTTP_CAINFO"] = self.mitm_certificate_path
             full_env["GOPROXY"] = "direct"
+
         return full_env
 
     def restart_snapd(self):
@@ -122,6 +118,13 @@ class BuilderProxyOperationMixin:
         self.backend.run(["systemctl", "restart", "snapd"])
 
     def delete_apt_cache(self):
+
         self.backend.run(
             ["rm", "-rf", "/var/lib/apt/lists/*"]
         )
+
+    def configure_git_protocol_v2(self):
+        if self.backend.series == "focal":
+            self.backend.run(
+                ["git", "config", "--global", "protocol.version", "2"]
+            )
diff --git a/lpbuildd/target/tests/test_build_rock.py b/lpbuildd/target/tests/test_build_rock.py
index 53aac1d..f4cc3ed 100644
--- a/lpbuildd/target/tests/test_build_rock.py
+++ b/lpbuildd/target/tests/test_build_rock.py
@@ -7,7 +7,7 @@ from textwrap import dedent
 import responses
 from fixtures import FakeLogger, TempDir
 from systemfixtures import FakeFilesystem
-from testtools.matchers import AnyMatch, MatchesAll, MatchesListwise
+from testtools.matchers import AnyMatch, MatchesAll, MatchesListwise, Not
 from testtools.testcase import TestCase
 
 from lpbuildd.target.backend import InvalidBuildFilePath
@@ -417,6 +417,74 @@ class TestBuildRock(TestCase):
             build_rock.backend.backend_fs["/usr/local/bin/lpbuildd-git-proxy"],
         )
 
+    def test_install_fetch_service(self):
+        args = [
+            "build-rock",
+            "--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_rock = parse_args(args=args).operation
+        build_rock.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_rock.install()
+        self.assertThat(
+            build_rock.backend.run.calls,
+            MatchesAll(
+                Not(AnyMatch(RanCommand(
+                    ["git", "config", "--global", "protocol.version", "2"]
+                ))),
+            ),
+        )
+
+    def test_install_fetch_service_focal(self):
+        args = [
+            "build-rock",
+            "--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_rock = parse_args(args=args).operation
+        build_rock.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_rock.install()
+        self.assertThat(
+            build_rock.backend.run.calls,
+            MatchesAll(
+                Not(AnyMatch(RanCommand(
+                    ["git", "config", "--global", "protocol.version", "2"]
+                ))),
+            ),
+        )
+
     def test_repo_bzr(self):
         args = [
             "build-rock",
@@ -782,7 +850,6 @@ class TestBuildRock(TestCase):
             "SNAPPY_STORE_NO_CDN": "1",
             'CARGO_HTTP_CAINFO': '/usr/local/share/ca-certificates/local-ca.crt',
             'GOPROXY': 'direct',
-            'GIT_PROTOCOL': 'version=2',
         }
         self.assertThat(
             build_rock.backend.run.calls,
diff --git a/lpbuildd/target/tests/test_build_snap.py b/lpbuildd/target/tests/test_build_snap.py
index da7a7ba..6729aa3 100644
--- a/lpbuildd/target/tests/test_build_snap.py
+++ b/lpbuildd/target/tests/test_build_snap.py
@@ -12,7 +12,7 @@ import responses
 from fixtures import FakeLogger, TempDir
 from systemfixtures import FakeFilesystem
 from testtools import TestCase
-from testtools.matchers import AnyMatch, MatchesAll, MatchesListwise
+from testtools.matchers import AnyMatch, MatchesAll, MatchesListwise, Not
 
 from lpbuildd.target.build_snap import (
     RETCODE_FAILURE_BUILD,
@@ -390,6 +390,58 @@ class TestBuildSnap(TestCase):
             ),
         )
 
+    def test_install_fetch_service(self):
+        args = [
+            "buildsnap",
+            "--backend=fake",
+            "--series=xenial",
+            "--arch=amd64",
+            "1",
+            "--branch",
+            "lp:foo",
+            "test-snap",
+            "--use_fetch_service",
+            "--fetch-service-mitm-certificate",
+            # Base64 content_of_cert
+            "Y29udGVudF9vZl9jZXJ0",
+        ]
+        build_snap = parse_args(args=args).operation
+        build_snap.install()
+        self.assertThat(
+            build_snap.backend.run.calls,
+            MatchesAll(
+                Not(AnyMatch(RanCommand(
+                    ["git", "config", "--global", "protocol.version", "2"]
+                ))),
+            ),
+        )
+
+    def test_install_fetch_service_focal(self):
+        args = [
+            "buildsnap",
+            "--backend=fake",
+            "--series=focal",
+            "--arch=amd64",
+            "1",
+            "--branch",
+            "lp:foo",
+            "test-snap",
+            "--use_fetch_service",
+            "--fetch-service-mitm-certificate",
+            # Base64 content_of_cert
+            "Y29udGVudF9vZl9jZXJ0",
+        ]
+        build_snap = parse_args(args=args).operation
+        build_snap.install()
+        self.assertThat(
+            build_snap.backend.run.calls,
+            MatchesAll(
+                AnyMatch(RanCommand(
+                    ["git", "config", "--global", "protocol.version", "2"]
+                )),
+            ),
+        )
+
     def test_repo_bzr(self):
         args = [
             "buildsnap",
@@ -663,7 +715,6 @@ class TestBuildSnap(TestCase):
             "https_proxy": "http://proxy.example:3128/";,
             "GIT_PROXY_COMMAND": "/usr/local/bin/lpbuildd-git-proxy",
             "SNAPPY_STORE_NO_CDN": "1",
-            'GIT_PROTOCOL': 'version=2'
         }
         self.assertThat(
             build_snap.backend.run.calls,