launchpad-reviewers team mailing list archive

[Ines Almeida <mp+488673@xxxxxxxxxxxxxxxxxx>]

Added a few more comments (some related to new changes others slipped through the cracks).
Again, nothing major, just a couple of typos and a couple of suggestions

Diff comments:

> diff --git a/lib/lp/bugs/browser/structuralsubscription.py b/lib/lp/bugs/browser/structuralsubscription.py
> index ec278f7..f4874ef 100644
> --- a/lib/lp/bugs/browser/structuralsubscription.py
> +++ b/lib/lp/bugs/browser/structuralsubscription.py
> @@ -284,9 +285,11 @@ class StructuralSubscriptionView(LaunchpadFormView):
>      def userIsDriver(self):
>          """Has the current user driver permissions?"""
>          # We only want to look at this if the target is a
> -        # distribution source package, in order to maintain
> -        # compatibility with the obsolete bug contacts feature.
> -        if IDistributionSourcePackage.providedBy(self.context):
> +        # distribution source packagfe or external package, in order to

Typo `packagfe`

> +        # maintain compatibility with the obsolete bug contacts feature.
> +        if IDistributionSourcePackage.providedBy(
> +            self.context
> +        ) or IExternalPackage.providedBy(self.context):
>              return check_permission(
>                  "launchpad.Driver", self.context.distribution
>              )
> diff --git a/lib/lp/bugs/model/tests/test_bugtask.py b/lib/lp/bugs/model/tests/test_bugtask.py
> index 4549f0a..5306398 100644
> --- a/lib/lp/bugs/model/tests/test_bugtask.py
> +++ b/lib/lp/bugs/model/tests/test_bugtask.py
> @@ -3581,6 +3615,44 @@ class TestValidateTarget(TestCaseWithFactory, ValidateTargetMixin):
>              dsp,
>          )
>  
> +    def test_externalpackage_task_is_allowed(self):
> +        # An External task can coexist with a task for its Distribution.

nit: *ExternalPackage task for clarity

