launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28402
[Merge] ~cjwatson/launchpad:hardcode-store-risks into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:hardcode-store-risks into launchpad:master.
Commit message:
Hardcode the list of "risks" in Canonical's stores
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/420737
Canonical's stores (the snap store, Charmhub, etc.) use a vocabulary of "risks" that form part of channel names. When we initially set this up it made sense to query the snap store to get hold of that vocabulary. However, the list of risks has been stable for some time and is unlikely to change without substantial notice, and having to query an external service just in order to do things like splitting a channel string into components is a significant amount of complexity; the endpoint we're querying is also deprecated.
Replace all this with a simple hardcoded enumeration, in `lp.registry` since nowadays this applies to multiple applications.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:hardcode-store-risks into launchpad:master.
diff --git a/lib/lp/charms/browser/tests/test_charmrecipe.py b/lib/lp/charms/browser/tests/test_charmrecipe.py
index e37a38a..2324fa6 100644
--- a/lib/lp/charms/browser/tests/test_charmrecipe.py
+++ b/lib/lp/charms/browser/tests/test_charmrecipe.py
@@ -60,7 +60,6 @@ from lp.services.job.interfaces.job import JobStatus
from lp.services.propertycache import get_property_cache
from lp.services.webapp import canonical_url
from lp.services.webapp.servers import LaunchpadTestRequest
-from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
from lp.testing import (
BrowserTestCase,
login,
@@ -70,8 +69,6 @@ from lp.testing import (
TestCaseWithFactory,
time_counter,
)
-from lp.testing.fakemethod import FakeMethod
-from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import (
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
@@ -127,13 +124,6 @@ class BaseTestCharmRecipeView(BrowserTestCase):
super().setUp()
self.useFixture(FeatureFixture({CHARM_RECIPE_ALLOW_CREATE: "on"}))
self.useFixture(FakeLogger())
- self.snap_store_client = FakeMethod()
- self.snap_store_client.listChannels = FakeMethod(result=[
- {"name": "stable", "display_name": "Stable"},
- {"name": "edge", "display_name": "Edge"},
- ])
- self.useFixture(
- ZopeUtilityFixture(self.snap_store_client, ISnapStoreClient))
self.person = self.factory.makePerson(
name="test-person", displayname="Test Person")
diff --git a/lib/lp/registry/enums.py b/lib/lp/registry/enums.py
index d150e02..119d5e0 100644
--- a/lib/lp/registry/enums.py
+++ b/lib/lp/registry/enums.py
@@ -18,6 +18,7 @@ __all__ = [
'VCSType',
'SharingPermission',
'SpecificationSharingPolicy',
+ 'StoreRisk',
'TeamMembershipPolicy',
'TeamMembershipRenewalPolicy',
]
@@ -491,3 +492,20 @@ class PollSort(EnumeratedType):
Sort polls with the earliest closing date first.
""")
+
+
+class StoreRisk(EnumeratedType):
+ """The "risk" element of a channel in one of Canonical's stores.
+
+ The snap store, Charmhub, and other similar stores operated by Canonical
+ have a model where a "channel" consists of a track, a risk, and a
+ branch. The track and branch elements are both somewhat free-form (the
+ track functions a little like a major version number in semver, while
+ the branch is for short-lived hotfixes), but the risk element is from a
+ fixed vocabulary, modelled here.
+ """
+
+ STABLE = Item("stable", "Stable")
+ CANDIDATE = Item("candidate", "Candidate")
+ BETA = Item("beta", "Beta")
+ EDGE = Item("edge", "Edge")
diff --git a/lib/lp/services/webhooks/tests/test_browser.py b/lib/lp/services/webhooks/tests/test_browser.py
index 12089e1..d5ae2c6 100644
--- a/lib/lp/services/webhooks/tests/test_browser.py
+++ b/lib/lp/services/webhooks/tests/test_browser.py
@@ -25,7 +25,6 @@ from lp.oci.interfaces.ocirecipe import (
from lp.services.features.testing import FeatureFixture
from lp.services.webapp.interfaces import IPlacelessAuthUtility
from lp.services.webapp.publisher import canonical_url
-from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
from lp.soyuz.interfaces.livefs import (
LIVEFS_FEATURE_FLAG,
LIVEFS_WEBHOOKS_FEATURE_FLAG,
@@ -35,8 +34,6 @@ from lp.testing import (
record_two_runs,
TestCaseWithFactory,
)
-from lp.testing.fakemethod import FakeMethod
-from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import DatabaseFunctionalLayer
from lp.testing.matchers import HasQueryCount
from lp.testing.pages import extract_text
@@ -100,13 +97,6 @@ class SnapTestHelpers:
("snap:build:0.1", "Snap build"),
]
- def setUp(self):
- super().setUp()
- snap_store_client = FakeMethod()
- snap_store_client.listChannels = FakeMethod(result=[])
- self.useFixture(
- ZopeUtilityFixture(snap_store_client, ISnapStoreClient))
-
def makeTarget(self):
self.useFixture(FeatureFixture({
'webhooks.new.enabled': 'true',
diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index bbc0be7..d3f4bca 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -144,10 +144,6 @@ class TestSnapViewsFeatureFlag(TestCaseWithFactory):
# Without a private_snap feature flag, we will not create Snaps for
# private contexts.
self.useFixture(BranchHostingFixture())
- self.snap_store_client = FakeMethod()
- self.snap_store_client.listChannels = FakeMethod(result=[])
- self.useFixture(
- ZopeUtilityFixture(self.snap_store_client, ISnapStoreClient))
owner = self.factory.makePerson()
branch = self.factory.makeAnyBranch(
owner=owner, information_type=InformationType.USERDATA)
@@ -166,10 +162,6 @@ class BaseTestSnapView(BrowserTestCase):
self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.useFixture(FakeLogger())
self.snap_store_client = FakeMethod()
- self.snap_store_client.listChannels = FakeMethod(result=[
- {"name": "stable", "display_name": "Stable"},
- {"name": "edge", "display_name": "Edge"},
- ])
self.snap_store_client.requestPackageUploadPermission = (
getUtility(ISnapStoreClient).requestPackageUploadPermission)
self.useFixture(
diff --git a/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py b/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py
index 6deb9de..78d6a48 100644
--- a/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py
+++ b/lib/lp/snappy/browser/widgets/tests/test_storechannelswidget.py
@@ -11,18 +11,16 @@ from zope.formlib.interfaces import (
from zope.schema import List
from lp.app.validators import LaunchpadValidationError
+from lp.registry.enums import StoreRisk
from lp.services.beautifulsoup import BeautifulSoup
from lp.services.webapp.escaping import html_escape
from lp.services.webapp.servers import LaunchpadTestRequest
from lp.snappy.browser.widgets.storechannels import StoreChannelsWidget
-from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
from lp.snappy.vocabularies import SnapStoreChannelVocabulary
from lp.testing import (
TestCaseWithFactory,
verifyObject,
)
-from lp.testing.fakemethod import FakeMethod
-from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import DatabaseFunctionalLayer
@@ -38,18 +36,6 @@ class TestStoreChannelsWidget(TestCaseWithFactory):
request = LaunchpadTestRequest()
self.widget = StoreChannelsWidget(field, None, request)
- # setup fake store client response for available channels/risks
- self.risks = [
- {"name": "stable", "display_name": "Stable"},
- {"name": "candidate", "display_name": "Candidate"},
- {"name": "beta", "display_name": "Beta"},
- {"name": "edge", "display_name": "Edge"},
- ]
- snap_store_client = FakeMethod()
- snap_store_client.listChannels = FakeMethod(result=self.risks)
- self.useFixture(
- ZopeUtilityFixture(snap_store_client, ISnapStoreClient))
-
def test_implements(self):
self.assertTrue(verifyObject(IBrowserWidget, self.widget))
self.assertTrue(verifyObject(IInputWidget, self.widget))
@@ -280,7 +266,7 @@ class TestStoreChannelsWidget(TestCaseWithFactory):
soup = BeautifulSoup(markup)
fields = soup.find_all(["input"], {"id": re.compile(".*")})
expected_ids = [
- "field.channels.risks.%d" % i for i in range(len(self.risks))]
+ "field.channels.risks.%d" % i for i in range(len(StoreRisk))]
expected_ids.append("field.channels.track")
expected_ids.append("field.channels.branch")
ids = [field["id"] for field in fields]
diff --git a/lib/lp/snappy/interfaces/snapstoreclient.py b/lib/lp/snappy/interfaces/snapstoreclient.py
index 1e00979..f809b99 100644
--- a/lib/lp/snappy/interfaces/snapstoreclient.py
+++ b/lib/lp/snappy/interfaces/snapstoreclient.py
@@ -7,7 +7,6 @@ __all__ = [
'BadRefreshResponse',
'BadRequestPackageUploadResponse',
'BadScanStatusResponse',
- 'BadSearchResponse',
'ISnapStoreClient',
'NeedsRefreshResponse',
'ScanFailedResponse',
@@ -67,10 +66,6 @@ class ScanFailedResponse(SnapStoreError):
pass
-class BadSearchResponse(SnapStoreError):
- pass
-
-
class ISnapStoreClient(Interface):
"""Interface for the API provided by the snap store."""
@@ -143,12 +138,3 @@ class ISnapStoreClient(Interface):
store with further information about this upload, and `revision`
is the store revision number for the upload or None.
"""
-
- def listChannels():
- """Fetch the current list of channels from the store.
-
- :raises BadSearchResponse: if the attempt to fetch the list of
- channels from the store fails.
- :return: A list of dictionaries, one per channel, each of which
- contains at least "name" and "display_name" keys.
- """
diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
index b56f5b6..e1478dd 100644
--- a/lib/lp/snappy/model/snapstoreclient.py
+++ b/lib/lp/snappy/model/snapstoreclient.py
@@ -10,8 +10,6 @@ __all__ = [
import base64
import json
import string
-import time
-from urllib.parse import urlsplit
from lazr.restful.utils import get_current_browser_request
from pymacaroons import Macaroon
@@ -27,9 +25,7 @@ from lp.services.crypto.interfaces import (
CryptoError,
IEncryptedContainer,
)
-from lp.services.features import getFeatureFlag
from lp.services.librarian.utils import EncodableLibraryFileAlias
-from lp.services.memcache.interfaces import IMemcacheClient
from lp.services.scripts import log
from lp.services.timeline.requesttimeline import get_request_timeline
from lp.services.timeout import urlfetch
@@ -38,7 +34,6 @@ from lp.snappy.interfaces.snapstoreclient import (
BadRefreshResponse,
BadRequestPackageUploadResponse,
BadScanStatusResponse,
- BadSearchResponse,
ISnapStoreClient,
NeedsRefreshResponse,
ScanFailedResponse,
@@ -384,41 +379,3 @@ class SnapStoreClient:
return response_data["url"], response_data["revision"]
except requests.HTTPError as e:
raise cls._makeSnapStoreError(BadScanStatusResponse, e)
-
- @classmethod
- def listChannels(cls):
- """See `ISnapStoreClient`."""
- if config.snappy.store_search_url is None:
- return _default_store_channels
- memcache_client = getUtility(IMemcacheClient)
- search_host = urlsplit(config.snappy.store_search_url).hostname
- memcache_key = ("%s:channels" % search_host).encode("UTF-8")
- description = "channels for %s" % search_host
-
- channels = memcache_client.get_json(memcache_key, log, description)
-
- if (channels is None and
- not getFeatureFlag("snap.disable_channel_search")):
- path = "api/v1/channels"
- timeline = cls._getTimeline()
- if timeline is not None:
- action = timeline.start("store-search-get", "/" + path)
- channels_url = urlappend(config.snappy.store_search_url, path)
- try:
- response = urlfetch(
- channels_url, headers={"Accept": "application/hal+json"})
- except requests.HTTPError as e:
- raise cls._makeSnapStoreError(BadSearchResponse, e)
- finally:
- if timeline is not None:
- action.finish()
- channels = response.json().get("_embedded", {}).get(
- "clickindex:channel", [])
- DAY_IN_SECONDS = 60 * 60 * 24
- # pymemcache's `expire` expects an int
- expire_time = int(time.time()) + DAY_IN_SECONDS
- memcache_client.set_json(
- memcache_key, channels, expire_time, logger=log)
- if channels is None:
- channels = _default_store_channels
- return channels
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 0dae2d5..abd99d3 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -2819,10 +2819,6 @@ class TestSnapWebservice(TestCaseWithFactory):
super().setUp()
self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
self.snap_store_client = FakeMethod()
- self.snap_store_client.listChannels = FakeMethod(result=[
- {"name": "stable", "display_name": "Stable"},
- {"name": "edge", "display_name": "Edge"},
- ])
self.snap_store_client.requestPackageUploadPermission = (
getUtility(ISnapStoreClient).requestPackageUploadPermission)
self.useFixture(
diff --git a/lib/lp/snappy/tests/test_snapbuildjob.py b/lib/lp/snappy/tests/test_snapbuildjob.py
index e0c485e..c483e6d 100644
--- a/lib/lp/snappy/tests/test_snapbuildjob.py
+++ b/lib/lp/snappy/tests/test_snapbuildjob.py
@@ -74,7 +74,6 @@ class FakeSnapStoreClient:
self.uploadFile = FakeMethod()
self.push = FakeMethod()
self.checkStatus = FakeMethod()
- self.listChannels = FakeMethod(result=[])
class FileUploaded(MatchesListwise):
diff --git a/lib/lp/snappy/tests/test_snapstoreclient.py b/lib/lp/snappy/tests/test_snapstoreclient.py
index b4bd3ea..e702558 100644
--- a/lib/lp/snappy/tests/test_snapstoreclient.py
+++ b/lib/lp/snappy/tests/test_snapstoreclient.py
@@ -44,7 +44,6 @@ from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
from lp.snappy.interfaces.snapstoreclient import (
BadRequestPackageUploadResponse,
BadScanStatusResponse,
- BadSearchResponse,
ISnapStoreClient,
ScanFailedResponse,
UnauthorizedUploadResponse,
@@ -741,42 +740,3 @@ class TestSnapStoreClient(TestCaseWithFactory):
"can_release": False,
})
self.assertEqual((None, None), self.client.checkStatus(status_url))
-
- @responses.activate
- def test_listChannels(self):
- self._addChannelsResponse()
- self.assertEqual(self.channels, self.client.listChannels())
- self.assertThat(responses.calls[-1].request, RequestMatches(
- url=Equals("http://search.example/api/v1/channels";),
- method=Equals("GET"),
- headers=ContainsDict({"Accept": Equals("application/hal+json")})))
- self.assertEqual(
- self.channels,
- json.loads(getUtility(IMemcacheClient).get(
- self.channels_memcache_key)))
- responses.reset()
- self.assertEqual(self.channels, self.client.listChannels())
- self.assertContentEqual([], responses.calls)
-
- @responses.activate
- def test_listChannels_404(self):
- responses.add(
- "GET", "http://search.example/api/v1/channels";, status=404)
- self.assertRaisesWithContent(
- BadSearchResponse, "404 Client Error: Not Found",
- self.client.listChannels)
-
- @responses.activate
- def test_listChannels_disable_search(self):
- self.useFixture(
- FeatureFixture({"snap.disable_channel_search": "on"}))
- expected_channels = [
- {"name": "candidate", "display_name": "Candidate"},
- {"name": "edge", "display_name": "Edge"},
- {"name": "beta", "display_name": "Beta"},
- {"name": "stable", "display_name": "Stable"},
- ]
- self.assertEqual(expected_channels, self.client.listChannels())
- self.assertContentEqual([], responses.calls)
- self.assertIsNone(
- getUtility(IMemcacheClient).get(self.channels_memcache_key))
diff --git a/lib/lp/snappy/validators/channels.py b/lib/lp/snappy/validators/channels.py
index 6fa3cef..77a210b 100644
--- a/lib/lp/snappy/validators/channels.py
+++ b/lib/lp/snappy/validators/channels.py
@@ -3,10 +3,9 @@
"""Validators for the .store_channels attribute."""
-from zope.schema.vocabulary import getVocabularyRegistry
-
from lp import _
from lp.app.validators import LaunchpadValidationError
+from lp.registry.enums import StoreRisk
from lp.services.webapp.escaping import (
html_escape,
structured,
@@ -19,13 +18,7 @@ channel_components_delimiter = '/'
def _is_risk(component):
"""Does this channel component identify a risk?"""
- vocabulary = getVocabularyRegistry().get(None, "SnapStoreChannel")
- try:
- vocabulary.getTermByToken(component)
- except LookupError:
- return False
- else:
- return True
+ return component in {item.title for item in StoreRisk.items}
def split_channel_name(channel):
diff --git a/lib/lp/snappy/validators/tests/test_channels.py b/lib/lp/snappy/validators/tests/test_channels.py
index 02b19ad..11a3d9b 100644
--- a/lib/lp/snappy/validators/tests/test_channels.py
+++ b/lib/lp/snappy/validators/tests/test_channels.py
@@ -2,14 +2,11 @@
# GNU Affero General Public License version 3 (see the file LICENSE).
from lp.app.validators import LaunchpadValidationError
-from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
from lp.snappy.validators.channels import (
channels_validator,
split_channel_name,
)
from lp.testing import TestCaseWithFactory
-from lp.testing.fakemethod import FakeMethod
-from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import LaunchpadFunctionalLayer
@@ -17,19 +14,6 @@ class TestChannelsValidator(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
- def setUp(self):
- super().setUp()
- self.risks = [
- {"name": "stable", "display_name": "Stable"},
- {"name": "candidate", "display_name": "Candidate"},
- {"name": "beta", "display_name": "Beta"},
- {"name": "edge", "display_name": "Edge"},
- ]
- snap_store_client = FakeMethod()
- snap_store_client.listChannels = FakeMethod(result=self.risks)
- self.useFixture(
- ZopeUtilityFixture(snap_store_client, ISnapStoreClient))
-
def test_split_channel_name_no_track_or_branch(self):
self.assertEqual((None, "edge", None), split_channel_name("edge"))
diff --git a/lib/lp/snappy/vocabularies.py b/lib/lp/snappy/vocabularies.py
index 8a22a9c..4c694e9 100644
--- a/lib/lp/snappy/vocabularies.py
+++ b/lib/lp/snappy/vocabularies.py
@@ -17,7 +17,6 @@ from storm.locals import (
Desc,
Not,
)
-from zope.component import getUtility
from zope.interface import implementer
from zope.schema.vocabulary import (
SimpleTerm,
@@ -25,6 +24,7 @@ from zope.schema.vocabulary import (
)
from zope.security.proxy import removeSecurityProxy
+from lp.registry.enums import StoreRisk
from lp.registry.model.distribution import Distribution
from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.series import ACTIVE_STATUSES
@@ -34,7 +34,6 @@ from lp.services.utils import seconds_since_epoch
from lp.services.webapp.vocabulary import StormVocabularyBase
from lp.snappy.interfaces.snap import ISnap
from lp.snappy.interfaces.snappyseries import ISnappyDistroSeries
-from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
from lp.snappy.model.snappyseries import (
SnappyDistroSeries,
SnappyDistroSeriesMixin,
@@ -224,17 +223,15 @@ class SnapStoreChannelVocabulary(SimpleVocabulary):
"""A vocabulary for searching store channels."""
def __init__(self, context=None):
- channels = getUtility(ISnapStoreClient).listChannels()
terms = [
- self.createTerm(
- channel["name"], channel["name"], channel["display_name"])
- for channel in channels]
+ self.createTerm(item.title, item.title, item.description)
+ for item in StoreRisk.items]
if ISnap.providedBy(context):
# Supplement the vocabulary with any obsolete channels still
# used by this context.
context_channels = removeSecurityProxy(context)._store_channels
if context_channels is not None:
- known_names = {channel["name"] for channel in channels}
+ known_names = {item.title for item in StoreRisk.items}
for name in context_channels:
if name not in known_names:
terms.append(self.createTerm(name, name, name))
