launchpad-reviewers team mailing list archive

[Ines Almeida <mp+493890@xxxxxxxxxxxxxxxxxx>]

Left a few comments

I think the parts around the Webhook model and creating the webhook targetting archives are good (you could even separate them into a separate MP), but there are some things to be done on the actual webhook triggering side

Diff comments:

> diff --git a/lib/lp/services/webhooks/tests/test_job.py b/lib/lp/services/webhooks/tests/test_job.py
> index 2fee4d2..b4e69b9 100644
> --- a/lib/lp/services/webhooks/tests/test_job.py
> +++ b/lib/lp/services/webhooks/tests/test_job.py
> @@ -438,6 +440,31 @@ class TestWebhookDeliveryJob(TestCaseWithFactory):
>              repr(job),
>          )
>  
> +    def test_source_package_upload__repr__(self):
> +        # `WebhookDeliveryJob` objects for charm recipes have an informative

Docstring needs updating

> +        # __repr__.
> +        ppa = self.factory.makeArchive(
> +            name="test-ppa", purpose=ArchivePurpose.PPA
> +        )
> +        hook = self.factory.makeWebhook(
> +            target=ppa,
> +            delivery_url="http://localhost/test-webhook";,
> +            event_types=["archive:source-package-upload:0.1"],
> +        )
> +        packageupload = self.factory.makePackageUpload(

If here we are just testing the __repr__ of the webhook, than I don't see what you would need to create the actual `packageupload`

> +            archive=ppa,
> +            status=PackageUploadStatus.DONE,
> +        )
> +        job = WebhookDeliveryJob.create(
> +            hook,
> +            "archive:source-package-upload:0.1",
> +            payload={"status": packageupload.status.name},
> +        )
> +        self.assertEqual(
> +            "<WebhookDeliveryJob for webhook %d on %r>" % (hook.id, ppa),
> +            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
> diff --git a/lib/lp/soyuz/subscribers/debbuild.py b/lib/lp/soyuz/subscribers/debbuild.py
> new file mode 100644
> index 0000000..98ee9d7
> --- /dev/null
> +++ b/lib/lp/soyuz/subscribers/debbuild.py
> @@ -0,0 +1,42 @@
> +# Copyright 2021 Canonical Ltd.  This software is licensed under the

nit: Should be 2025

> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Event subscribers for deb builds."""
> +
> +from zope.component import getUtility
> +
> +from lp.services.webapp.publisher import canonical_url
> +from lp.services.webhooks.interfaces import IWebhookSet
> +
> +
> +def _trigger_package_upload_webhook(upload, event_type, action):
> +    payload = {
> +        "package_upload": canonical_url(upload, force_local_path=True),
> +        "action": action,
> +        "status": upload.status.name,
> +        "archive": upload.archive.name,

I think we should actually add the link to the archive itself instead of just the name

```
canonical_url(upload.archive, force_local_path=True)
```

> +    }
> +
> +    # Source package information
> +    if upload.sources[0] and upload.sources[0].sourcepackagerelease:
> +        payload["package_name"] = str(
> +            upload.sources[0].sourcepackagerelease.sourcepackagename
> +        )
> +        payload["package_version"] = str(
> +            upload.sources[0].sourcepackagerelease.version
> +        )
> +
> +    getUtility(IWebhookSet).trigger(upload.archive, event_type, payload)
> +
> +
> +def source_package_upload_webhook(upload, event):
> +    """Webhook for source package uploads."""
> +    if not upload.sources:

It's hard to know what the `upload` variable here is and why this returns in there are no sources.
It might be correct, but please add a comment explaining why we `return` and consider if there is another name for `upload` that would work as well and be more clear

> +        return
> +
> +    if any(field.name == "status" for field in event.edited_fields):

I see instances in the existing of of `status_changed = "status" in event.edited_fields` which seems nicer than a for loop IMO - check if that's possible?

> +        _trigger_package_upload_webhook(
> +            upload,
> +            "archive:source-package-upload:0.1",
> +            "Source package upload",

It doesn't feel right that the action is `Source package upload` if this is triggered on any status change. You might have more insight about this than me, but should we trigger this event only when the status changes to a specific value?

Otherwise, the "action" feels more like "status changed" one. It might be the case that there is only one status change in these `PackageUpload` objects, but if so I'd still make sure we trigger when the status is changed to "X"

> +        )
> diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
> index 1fd3dd6..2945870 100644
> --- a/lib/lp/soyuz/tests/test_archive.py
> +++ b/lib/lp/soyuz/tests/test_archive.py
> @@ -7340,3 +7344,96 @@ class TestArchiveMetadataOverrides(TestCaseWithFactory):
>              Unauthorized,
>              lambda: primary_archive.setMetadataOverrides(overrides),
>          )
> +
> +
> +class TestSourcePackageUploadWebhooks(TestCaseWithFactory):
> +    """Tests that source package upload webhooks fire correctly."""
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def test_source_package_upload_status_change_triggers_webhook(self):
> +        archive = self.factory.makeArchive(name="test-archive")
> +        hook = self.factory.makeWebhook(
> +            target=archive,
> +            delivery_url="http://localhost/test-webhook";,
> +            event_types=["archive:source-package-upload:0.1"],
> +        )
> +
> +        upload = self.factory.makePackageUpload(
> +            archive=archive,
> +            status=PackageUploadStatus.NEW,
> +        )
> +        upload.addSource(
> +            self.factory.makeSourcePackageRelease(
> +                sourcepackagename=self.factory.makeSourcePackageName(
> +                    name="mypkg"
> +                ),
> +                version="1.0.0",
> +            )
> +        )
> +
> +        upload_unwrapped = removeSecurityProxy(upload)
> +        snapshot = type(
> +            "UploadSnapshot", (), {"status": upload_unwrapped.status}
> +        )()
> +        upload_unwrapped.setAccepted()
> +
> +        notify(

Hmmm, it seems like we're not testing whether the webhook is triggered when the upload happens in these 2 tests.
This `notify(ObjectModifiedEvent)` seems to be the thing that actually is triggering the webhook, not the upload.

There are 2 sets of tests you can do here:
 - Test that when you call `notify(ObjectModifiedEvent(...)` on the package upload, the webhook is triggered - whcih is what you're doing here
 - Test that when the upload happens, the `ObjectModifiedEvent` event is trigged (and the webhook is triggered as well as a response)

You're doing the first, but not the second. To test the second, you shouldn't call the `notify` function, and just ensure that that function is called somewhere in the pipeline. We can check together where that might happen if that helps

> +            ObjectModifiedEvent(
> +                upload_unwrapped,
> +                object_before_modification=snapshot,
> +                edited_fields=[type(upload_unwrapped).status],
> +            )
> +        )
> +
> +        job = hook.deliveries.one()
> +        self.assertEqual(job.event_type, "archive:source-package-upload:0.1")
> +
> +        payload = job.payload
> +        self.assertEqual(
> +            payload["package_upload"],
> +            canonical_url(upload, force_local_path=True),
> +        )
> +        self.assertEqual(payload["action"], "Source package upload")
> +        self.assertEqual(payload["status"], "ACCEPTED")
> +        self.assertEqual(payload["archive"], "test-archive")
> +        self.assertEqual(payload["package_name"], "mypkg")
> +        self.assertEqual(payload["package_version"], "1.0.0")
> +
> +    def test_no_source_package_upload_status_change_does_not_trigger_webhook(
> +        self,
> +    ):
> +        archive = self.factory.makeArchive(name="test-archive")
> +        hook = self.factory.makeWebhook(
> +            target=archive,
> +            delivery_url="http://localhost/test-webhook";,
> +            event_types=["archive:source-package-upload:0.1"],
> +        )
> +
> +        upload = self.factory.makePackageUpload(
> +            archive=archive,
> +            status=PackageUploadStatus.NEW,
> +        )
> +        upload.addSource(
> +            self.factory.makeSourcePackageRelease(
> +                sourcepackagename=self.factory.makeSourcePackageName(
> +                    name="mypkg"
> +                ),
> +                version="1.0.0",
> +            )
> +        )
> +
> +        upload_unwrapped = removeSecurityProxy(upload)
> +        snapshot = type(
> +            "UploadSnapshot", (), {"status": upload_unwrapped.status}
> +        )()
> +
> +        notify(
> +            ObjectModifiedEvent(
> +                upload_unwrapped,
> +                object_before_modification=snapshot,
> +                edited_fields=[],
> +            )
> +        )
> +
> +        self.assertEqual(hook.deliveries.count(), 0)


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