> +        d = self.factory.makeDistribution()
> +        task = self.factory.makeBugTask(target=d)
> +        externalpackage = self.factory.makeExternalPackage(distribution=d)
> +        validate_target(task.bug, externalpackage)
> +
> +    def test_externalpackage_task_with_dsp_is_allowed(self):
> +        # An External task can coexist with a task for a
> +        # DistributionSourcePackage with the same name.
> +        d = self.factory.makeDistribution()
> +        dsp = self.factory.makeDistributionSourcePackage(distribution=d)
> +        task = self.factory.makeBugTask(target=dsp)
> +        externalpackage = self.factory.makeExternalPackage(distribution=d)
> +        validate_target(task.bug, externalpackage)
> +
> +    def test_different_externalpackage_tasks_are_allowed(self):
> +        # An ExternalPackage task can also coexist with a task for another one.
> +        externalpackage = self.factory.makeExternalPackage()
> +        task = self.factory.makeBugTask(target=externalpackage)
> +        externalpackage = self.factory.makeExternalPackage(
> +            distribution=externalpackage.distribution
> +        )
> +        validate_target(task.bug, externalpackage)
> +
> +    def test_same_externalpackage_task_is_forbidden(self):
> +        # But an ExternalPackage task cannot coexist with a task for itself.
> +        externalpackage = self.factory.makeExternalPackage()
> +        task = self.factory.makeBugTask(target=externalpackage)
> +        self.assertRaisesWithContent(
> +            IllegalTarget,
> +            "A fix for this bug has already been requested for %s"
> +            % (externalpackage.displayname),
> +            validate_target,
> +            task.bug,
> +            externalpackage,
> +        )
> +
>      def test_illegal_information_type_disallowed(self):
>          # The bug's current information_type must be permitted by the
>          # new target.
> diff --git a/lib/lp/registry/interfaces/externalpackage.py b/lib/lp/registry/interfaces/externalpackage.py
> new file mode 100644
> index 0000000..e5ec7f4
> --- /dev/null
> +++ b/lib/lp/registry/interfaces/externalpackage.py
> @@ -0,0 +1,157 @@
> +# Copyright 2009, 2025 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""External package interfaces."""
> +
> +__all__ = [
> +    "IExternalPackage",
> +    "ExternalPackageType",
> +]
> +
> +from lazr.enum import DBEnumeratedType, DBItem
> +from lazr.restful.declarations import exported, exported_as_webservice_entry
> +from lazr.restful.fields import Reference
> +from zope.interface import Attribute
> +from zope.schema import TextLine
> +
> +from lp import _
> +from lp.app.interfaces.launchpad import IHeadingContext
> +from lp.bugs.interfaces.bugtarget import IBugTarget, IHasOfficialBugTags
> +from lp.registry.interfaces.distribution import IDistribution
> +from lp.registry.interfaces.role import IHasDrivers
> +
> +
> +@exported_as_webservice_entry(as_of="beta")
> +class IExternalPackageView(
> +    IHeadingContext,
> +    IBugTarget,
> +    IHasOfficialBugTags,
> +    IHasDrivers,
> +):
> +    """`IExternalPackage` attributes that require launchpad.View."""
> +
> +    packagetype = Attribute("The package type")
> +
> +    channel = Attribute("The package channel")
> +
> +    display_channel = TextLine(title=_("Display channel name"), readonly=True)
> +
> +    distribution = exported(
> +        Reference(IDistribution, title=_("The distribution."))
> +    )
> +    sourcepackagename = Attribute("The source package name.")
> +
> +    name = exported(
> +        TextLine(title=_("The source package name as text"), readonly=True)
> +    )
> +    display_name = exported(
> +        TextLine(title=_("Display name for this package."), readonly=True)
> +    )
> +    displayname = Attribute("Display name (deprecated)")
> +    title = exported(
> +        TextLine(title=_("Title for this package."), readonly=True)
> +    )
> +
> +    drivers = Attribute("The drivers for the distribution.")
> +
> +    def __eq__(other):
> +        """IExternalPackage comparison method.
> +
> +        Distro sourcepackages compare equal only if their fields compare equal.
> +        """
> +
> +    def __ne__(other):
> +        """IExternalPackage comparison method.
> +
> +        External packages compare not equal if either of their
> +        fields compare not equal.
> +        """
> +
> +
> +@exported_as_webservice_entry(as_of="beta")
> +class IExternalPackage(
> +    IExternalPackageView,
> +):
> +    """Represents an ExternalPackage in a distribution.
> +
> +    Create IExternalPackage by invoking `IDistribution.getExternalPackage()`.
> +    """
> +
> +
> +class ExternalPackageType(DBEnumeratedType):

On the one hand, maybe we will only set `packagetype` for external packages. On the other, this feels like it can be a generic enum. I'd lean towards naming this just `PackageType`

> +    """Bug Task Status

Docstring needs updating

> +
> +    The various possible states for a bugfix in a specific place.
> +    """
> +
> +    UNKNOWN = DBItem(
> +        0,
> +        """
> +        Unknown
> +
> +        Unknown external package
> +        """,
> +    )
> +
> +    SNAP = DBItem(
> +        1,
> +        """
> +        Snap
> +
> +        Snap external package
> +        """,
> +    )
> +
> +    CHARM = DBItem(
> +        2,
> +        """
> +        Charm
> +
> +        Charm external package
> +        """,
> +    )
> +
> +    ROCK = DBItem(
> +        3,
> +        """
> +        Rock
> +
> +        Rock external package
> +        """,
> +    )
> +
> +    PYTHON = DBItem(
> +        4,
> +        """
> +        Python
> +
> +        Python external package
> +        """,
> +    )
> +
> +    CONDA = DBItem(
> +        5,
> +        """
> +        Conda
> +
> +        Conda external package
> +        """,
> +    )
> +
> +    CARGO = DBItem(
> +        6,
> +        """
> +        Cargo
> +
> +        Cargo external package
> +        """,
> +    )
> +
> +    MAVEN = DBItem(
> +        7,
> +        """
> +        Maven
> +
> +        Maven external package
> +        """,
> +    )
> diff --git a/lib/lp/registry/model/externalpackage.py b/lib/lp/registry/model/externalpackage.py
> new file mode 100644
> index 0000000..a92c19f
> --- /dev/null
> +++ b/lib/lp/registry/model/externalpackage.py
> @@ -0,0 +1,152 @@
> +# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the

