launchpad-reviewers team mailing list archive

[Ines Almeida <mp+473393@xxxxxxxxxxxxxxxxxx>]

Looking good, left a few comments to be addressed

Lint build failed, need to look into something needs fixing or just needs a re-run.

For another MP:
For snaps we set a `SNAP_USE_FETCH_SERVICE_FEATURE_FLAG` feature flag. I don't think we need to add one here specially since there are feature flags for building rocks already, but I'd like it if we removed the SNAP one to get both consistent given it seems to be no longer required.

Diff comments:

> diff --git a/lib/lp/rocks/model/rockrecipe.py b/lib/lp/rocks/model/rockrecipe.py
> index cb7d748..a597ad5 100644
> --- a/lib/lp/rocks/model/rockrecipe.py
> +++ b/lib/lp/rocks/model/rockrecipe.py
> @@ -241,6 +241,8 @@ class RockRecipe(StormBase):
>  
>      _store_channels = JSON("store_channels", allow_none=True)
>  
> +    _use_fetch_service = Bool(name="use_fetch_service", allow_none=False)

We used this pattern (private variable with getter and setter) due to the feature flag to be able to `hide` the value if the feature flag was False.

I don't think it hurts to keep this pattern, but at this point it would be simpler to just have 
```
use_fetch_service = Bool(name="use_fetch_service", allow_none=False)
```
and remove the getter and setter methods in the lines below.

