launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32738
Re: [Merge] ~enriqueesanchz/launchpad:add-external-package into launchpad:master
Diff comments:
> 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
> @@ -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")]
Done, I've changed the channel to use a tuple instead of a dict so we save some bytes and we are compatible with this function.
> + 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/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):
Nice catch! Done
> + # 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
Added
> + # 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/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)
done :)
> +
> + @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):
Agree, I was wondering if we should try to have 100% code coverage but it doesn't make sense to repeat code (we have to do it in the model with `display_name`, `displayname`... as otherwise we would need to change a lot of LP code)
> + """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
