launchpad-reviewers team mailing list archive

[Ines Almeida <mp+494351@xxxxxxxxxxxxxxxxxx>]

Looking good in general, but the feature flag isn't doing anything right now. See comment

Diff comments:

> diff --git a/lib/lp/services/webhooks/tests/test_job.py b/lib/lp/services/webhooks/tests/test_job.py
> index 2fee4d2..4dd75a5 100644
> --- a/lib/lp/services/webhooks/tests/test_job.py
> +++ b/lib/lp/services/webhooks/tests/test_job.py
> @@ -322,7 +324,7 @@ class MockWebhookClient(WebhookClient):
>  class TestWebhookDeliveryJob(TestCaseWithFactory):
>      """Tests for `WebhookDeliveryJob`."""
>  
> -    layer = ZopelessDatabaseLayer

Can you check if this is still needed after you removed the "package upload" creation in the test?

> +    layer = LaunchpadZopelessLayer
>  
>      def makeAndRunJob(
>          self,
> @@ -438,6 +440,18 @@ class TestWebhookDeliveryJob(TestCaseWithFactory):
>              repr(job),
>          )
>  
> +    def test_archive__repr__(self):
> +        # `WebhookDeliveryJob` objects for archives have an informative
> +        # __repr__.
> +        with FeatureFixture({ARCHIVE_WEBHOOKS_FEATURE_FLAG: "on"}):

Afaics, this feature flag isn't needed here or in any other tests in this MP

You are using the feature flag in tests but you aren't using it to block anything at this point.

You'd use these feature flags when you want to enable or disable some functionality, e.g., only allow triggering a certain webhook for archives if the feature flag is ON, OR only allow creating a webhook for an  archive if the webhook is ON

Right now, in this MP, we aren't doing either, so the feature flag is meaningless and just adds noise.

We could consider adding the feature flag to only allow creating webhooks for archives when the flag is ON, but there isn't a great need for that, specially since we're not exposing any UI or adding actual trigger functionality.

We can add it for the actual trigger functionality, as for example `CI_WEBHOOKS_FEATURE_FLAG` does.

TL;DR: you can remove all things related to this feature flag as it isn't being used anywhere. We can discuss adding it to the trigger functionality as we do for other webhooks. Or if you really want to use feature flags here, we can discuss how to use then properly for the "creation" use-case

> +            archive = self.factory.makeArchive(name="test-archive")
> +        hook = self.factory.makeWebhook(target=archive)
> +        job = WebhookDeliveryJob.create(hook, "test", payload={"foo": "bar"})
> +        self.assertEqual(
> +            "<WebhookDeliveryJob for webhook %d on %r>" % (hook.id, archive),
> +            repr(job),
> +        )
> +
>      def test_short_lease_and_timeout(self):
>          # Webhook jobs have a request timeout of 30 seconds, a celery
>          # timeout of 45 seconds, and a lease of 60 seconds, to give


-- 
https://code.launchpad.net/~vaishnavi-asawale/launchpad/+git/launchpad/+merge/494351
Your team Launchpad code reviewers is requested to review the proposed merge of ~vaishnavi-asawale/launchpad:configure-archive-webhooks into launchpad:master.