Merge ~ines-almeida/launchpad:fetch-service-add-missing-fetch-service-args into launchpad:master

[Ines Almeida]

Add missing `use_fetch_service` arg when starting a build

[Jürgen Gmach (jugmac00)]

It looks like that as a minimum we need to add the new arg to the `BuildArgs` class in buildfarmjobbehaviour.py.

Additionally, I would love to see an additional or updated test related to this code change.

Also, how did you notice the missing arg? This might be valuable information to avoid similar issues in the future, as we need to add new build types in the next cycle.

[Ines Almeida (ines-almeida)]

Updated (still squashed into 1 commit since it was just a 1-liner)

This was a very quick MP that I opened right before the EOD to get it out of the way, it isn't blocking any testing as I cowboyed it in qastaging.

As for noticing the missing arg: I simply noticed some code wasn't running in buildd, and I checked the build logs to find that this expected arg wasn't there. Adding it fixed the initial issue.

[Jürgen Gmach (jugmac00)]

review: Approve

diff --git a/lib/lp/buildmaster/builderproxy.py b/lib/lp/buildmaster/builderproxy.py
index 9bacf76..1fc0d9b 100644
--- a/lib/lp/buildmaster/builderproxy.py
+++ b/lib/lp/buildmaster/builderproxy.py
@@ -98,6 +98,7 @@ class BuilderProxyMixin:
 
         args["proxy_url"] = session_data["proxy_url"]
         args["revocation_endpoint"] = session_data["revocation_endpoint"]
+        args["use_fetch_service"] = use_fetch_service
 
     @defer.inlineCallbacks
     def endProxySession(self, upload_path: str):
diff --git a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
index 6a808b3..3c8a792 100644
--- a/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py
@@ -142,6 +142,8 @@ class BuildArgs(TypedDict, total=False):
     # A list of base64-encoded public keys for apt archives used by this
     # build.
     trusted_keys: List[str]
+    # Is true, use the fetch service as the proxy within the builder.
+    use_fetch_service: bool
 
 
 class IBuildFarmJobBehaviour(Interface):
diff --git a/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py b/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py
index 07eb6e8..aaff515 100644
--- a/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py
+++ b/lib/lp/charms/tests/test_charmrecipebuildbehaviour.py
@@ -315,6 +315,7 @@ class TestAsyncCharmRecipeBuildBehaviour(
                     ),
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
@@ -356,6 +357,7 @@ class TestAsyncCharmRecipeBuildBehaviour(
                     ),
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
@@ -468,6 +470,7 @@ class TestAsyncCharmRecipeBuildBehaviour(
         self.assertThat(
             build_request[4]["proxy_url"], ProxyURLMatcher(job, self.now)
         )
+        self.assertFalse(build_request[4]["use_fetch_service"])
 
     @defer.inlineCallbacks
     def test_composeBuildRequest_git_ref_deleted(self):
diff --git a/lib/lp/code/model/tests/test_cibuildbehaviour.py b/lib/lp/code/model/tests/test_cibuildbehaviour.py
index 727f75a..d0c2beb 100644
--- a/lib/lp/code/model/tests/test_cibuildbehaviour.py
+++ b/lib/lp/code/model/tests/test_cibuildbehaviour.py
@@ -311,6 +311,7 @@ class TestAsyncCIBuildBehaviour(StatsMixin, TestCIBuildBehaviourBase):
                     ),
                     "series": Equals(job.build.distro_series.name),
                     "trusted_keys": Equals(expected_trusted_keys),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
@@ -467,6 +468,7 @@ class TestAsyncCIBuildBehaviour(StatsMixin, TestCIBuildBehaviourBase):
                         }
                     ),
                     "secrets": Equals({"soss_read_auth": "user:pass"}),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
@@ -648,6 +650,7 @@ class TestAsyncCIBuildBehaviour(StatsMixin, TestCIBuildBehaviourBase):
                     ),
                     "series": Equals(job.build.distro_series.name),
                     "trusted_keys": Equals(expected_trusted_keys),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index e0bd58e..6d74c78 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -227,6 +227,7 @@ class TestAsyncOCIRecipeBuildBehaviour(
         self.assertEqual([], self.proxy_api.tokens.requests)
         self.assertNotIn("proxy_url", args)
         self.assertNotIn("revocation_endpoint", args)
+        self.assertNotIn("use_fetch_service", args)
 
     @defer.inlineCallbacks
     def test_requestProxyToken_no_secret(self):
@@ -520,6 +521,7 @@ class TestAsyncOCIRecipeBuildBehaviour(
                             ),
                         }
                     ),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
@@ -623,6 +625,7 @@ class TestAsyncOCIRecipeBuildBehaviour(
                             ),
                         }
                     ),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
@@ -683,6 +686,7 @@ class TestAsyncOCIRecipeBuildBehaviour(
                             ),
                         }
                     ),
+                    "use_fetch_service": Is(False),
                 }
             ),
         )
@@ -867,6 +871,7 @@ class TestAsyncOCIRecipeBuildBehaviour(
             args = yield job.extraBuildArgs()
         self.assertNotIn("proxy_url", args)
         self.assertNotIn("revocation_endpoint", args)
+        self.assertNotIn("use_fetch_service", args)
 
 
 class TestHandleStatusForOCIRecipeBuild(
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index d48efbc..81d6926 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -399,6 +399,7 @@ class TestAsyncSnapBuildBehaviourFetchService(
         )
         self.assertIn("proxy_url", args)
         self.assertIn("revocation_endpoint", args)
+        self.assertTrue(args["use_fetch_service"])
         self.assertIn("secrets", args)
         self.assertIn("fetch_service_mitm_certificate", args["secrets"])
         self.assertIn(
@@ -575,6 +576,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
         self.assertEqual([], self.proxy_api.tokens.requests)
         self.assertNotIn("proxy_url", args)
         self.assertNotIn("revocation_endpoint", args)
+        self.assertNotIn("use_fetch_service", args)
 
     @defer.inlineCallbacks
     def test_requestProxyToken_no_secret(self):
@@ -660,6 +662,11 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
             self.assertIn("universe", archive_line)
         with dbuser(config.builddmaster.dbuser):
             args = yield job.extraBuildArgs()
+
+        # XXX ines-almeida 2024-04-26: use_fetch_service is `None` because we
+        # are not setting the fetch-service feature flag 'ON' for these tests
+        # (since that's not the point of these test). Once we remove the
+        # feature flag, this will either be True or False - not None.
         self.assertThat(
             args,
             MatchesDict(
@@ -681,6 +688,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
                     "target_architectures": Equals(["i386"]),
+                    "use_fetch_service": Is(None),
                 }
             ),
         )
@@ -734,6 +742,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
                     "target_architectures": Equals(["i386"]),
+                    "use_fetch_service": Is(None),
                 }
             ),
         )
@@ -776,6 +785,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
                     "target_architectures": Equals(["i386"]),
+                    "use_fetch_service": Is(None),
                 }
             ),
         )
@@ -849,6 +859,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
                     "target_architectures": Equals(["i386"]),
+                    "use_fetch_service": Is(None),
                 }
             ),
         )
@@ -894,6 +905,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
                     "target_architectures": Equals(["i386"]),
+                    "use_fetch_service": Is(None),
                 }
             ),
         )
@@ -936,6 +948,7 @@ class TestAsyncSnapBuildBehaviourBuilderProxy(
                     "series": Equals("unstable"),
                     "trusted_keys": Equals(expected_trusted_keys),
                     "target_architectures": Equals(["i386"]),
+                    "use_fetch_service": Is(None),
                 }
             ),
         )