nit: 2025

> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Classes to represent external packages in a distribution."""
> +
> +__all__ = [
> +    "ExternalPackage",
> +]
> +
> +from zope.interface import implementer
> +
> +from lp.bugs.model.bugtarget import BugTargetBase
> +from lp.bugs.model.structuralsubscription import (
> +    StructuralSubscriptionTargetMixin,
> +)
> +from lp.registry.interfaces.distribution import IDistribution
> +from lp.registry.interfaces.externalpackage import (
> +    ExternalPackageType,
> +    IExternalPackage,
> +)
> +from lp.registry.interfaces.sourcepackagename import ISourcePackageName
> +from lp.registry.model.hasdrivers import HasDriversMixin
> +from lp.services.channels import channel_list_to_string, channel_string_to_list
> +from lp.services.propertycache import cachedproperty
> +
> +
> +@implementer(IExternalPackage)
> +class ExternalPackage(
> +    BugTargetBase,
> +    HasDriversMixin,
> +    StructuralSubscriptionTargetMixin,
> +):
> +    """This is a "Magic External Package". It is not a Storm model, but instead
> +    it represents a package with a particular name, type and channel in a
> +    particular distribution.
> +    """
> +
> +    def __init__(
> +        self,
> +        distribution: IDistribution,
> +        sourcepackagename: ISourcePackageName,
> +        packagetype: ExternalPackageType,
> +        channel: (str, tuple, list),
> +    ) -> "ExternalPackage":
> +        self.distribution = distribution
> +        self.sourcepackagename = sourcepackagename
> +        self.packagetype = packagetype
> +        self.channel = self.validate_channel(channel)
> +
> +    def __repr__(self) -> str:
> +        return f"<{self.__class__.__name__} '{self.display_name}'>"
> +
> +    def validate_channel(self, channel: (str, tuple, list)) -> tuple:
> +        if channel is None:
> +            return None
> +
> +        if not isinstance(channel, (str, tuple, list)):
> +            raise ValueError("Channel must be a str, tuple or list")
> +
> +        return channel_string_to_list(channel)
> +
> +    @property
> +    def name(self) -> str:
> +        """See `IExternalPackage`."""
> +        return self.sourcepackagename.name
> +
> +    @property
> +    def display_channel(self) -> str:
> +        """See `IExternalPackage`."""
> +        if not self.channel:
> +            return None
> +
> +        return channel_list_to_string(*self.channel)
> +
> +    @cachedproperty
> +    def display_name(self) -> str:
> +        """See `IExternalPackage`."""
> +        if self.channel:
> +            return "%s - %s @%s in %s" % (
> +                self.sourcepackagename.name,
> +                self.packagetype,
> +                self.display_channel,
> +                self.distribution.display_name,
> +            )
> +
> +        return "%s - %s in %s" % (
> +            self.sourcepackagename.name,
> +            self.packagetype,
> +            self.distribution.display_name,
> +        )
> +
> +    # There are different places of launchpad codebase where they use different
> +    # display names
> +    @property
> +    def displayname(self) -> str:

I see in SourcePackage that we just do `displayname = display_name`
Might make it less noisy if it looks clean and make sense, but also up to you - this is clear as is

> +        """See `IExternalPackage`."""
> +        return self.display_name
> +
> +    @property
> +    def bugtargetdisplayname(self) -> str:
> +        """See `IExternalPackage`."""
> +        return self.display_name
> +
> +    @property
> +    def bugtargetname(self) -> str:
> +        """See `IExternalPackage`."""
> +        return self.display_name
> +
> +    @property
> +    def title(self) -> str:
> +        """See `IExternalPackage`."""
> +        return self.display_name
> +
> +    def __eq__(self, other: "ExternalPackage") -> str:
> +        """See `IExternalPackage`."""
> +        return (
> +            (IExternalPackage.providedBy(other))
> +            and (self.distribution.id == other.distribution.id)
> +            and (self.sourcepackagename.id == other.sourcepackagename.id)
> +            and (self.packagetype == other.packagetype)
> +            and (self.channel == other.channel)
> +        )
> +
> +    def __hash__(self) -> int:
> +        """Return the combined attributes hash."""
> +        return hash(
> +            (
> +                self.distribution,
> +                self.sourcepackagename,
> +                self.packagetype,
> +                self.display_channel,
> +            )
> +        )
> +
> +    @property
> +    def drivers(self) -> list:
> +        """See `IHasDrivers`."""
> +        return self.distribution.drivers
> +
> +    @property
> +    def official_bug_tags(self) -> list:
> +        """See `IHasBugs`."""
> +        return self.distribution.official_bug_tags
> +
> +    @property
> +    def pillar(self) -> IDistribution:
> +        """See `IBugTarget`."""
> +        return self.distribution
> +
> +    def _getOfficialTagClause(self):
> +        """See `IBugTarget`."""
> +        return self.distribution._getOfficialTagClause()
> diff --git a/lib/lp/services/channels.py b/lib/lp/services/channels.py
> index 2d36856..508a44e 100644
> --- a/lib/lp/services/channels.py
> +++ b/lib/lp/services/channels.py
> @@ -33,9 +33,19 @@ def channel_string_to_list(channel):
>  
>      :raises ValueError: If the channel string is invalid.
>      """
> -    components = channel.split(CHANNEL_COMPONENTS_DELIMITER)
> +    if isinstance(channel, str):
> +        components = channel.split(CHANNEL_COMPONENTS_DELIMITER)
> +    else:
> +        components = channel
> +
>      if len(components) == 3:
>          track, risk, branch = components
> +        if not _is_risk(risk):