> +
>      def __init__(
>          self,
>          registrant,
> diff --git a/lib/lp/rocks/tests/test_rockrecipe.py b/lib/lp/rocks/tests/test_rockrecipe.py
> index 1f9c807..e9a95bf 100644
> --- a/lib/lp/rocks/tests/test_rockrecipe.py
> +++ b/lib/lp/rocks/tests/test_rockrecipe.py
> @@ -443,6 +455,7 @@ class TestRockRecipeSet(TestCaseWithFactory):
>          self.assertIsNone(recipe.store_name)
>          self.assertIsNone(recipe.store_secrets)
>          self.assertEqual([], recipe.store_channels)
> +        self.assertFalse(recipe.use_fetch_service)

Can you add a test to verify that non-admins can't set the `use_fetch_service` field, as we have for snaps (see TestSnapSet.test_admins_can_update_admin_only_fields)

>  
>      def test_creation_no_source(self):
>          # Attempting to create a rock recipe without a Git repository
> diff --git a/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py b/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py
> index 88cea3a..3dcd1db 100644
> --- a/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py
> +++ b/lib/lp/rocks/tests/test_rockrecipebuildbehaviour.py
> @@ -425,6 +438,151 @@ class TestAsyncRockRecipeBuildBehaviour(
>          )
>  
>  
> +class TestAsyncSnapBuildBehaviourFetchService(

(1) Should be TestAsync*Rock*BuildBehaviourFetchService

(2) Is there a reason why we are not adding all the tests under `TestAsyncSnapBuildBehaviourFetchService` here as well?

(3) these feels like duplication from the snaps that could be avoided given none of these tests are very rock specific (nor are the snap ones snap specific) - it just requires the right inheritance.
Given we've followed down this path to duplicate what exists for snaps, I think it's OK to leave it. Just leaving this note here

> +    StatsMixin, TestRockRecipeBuildBehaviourBase
> +):
> +    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
> +        timeout=30
> +    )
> +
> +    @defer.inlineCallbacks
> +    def setUp(self):
> +        super().setUp()
> +        self.session = {
> +            "id": "1",
> +            "token": uuid.uuid4().hex,
> +        }
> +        self.fetch_service_url = (
> +            "http://{session_id}:{token}@{host}:{port}".format(
> +                session_id=self.session["id"],
> +                token=self.session["token"],
> +                host=config.builddmaster.fetch_service_host,
> +                port=config.builddmaster.fetch_service_port,
> +            )
> +        )
> +        self.fetch_service_api = self.useFixture(
> +            InProcessFetchServiceAuthAPIFixture()
> +        )
> +        yield self.fetch_service_api.start()
> +        self.now = time.time()
> +        self.useFixture(fixtures.MockPatch("time.time", return_value=self.now))
> +        self.addCleanup(shut_down_default_process_pool)
> +        self.setUpStats()
> +
> +    def makeJob(self, **kwargs):
> +        # We need a builder worker in these tests, in order that requesting
> +        # a proxy token can piggyback on its reactor and pool.
> +        job = super().makeJob(**kwargs)
> +        builder = MockBuilder()
> +        builder.processor = job.build.processor
> +        worker = self.useFixture(WorkerTestHelpers()).getClientWorker()
> +        job.setBuilder(builder, worker)
> +        self.addCleanup(worker.pool.closeCachedConnections)
> +        return job
> +
> +    @defer.inlineCallbacks
> +    def test_requestFetchServiceSession_unconfigured(self):
> +        """Create a rock recipe build request with an incomplete fetch service
> +        configuration.
> +
> +        If `fetch_service_host` is not provided the function will return
> +        without populating `proxy_url` and `revocation_endpoint`.
> +        """
> +        self.pushConfig("builddmaster", fetch_service_host=None)
> +        job = self.makeJob(use_fetch_service=True)
> +        with dbuser(config.builddmaster.dbuser):
> +            args = yield job.extraBuildArgs()
> +        self.assertEqual([], self.fetch_service_api.sessions.requests)
> +        self.assertNotIn("proxy_url", args)
> +        self.assertNotIn("revocation_endpoint", args)
> +
> +    @defer.inlineCallbacks
> +    def test_requestFetchServiceSession_no_certificate(self):
> +        """Create a rock recipe build request with an incomplete fetch service
> +        configuration.
> +
> +        If `fetch_service_mitm_certificate` is not provided
> +        the function raises a `CannotBuild` error.
> +        """
> +        self.pushConfig("builddmaster", fetch_service_mitm_certificate=None)
> +        job = self.makeJob(use_fetch_service=True)
> +        expected_exception_msg = (
> +            "fetch_service_mitm_certificate is not configured."
> +        )
> +        with ExpectedException(CannotBuild, expected_exception_msg):
> +            yield job.extraBuildArgs()
> +
> +    @defer.inlineCallbacks
> +    def test_requestFetchServiceSession_no_secret(self):
> +        """Create a rock recipe build request with an incomplete fetch service
> +        configuration.
> +
> +        If `fetch_service_control_admin_secret` is not provided
> +        the function raises a `CannotBuild` error.
> +        """
> +        self.pushConfig(
> +            "builddmaster", fetch_service_control_admin_secret=None
> +        )
> +        job = self.makeJob(use_fetch_service=True)
> +        expected_exception_msg = (
> +            "fetch_service_control_admin_secret is not configured."
> +        )
> +        with ExpectedException(CannotBuild, expected_exception_msg):
> +            yield job.extraBuildArgs()
> +
> +    @defer.inlineCallbacks
> +    def test_requestFetchServiceSession(self):
> +        """Create a rock recipe build request with a successful fetch service
> +        configuration.
> +
> +        `proxy_url` and `revocation_endpoint` are correctly populated.
> +        """
> +        job = self.makeJob(use_fetch_service=True)
> +        args = yield job.extraBuildArgs()
> +        request_matcher = MatchesDict(
> +            {
> +                "method": Equals(b"POST"),
> +                "uri": Equals(b"/session"),
> +                "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(
> +                    {
> +                        "policy": Equals("permissive"),
> +                    }
> +                ),
> +            }
> +        )
> +        self.assertThat(
> +            self.fetch_service_api.sessions.requests,
> +            MatchesListwise([request_matcher]),
> +        )
> +        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(
> +            "fake-cert", args["secrets"]["fetch_service_mitm_certificate"]
> +        )
> +
> +
>  class MakeRockRecipeBuildMixin:
>      """Provide the common makeBuild method returning a queued build."""
>  


-- 
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/473393
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:fetch-service-configuration-for-rock-builds into launchpad:master.