launchpad-reviewers team mailing list archive

[Ines Almeida <mp+494750@xxxxxxxxxxxxxxxxxx>]

Added a couple more comments
Nice work getting into these complex systems!

Diff comments:

> diff --git a/lib/lp/services/features/flags.py b/lib/lp/services/features/flags.py
> index 85a37d0..57f5f2b 100644
> --- a/lib/lp/services/features/flags.py
> +++ b/lib/lp/services/features/flags.py
> @@ -349,6 +349,14 @@ flag_info = sorted(
>              "",
>              "",
>          ),
> +        (
> +            "archive.webhooks.enabled",

My feeling is that we will never want to enable the webhook for a unit and not the other, even if it happens in 2 units... So I'd be OK with keeping the flag generic and turn off all archive webhooks if we find anything wrong

Though I don't mind if we go with more granularity

> +            "boolean",
> +            "If true, allow webhooks to be configured on archives",
> +            "enabled",
> +            "",
> +            "",
> +        ),
>      ]
>  )
>  
> diff --git a/lib/lp/soyuz/subscribers/archive.py b/lib/lp/soyuz/subscribers/archive.py
> new file mode 100644
> index 0000000..1ecb44f
> --- /dev/null
> +++ b/lib/lp/soyuz/subscribers/archive.py
> @@ -0,0 +1,54 @@
> +# Copyright 2025 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Event subscribers for source package uploads"""
> +
> +from zope.component import getUtility
> +
> +from lp.services.features import getFeatureFlag
> +from lp.services.webapp.publisher import canonical_url
> +from lp.services.webhooks.interfaces import IWebhookSet
> +from lp.soyuz.enums import PackageUploadStatus
> +from lp.soyuz.interfaces.archive import ARCHIVE_WEBHOOKS_FEATURE_FLAG
> +
> +
> +def _trigger_source_package_status_change_webhook(upload, event_type, action):
> +    if getFeatureFlag(ARCHIVE_WEBHOOKS_FEATURE_FLAG):
> +        payload = {
> +            "package_upload": canonical_url(upload, force_local_path=True),
> +            "action": action,
> +            "status": upload.status.name,
> +            "archive": 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 package_status_change_webhook(upload, event):
> +    """Webhook for source and binary package uploads."""
> +
> +    # For source packages
> +    # Instead of checking upload.sources, we check upload.builds, because
> +    # there are instances of rejected source package uploads which do not have
> +    # any sources
> +    if not upload.builds:
> +        if "status" in event.edited_fields and (
> +            upload.status == PackageUploadStatus.ACCEPTED
> +            or upload.status == PackageUploadStatus.REJECTED
> +            or upload.status == PackageUploadStatus.UNAPPROVED
> +        ):
> +            status = upload.status.name.lower()
> +            _trigger_source_package_status_change_webhook(
> +                upload,
> +                f"archive:source-package-upload:0.1::{status}",
> +                f"source-package-upload-{status}",

I'm on the fence, but I think the action should be "status-changed" here.
We then give the "status" in the payload for the user to do what they want with it, and we allow users to subscope it eitherways...

The 'status-changed' feels more coherent with other webhooks.

I'm not 100% set on it though, happy to hear opposing opinions!

> +            )


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