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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
