launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+461721@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/buildmaster/downloader.py b/lib/lp/buildmaster/downloader.py
> index fc16852..cee308a 100644
> --- a/lib/lp/buildmaster/downloader.py
> +++ b/lib/lp/buildmaster/downloader.py
> @@ -94,3 +110,24 @@ class RequestProcess(AMPChild):
>              )
>              response.raise_for_status()
>              return response.json()
> +
> +    @RequestFetchServiceSessionCommand.responder
> +    def requestFetchServiceSessionCommand(
> +        self, url, auth_header, proxy_username
> +    ):
> +        with Session() as session:
> +            session.trust_env = False
> +            # XXX pelpsi 2024-02-28: should take the same

https://docs.google.com/document/d/1Ta0THOsHLwbOA6H7ewHa-6s2GtZRWxvvtiMKFk5jiq8/edit from here I can see that the API take these two params as input, it's not clear if they are mandatory or not. I should ask to Claudio to clarify that.

> +            # json body? From ST108 we should provide
> +            # {
> +            #   "timeout": <int>,			// session timeout in seconds
> +            #   "policy": <string>			// "strict" or "permissive"
> +            # }
> +            response = session.post(
> +                url,
> +                headers={"Authorization": auth_header},
> +                json={"username": proxy_username},
> +            )
> +            print(" CI SIAMO???? ")
> +            response.raise_for_status()
> +            return response.json()
> diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> index 5ec5804..45ee8c8 100644
> --- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> +++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
> @@ -296,6 +317,85 @@ class TestAsyncSnapBuildBehaviour(StatsMixin, TestSnapBuildBehaviourBase):
>          )
>  
>      @defer.inlineCallbacks
> +    def test_requestFetchServiceSession_unconfigured(self):
> +        self.pushConfig("builddmaster", fetch_service_host=None)
> +        self.useFixture(
> +            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})

Yep, it's right but I don't know if it makes sense to add a test on a feature flag, what do you think? I think that I can add it with a comment to remove it in future.

> +        )
> +        snap = self.factory.makeSnap(use_fetch_service=True)
> +        request = self.factory.makeSnapBuildRequest(snap=snap)
> +        job = self.makeJob(snap=snap, build_request=request)
> +        with dbuser(config.builddmaster.dbuser):
> +            args = yield job.extraBuildArgs()
> +        self.assertEqual([], self.fetch_service_api.tokens.requests)
> +        self.assertNotIn("proxy_url", args)
> +        self.assertNotIn("revocation_endpoint", args)
> +
> +    @defer.inlineCallbacks
> +    def test_requestFetchServiceSession_no_secret(self):
> +        self.pushConfig(
> +            "builddmaster", fetch_service_auth_api_admin_secret=None
> +        )
> +        self.useFixture(
> +            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
> +        )
> +        snap = self.factory.makeSnap(use_fetch_service=True)
> +        request = self.factory.makeSnapBuildRequest(snap=snap)
> +        job = self.makeJob(snap=snap, build_request=request)
> +        expected_exception_msg = (
> +            "fetch_service_auth_api_admin_secret is not configured."
> +        )
> +        with ExpectedException(CannotBuild, expected_exception_msg):
> +            yield job.extraBuildArgs()
> +
> +    @defer.inlineCallbacks
> +    def test_requestFetchServiceSession(self):
> +        self.useFixture(
> +            FeatureFixture({SNAP_USE_FETCH_SERVICE_FEATURE_FLAG: "on"})
> +        )
> +        snap = self.factory.makeSnap(use_fetch_service=True)
> +        request = self.factory.makeSnapBuildRequest(snap=snap)
> +        job = self.makeJob(snap=snap, build_request=request)
> +        yield job.extraBuildArgs()
> +        expected_uri = urlsplit(
> +            config.builddmaster.fetch_service_auth_api_endpoint
> +        ).path.encode("UTF-8")
> +        request_matcher = MatchesDict(
> +            {
> +                "method": Equals(b"POST"),
> +                "uri": Equals(expected_uri),
> +                "headers": ContainsDict(
> +                    {
> +                        b"Authorization": MatchesListwise(
> +                            [
> +                                Equals(
> +                                    b"Basic "
> +                                    + base64.b64encode(
> +                                        b"admin-launchpad.test:admin-secret"
> +                                    )
> +                                )
> +                            ]
> +                        ),
> +                        b"Content-Type": MatchesListwise(
> +                            [
> +                                Equals(b"application/json"),
> +                            ]
> +                        ),
> +                    }
> +                ),
> +                "json": MatchesDict(
> +                    {
> +                        "username": StartsWith(job.build.build_cookie + "-"),
> +                    }
> +                ),
> +            }
> +        )
> +        self.assertThat(
> +            self.fetch_service_api.tokens.requests,
> +            MatchesListwise([request_matcher]),
> +        )
> +
> +    @defer.inlineCallbacks
>      def test_requestProxyToken_unconfigured(self):
>          self.pushConfig("builddmaster", builder_proxy_host=None)
>          branch = self.factory.makeBranch()


-- 
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/461721
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:fetch-service-session-API into launchpad:master.