launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32730
Re: [Merge] ~enriqueesanchz/launchpad:add-external-package into launchpad:master
Reviewed, added a few comments but nothing too major. Looking good!
Diff comments:
> diff --git a/lib/lp/app/browser/tales.py b/lib/lp/app/browser/tales.py
> index 1714f52..da41350 100644
> --- a/lib/lp/app/browser/tales.py
> +++ b/lib/lp/app/browser/tales.py
> @@ -733,6 +734,9 @@ class ObjectImageDisplayAPI:
> sprite_string = "distribution"
> elif IDistributionSourcePackage.providedBy(context):
> sprite_string = "package-source"
> + elif IExternalPackage.providedBy(context):
> + # TODO: create a new sprite for ExternalPackages?
Sounds like a good idea, I agree it's not a priority.
> + sprite_string = "package-source"
> elif ISprint.providedBy(context):
> sprite_string = "meeting"
> elif IBug.providedBy(context):
> diff --git a/lib/lp/app/widgets/tests/test_launchpadtarget.py b/lib/lp/app/widgets/tests/test_launchpadtarget.py
> index 40350b8..16f5f3e 100644
> --- a/lib/lp/app/widgets/tests/test_launchpadtarget.py
> +++ b/lib/lp/app/widgets/tests/test_launchpadtarget.py
> @@ -61,6 +61,9 @@ class LaunchpadTargetWidgetTestCase(TestCaseWithFactory):
> self.package = self.factory.makeDSPCache(
> distroseries=distroseries, sourcepackagename="snarf"
> )
> + self.externalpackage = self.factory.makeExternalPackage(
> + distribution=self.distribution, sourcepackagename="snarf"
I would consider having a different source package name for the external package for the test to be more meaningful (i.e., the source package name not being the same, so we are sure we are targeting the external package)
> + )
> self.project = self.factory.makeProduct("pting")
> field = Reference(__name__="target", schema=Interface, title="target")
> field = field.bind(Thing())
> diff --git a/lib/lp/bugs/browser/bugtask.py b/lib/lp/bugs/browser/bugtask.py
> index 8fcb135..69f7f0c 100644
> --- a/lib/lp/bugs/browser/bugtask.py
> +++ b/lib/lp/bugs/browser/bugtask.py
> @@ -320,7 +321,12 @@ class BugTargetTraversalMixin:
> # rather than making it look as though this task was "not found",
> # because it was filtered out by privacy-aware code.
> for bugtask in bug.bugtasks:
> - if bugtask.target == context:
> + if bugtask.target == context or IExternalPackage.providedBy(
Hmmm, wouldn't this make it so that if I go to `ubuntu/+source/postgres/+bug/1` and there is an external task there that comes before this one, the browser will target the external one instead of the correct?
Maybe we need to check that there is no other bugtask that matches the context before returning the ExternalPackage one
> + bugtask.target
> + ):
> + # TODO: set +external urls for ExternalPackages
> + # actually we select the first ExternalPackage that appears
nit: s/actually/currently/g
another nit: throughout the Launchpad codebase it's common to have these types of comments but with a name and date so that it's easy to refer to who made the comment. Nowadays there is git blame, but I think a name is nicer so that someone changing a typo in the comment doesn't mess with it. Not all TODOs are the same, there is no need to over think it, but one to keep in mind
> +
> # Security proxy this object on the way out.
> return getUtility(IBugTaskSet).get(bugtask.id)
>
> diff --git a/lib/lp/bugs/browser/structuralsubscription.py b/lib/lp/bugs/browser/structuralsubscription.py
> index ec278f7..96f2687 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
> + # distribution or external package, in order to maintain
I get the meaning, but it would be clearer to say "distribution source package or external package" here, otherwise it sounds like you just mean "distribution"
> # compatibility with the obsolete bug contacts feature.
> - if IDistributionSourcePackage.providedBy(self.context):
> + 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/bugtask.py b/lib/lp/bugs/model/bugtask.py
> index d6116ce..5e9af48 100644
> --- a/lib/lp/bugs/model/bugtask.py
> +++ b/lib/lp/bugs/model/bugtask.py
> @@ -371,6 +389,9 @@ def validate_target(
> )
> except NotFoundError as e:
> raise IllegalTarget(e.args[0])
> + elif IExternalPackage.providedBy(target):
> + # TODO: Check with store/soss that package exists
I would add a little bit more context here
> + pass
>
> legal_types = target.pillar.getAllowedBugInformationTypes()
> new_pillar = target.pillar not in bug.affected_pillars
> @@ -627,6 +661,17 @@ class BugTask(StormBase):
> return self._status
>
> @property
> + def display_channel(self):
> + if self.channel is None:
> + return None
> +
> + channel_list = [self.channel.get("track"), self.channel.get("risk")]
Consider using the `channel_list_to_string` function in lib/lp/services/channels.py
> + if branch := self.channel.get("branch", "") != "":
> + channel_list.append(branch)
> +
> + return "/".join(channel_list)
> +
> + @property
> def title(self):
> """See `IBugTask`."""
> return 'Bug #%s in %s: "%s"' % (
> diff --git a/lib/lp/bugs/model/structuralsubscription.py b/lib/lp/bugs/model/structuralsubscription.py
> index 37e8be2..debb06e 100644
> --- a/lib/lp/bugs/model/structuralsubscription.py
> +++ b/lib/lp/bugs/model/structuralsubscription.py
> @@ -607,7 +608,12 @@ def get_structural_subscriptions_for_bug(bug, person=None):
> # This is here because of a circular import.
> from lp.registry.model.person import Person
>
> - bugtasks = bug.bugtasks
> + bugtasks = []
> + # TODO: support bug subscriptions
I'd also add a little more detail here, it's not clear this is about external packages from this message
> + for bugtask in bug.bugtasks:
> + if not IExternalPackage.providedBy(bugtask.target):
> + bugtasks.append(bugtask)
> +
> if not bugtasks:
> return EmptyResultSet()
> conditions = []
> diff --git a/lib/lp/bugs/model/tests/test_bugtask.py b/lib/lp/bugs/model/tests/test_bugtask.py
> index 4549f0a..59b850c 100644
> --- a/lib/lp/bugs/model/tests/test_bugtask.py
> +++ b/lib/lp/bugs/model/tests/test_bugtask.py
> @@ -3581,6 +3615,35 @@ class TestValidateTarget(TestCaseWithFactory, ValidateTargetMixin):
> dsp,
> )
>
> + def test_externalpackage_task_is_allowed(self):
> + # An External task can coexist with a task for its Distribution.
> + d = self.factory.makeDistribution()
> + task = self.factory.makeBugTask(target=d)
> + externalpackage = self.factory.makeExternalPackage(distribution=d)
> + validate_target(task.bug, externalpackage)
> +
> + def test_different_externalpackage_tasks_are_allowed(self):
Should we add a test to check that an external package can coexist with a sourcepackage for the same name and distribution?
> + # 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/bugs/scripts/bugtasktargetnamecaches.py b/lib/lp/bugs/scripts/bugtasktargetnamecaches.py
> index 70a82d4..a1d408e 100644
> --- a/lib/lp/bugs/scripts/bugtasktargetnamecaches.py
> +++ b/lib/lp/bugs/scripts/bugtasktargetnamecaches.py
> @@ -107,7 +107,10 @@ class BugTaskTargetNameCachesTunableLoop:
> (store.get(cls, id) if id is not None else None)
> for cls, id in zip(target_classes, target_bits)
> )
> - target = bug_target_from_key(*target_objects)
> +
> + # We don't need packagetype and channel to get items from
Why not? Aren't we able to return an external package here?
Can you add more details to the comment if so?
> + # target_classes
> + target = bug_target_from_key(*target_objects, None, None)
> new_name = target.bugtargetdisplayname
> cached_names.discard(new_name)
> # If there are any outdated names cached, update them all in
> diff --git a/lib/lp/bugs/tests/test_structuralsubscription.py b/lib/lp/bugs/tests/test_structuralsubscription.py
> index ce1f712..a22e0d6 100644
> --- a/lib/lp/bugs/tests/test_structuralsubscription.py
> +++ b/lib/lp/bugs/tests/test_structuralsubscription.py
> @@ -548,6 +548,22 @@ class TestGetStructuralSubscriptionTargets(TestCaseWithFactory):
> },
> )
>
> + def test_externalpackage_target(self):
> + actor = self.factory.makePerson()
> + login_person(actor)
> + externalpackage = self.factory.makeExternalPackage()
> + product = self.factory.makeProduct()
> + bug = self.factory.makeBug(target=product)
> + bug.addTask(actor, externalpackage)
> + product_bugtask = bug.bugtasks[0]
> + result = get_structural_subscription_targets(bug.bugtasks)
> + self.assertEqual(
> + set(result),
> + {
> + (product_bugtask, product),
I'm guessing we don't have an external package here because we don't allow it for now.
I'd add a description to the test that mentions that the goal of this test is to check that external package doesn't show because it's not implemented yet
> + },
> + )
> +
> def test_product_with_project_group(self):
> # get_structural_subscription_targets() will yield both a
> # product and its parent project group if it has one.
> diff --git a/lib/lp/registry/model/externalpackage.py b/lib/lp/registry/model/externalpackage.py
> new file mode 100644
> index 0000000..3488794
> --- /dev/null
> +++ b/lib/lp/registry/model/externalpackage.py
> @@ -0,0 +1,165 @@
> +# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
> +# 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.externalpackage import IExternalPackage
> +from lp.registry.model.hasdrivers import HasDriversMixin
> +from lp.services.propertycache import cachedproperty
> +
> +CHANNEL_FIELDS = ("track", "risk", "branch")
> +
> +
> +class ChannelFieldException(Exception):
> + """Channel fields are strings.
> + Track and Risk are required, Branch is optional.
> + """
> +
> +
> +@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, sourcepackagename, packagetype, channel):
> + 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: dict) -> str:
> + if channel is None:
> + return None
> + if not isinstance(channel, dict):
> + raise ChannelFieldException("Channel should be a dict")
> + if "track" not in channel:
> + raise ChannelFieldException("Track is a required field in channel")
> + if "risk" not in channel:
> + raise ChannelFieldException("Risk is a required field in channel")
> +
> + for k, v in channel.items():
> + if k not in CHANNEL_FIELDS:
> + raise ChannelFieldException(
> + f"{k} is not part of {CHANNEL_FIELDS}"
> + )
> + if not isinstance(v, str):
> + raise ChannelFieldException(
> + "All channel fields should be a string"
> + )
> + return channel
> +
> + @property
> + def name(self):
> + """See `IExternalPackage`."""
> + return self.sourcepackagename.name
> +
> + @property
> + def display_channel(self):
> + """See `IExternalPackage`."""
> + if not self.channel:
> + return None
> +
> + channel_list = [self.channel.get("track"), self.channel.get("risk")]
> + if (branch := self.channel.get("branch", "")) != "":
> + channel_list.append(branch)
> +
> + return "/".join(channel_list)
Consider the function I mentioned above (see if it makes sense for our case)
> +
> + @cachedproperty
> + def display_name(self):
> + """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):
> + """See `IExternalPackage`."""
> + return self.display_name
> +
> + @property
> + def bugtargetdisplayname(self):
> + """See `IExternalPackage`."""
> + return self.display_name
> +
> + @property
> + def bugtargetname(self):
> + """See `IExternalPackage`."""
> + return self.display_name
> +
> + @property
> + def title(self):
> + """See `IExternalPackage`."""
> + return self.display_name
> +
> + def __eq__(self, other):
> + """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):
> + """Return the combined attributes hash."""
> + return hash(
> + (
> + self.distribution,
> + self.sourcepackagename,
> + self.packagetype,
> + self.display_channel,
> + )
> + )
> +
> + @property
> + def drivers(self):
> + """See `IHasDrivers`."""
> + return self.distribution.drivers
> +
> + @property
> + def official_bug_tags(self):
> + """See `IHasBugs`."""
> + return self.distribution.official_bug_tags
> +
> + @property
> + def pillar(self):
> + """See `IBugTarget`."""
> + return self.distribution
> +
> + def _getOfficialTagClause(self):
> + """See `IBugTarget`."""
> + return self.distribution._getOfficialTagClause()
> diff --git a/lib/lp/registry/tests/test_externalpackage.py b/lib/lp/registry/tests/test_externalpackage.py
> new file mode 100644
> index 0000000..613cefe
> --- /dev/null
> +++ b/lib/lp/registry/tests/test_externalpackage.py
> @@ -0,0 +1,229 @@
> +# Copyright 2009-2025 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Tests for ExternalPackage."""
> +
> +from zope.security.proxy import removeSecurityProxy
> +
> +from lp.registry.interfaces.externalpackage import ExternalPackageType
> +from lp.registry.model.externalpackage import (
> + ChannelFieldException,
> + ExternalPackage,
> +)
> +from lp.testing import TestCaseWithFactory
> +from lp.testing.layers import DatabaseFunctionalLayer
> +
> +
> +class TestExternalPackage(TestCaseWithFactory):
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super().setUp()
> +
> + self.sourcepackagename = self.factory.getOrMakeSourcePackageName(
> + "mypackage"
> + )
> + self.channel = {"track": "12.81", "risk": "edge", "branch": "myfix"}
> + self.distribution = self.factory.makeDistribution(name="mydistro")
> +
> + self.externalpackage = self.distribution.getExternalPackage(
> + name=self.sourcepackagename,
> + packagetype=ExternalPackageType.SNAP,
> + channel=self.channel,
> + )
> + self.externalpackage_maven = self.distribution.getExternalPackage(
> + name=self.sourcepackagename,
> + packagetype=ExternalPackageType.MAVEN,
> + channel=None,
> + )
> + self.externalpackage_copy = ExternalPackage(
> + self.distribution,
> + sourcepackagename=self.sourcepackagename,
> + packagetype=ExternalPackageType.SNAP,
> + channel=self.channel,
> + )
> +
> + def test_repr(self):
> + """Test __repr__ function"""
> + self.assertEqual(
> + "<ExternalPackage 'mypackage - Snap @12.81/edge/myfix in "
> + "Mydistro'>",
> + self.externalpackage.__repr__(),
> + )
> + self.assertEqual(
> + "<ExternalPackage 'mypackage - Maven in Mydistro'>",
> + self.externalpackage_maven.__repr__(),
> + )
> +
> + def test_name(self):
> + """Test name property"""
> + self.assertEqual("mypackage", self.externalpackage.name)
> + self.assertEqual("mypackage", self.externalpackage_maven.name)
> +
> + def test_display_channel(self):
> + """Test display_channel property"""
> + self.assertEqual(
> + self.externalpackage.display_channel, "12.81/edge/myfix"
> + )
> + self.assertEqual(self.externalpackage_maven.display_channel, None)
> +
> + removeSecurityProxy(self.externalpackage).channel = {
> + "track": "12.81",
> + "risk": "candidate",
> + }
> + self.assertEqual(
> + "12.81/candidate", self.externalpackage.display_channel
> + )
> +
> + def test_channel_fields(self):
> + """Test invalid channel fields when creating an ExternalPackage"""
> + self.assertRaises(
> + ChannelFieldException,
> + ExternalPackage,
> + self.distribution,
> + self.sourcepackagename,
> + ExternalPackageType.SNAP,
> + {},
> + )
> + self.assertRaises(
> + ChannelFieldException,
> + ExternalPackage,
> + self.distribution,
> + self.sourcepackagename,
> + ExternalPackageType.CHARM,
> + {"track": 16},
> + )
> + self.assertRaises(
> + ChannelFieldException,
> + ExternalPackage,
> + self.distribution,
> + self.sourcepackagename,
> + ExternalPackageType.CHARM,
> + {"track": "16"},
> + )
> + self.assertRaises(
> + ChannelFieldException,
> + ExternalPackage,
> + self.distribution,
> + self.sourcepackagename,
> + ExternalPackageType.ROCK,
> + {"risk": "beta"},
> + )
> + self.assertRaises(
> + ChannelFieldException,
> + ExternalPackage,
> + self.distribution,
> + self.sourcepackagename,
> + ExternalPackageType.PYTHON,
> + {"track": "16", "risk": "beta", "foo": "bar"},
> + )
> + self.assertRaises(
> + ChannelFieldException,
> + ExternalPackage,
> + self.distribution,
> + self.sourcepackagename,
> + ExternalPackageType.CONDA,
> + 1,
> + )
> +
> + def test_display_name(self):
I would separate this into 2 tests, one with and one without the channel since it's testing 2 separate paths.
The other tests since it's mainly checking that they display the same as the original `display_name`, then I don't see a need for that
> + """Test display_name property"""
> + self.assertEqual(
> + "mypackage - Snap @12.81/edge/myfix in Mydistro",
> + self.externalpackage.display_name,
> + )
> + self.assertEqual(
> + "mypackage - Maven in Mydistro",
> + self.externalpackage_maven.display_name,
> + )
> +
> + def test_displayname(self):
> + """Test displayname property"""
> + self.assertEqual(
> + "mypackage - Snap @12.81/edge/myfix in Mydistro",
> + self.externalpackage.display_name,
> + )
> + self.assertEqual(
> + "mypackage - Maven in Mydistro",
> + self.externalpackage_maven.display_name,
> + )
> +
> + def test_bugtargetdisplayname(self):
> + """Test bugtargetdisplayname property"""
> + self.assertEqual(
> + "mypackage - Snap @12.81/edge/myfix in Mydistro",
> + self.externalpackage.bugtargetdisplayname,
> + )
> + self.assertEqual(
> + "mypackage - Maven in Mydistro",
> + self.externalpackage_maven.bugtargetdisplayname,
> + )
> +
> + def test_bugtargetname(self):
> + """Test bugtargetname property"""
> + self.assertEqual(
> + "mypackage - Snap @12.81/edge/myfix in Mydistro",
> + self.externalpackage.bugtargetname,
> + )
> + self.assertEqual(
> + "mypackage - Maven in Mydistro",
> + self.externalpackage_maven.bugtargetname,
> + )
> +
> + def test_title(self):
> + """Test title property"""
> + self.assertEqual(
> + "mypackage - Snap @12.81/edge/myfix package in Mydistro",
> + self.externalpackage.title,
> + )
> + self.assertEqual(
> + "mypackage - Maven package in Mydistro",
> + self.externalpackage_maven.title,
> + )
> +
> + def test_compare(self):
> + """Test __eq__ and __neq__"""
> + self.assertEqual(self.externalpackage, self.externalpackage_copy)
> + self.assertNotEqual(self.externalpackage, self.externalpackage_maven)
> +
> + def test_hash(self):
> + """Test __hash__"""
> + self.assertEqual(
> + removeSecurityProxy(self.externalpackage).__hash__(),
> + removeSecurityProxy(self.externalpackage_copy).__hash__(),
> + )
> + self.assertNotEqual(
> + removeSecurityProxy(self.externalpackage).__hash__(),
> + removeSecurityProxy(self.externalpackage_maven).__hash__(),
> + )
> +
> + def test_pillar(self):
> + """Test pillar property"""
> + self.assertEqual(self.externalpackage.pillar, self.distribution)
> +
> + def test_official_bug_tags(self):
> + """Test official_bug_tags property"""
> + self.assertEqual(
> + self.externalpackage.official_bug_tags,
> + self.distribution.official_bug_tags,
> + )
> +
> + def test__getOfficialTagClause(self):
> + """Test _getOfficialTagClause"""
> + self.assertEqual(
> + self.distribution._getOfficialTagClause(),
> + self.externalpackage._getOfficialTagClause(),
> + )
> +
> + def test_drivers_are_distributions(self):
> + """Drivers property returns the drivers for the distribution."""
> + self.assertNotEqual([], self.distribution.drivers)
> + self.assertEqual(
> + self.externalpackage.drivers, self.distribution.drivers
> + )
> +
> + def test_personHasDriverRights(self):
> + """A distribution driver has driver permissions on an
> + externalpackage."""
> + driver = self.distribution.drivers[0]
> + self.assertTrue(self.externalpackage.personHasDriverRights(driver))
--
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.
References