Nice one for making this logic sturdier.
It feels like it can be simplified (get `track, risk, branch` and then check that `_is_risk(risk)` it True and `_is_risk(branch)` is False. I'll leave that up to you

> +            raise ValueError("No valid risk provided: %r" % channel)
> +        if _is_risk(branch):
> +            raise ValueError(
> +                "Branch name cannot match a risk name: %r" % channel
> +            )
>      elif len(components) == 2:
>          # Identify risk to determine if this is track/risk or risk/branch.
>          if _is_risk(components[0]):
> diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
> index d23c481..a940fed 100644
> --- a/lib/lp/testing/factory.py
> +++ b/lib/lp/testing/factory.py
> @@ -5585,6 +5586,28 @@ class LaunchpadObjectFactory(ObjectFactory):
>              )
>          return dsp
>  
> +    def makeExternalPackage(
> +        self,
> +        sourcepackagename=None,
> +        packagetype=None,
> +        channel=None,
> +        distribution=None,
> +    ):
> +        if sourcepackagename is None or isinstance(sourcepackagename, str):
> +            sourcepackagename = self.getOrMakeSourcePackageName(
> +                sourcepackagename
> +            )
> +        if distribution is None:
> +            distribution = self.makeDistribution()
> +        if packagetype is None:
> +            packagetype = ExternalPackageType.SNAP
> +        if channel is None:
> +            channel = "12.1/stable"

If we want to generally have ExternalPackage.channel be a list (JSONB) then I'd make this default to the list format

> +
> +        return distribution.getExternalPackage(
> +            sourcepackagename, packagetype, channel
> +        )
> +
>      def makeEmailMessage(
>          self,
>          body=None,


-- 
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/488673
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:add-external-package into launchpad:master.