launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22548
[Merge] lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad with lp:~cjwatson/launchpad/better-bzr-mp-diffs as a prerequisite.
Commit message:
Extract initial Snap.store_name from snapcraft.yaml for Bazaar as well as Git.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-initial-name-bzr/+merge/345757
The potential timeout issues will be at least as bad as with git, but BranchHostingClient has all the same sort of timeout management code as we put together for GitHostingClient, which should mitigate that.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-initial-name-bzr into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2018-05-17 14:25:53 +0000
+++ lib/lp/code/interfaces/branch.py 2018-05-17 14:25:53 +0000
@@ -765,6 +765,15 @@
:param launchbag: `ILaunchBag`.
"""
+ def getBlob(filename, revision_id=None):
+ """Get a blob by file name from this branch.
+
+ :param filename: Relative path of a file in the branch.
+ :param revision_id: An optional revision ID. Defaults to the last
+ scanned revision ID of the branch.
+ :return: The blob content as a byte string.
+ """
+
def getDiff(new, old):
"""Get the diff between two revisions in this branch.
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2018-05-17 14:25:53 +0000
+++ lib/lp/code/model/branch.py 2018-05-17 14:25:53 +0000
@@ -10,12 +10,15 @@
from datetime import datetime
from functools import partial
+import json
import operator
+import os.path
from bzrlib import urlutils
from bzrlib.revision import NULL_REVISION
from lazr.lifecycle.event import ObjectCreatedEvent
import pytz
+from six.moves.urllib_parse import urlsplit
from sqlobject import (
ForeignKey,
IntCol,
@@ -84,6 +87,7 @@
)
from lp.code.errors import (
AlreadyLatestFormat,
+ BranchFileNotFound,
BranchMergeProposalExists,
BranchTargetError,
BranchTypeError,
@@ -172,10 +176,12 @@
ArrayAgg,
ArrayIntersects,
)
+from lp.services.features import getFeatureFlag
from lp.services.helpers import shortlist
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
from lp.services.mail.notificationrecipientset import NotificationRecipientSet
+from lp.services.memcache.interfaces import IMemcacheClient
from lp.services.propertycache import (
cachedproperty,
get_property_cache,
@@ -783,6 +789,63 @@
RevisionAuthor, revisions, ['revision_author_id'])
return DecoratedResultSet(result, pre_iter_hook=eager_load)
+ def getBlob(self, filename, revision_id=None, enable_memcache=None,
+ logger=None):
+ """See `IBranch`."""
+ hosting_client = getUtility(IBranchHostingClient)
+ if enable_memcache is None:
+ enable_memcache = not getFeatureFlag(
+ u'code.bzr.blob.disable_memcache')
+ if revision_id is None:
+ revision_id = self.last_scanned_id
+ if revision_id is None:
+ # revision_id may still be None if the branch scanner hasn't
+ # scanned this branch yet. In this case, we won't be able to
+ # guarantee that subsequent calls to this method within the same
+ # transaction with revision_id=None will see the same revision,
+ # and we won't be able to cache file lists. Neither is fatal,
+ # and this should be relatively rare.
+ enable_memcache = False
+ dirname = os.path.dirname(filename)
+ unset = object()
+ file_list = unset
+ if enable_memcache:
+ memcache_client = getUtility(IMemcacheClient)
+ instance_name = urlsplit(
+ config.codehosting.internal_bzr_api_endpoint).hostname
+ memcache_key = '%s:bzr-file-list:%s:%s:%s' % (
+ instance_name, self.id, revision_id, dirname)
+ if not isinstance(memcache_key, bytes):
+ memcache_key = memcache_key.encode('UTF-8')
+ cached_file_list = memcache_client.get(memcache_key)
+ if cached_file_list is not None:
+ try:
+ file_list = json.loads(cached_file_list)
+ except Exception:
+ logger.exception(
+ 'Cannot load cached file list for %s:%s:%s; deleting' %
+ (self.unique_name, revision_id, dirname))
+ memcache_client.delete(memcache_key)
+ if file_list is unset:
+ try:
+ inventory = hosting_client.getInventory(
+ self.unique_name, dirname, rev=revision_id)
+ file_list = {
+ entry['filename']: entry['file_id']
+ for entry in inventory['filelist']}
+ except BranchFileNotFound:
+ file_list = None
+ if enable_memcache:
+ # Cache the file list in case there's a request for another
+ # file in the same directory.
+ memcache_client.set(memcache_key, json.dumps(file_list))
+ file_id = (file_list or {}).get(os.path.basename(filename))
+ if file_id is None:
+ raise BranchFileNotFound(
+ self.unique_name, filename=filename, rev=revision_id)
+ return hosting_client.getBlob(
+ self.unique_name, file_id, rev=revision_id)
+
def getDiff(self, new, old=None):
"""See `IBranch`."""
hosting_client = getUtility(IBranchHostingClient)
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2018-03-06 00:59:06 +0000
+++ lib/lp/code/model/tests/test_branch.py 2018-05-17 14:25:53 +0000
@@ -11,6 +11,7 @@
datetime,
timedelta,
)
+import json
from bzrlib.branch import Branch
from bzrlib.bzrdir import BzrDir
@@ -61,6 +62,7 @@
AlreadyLatestFormat,
BranchCreatorNotMemberOfOwnerTeam,
BranchCreatorNotOwner,
+ BranchFileNotFound,
BranchTargetError,
CannotDeleteBranch,
CannotUpgradeNonHosted,
@@ -109,7 +111,10 @@
from lp.code.model.branchrevision import BranchRevision
from lp.code.model.codereviewcomment import CodeReviewComment
from lp.code.model.revision import Revision
-from lp.code.tests.helpers import add_revision_to_branch
+from lp.code.tests.helpers import (
+ add_revision_to_branch,
+ BranchHostingFixture,
+ )
from lp.codehosting.safe_open import BadUrl
from lp.codehosting.vfs.branchfs import get_real_branch_path
from lp.registry.enums import (
@@ -136,6 +141,7 @@
block_on_job,
monitor_celery,
)
+from lp.services.memcache.interfaces import IMemcacheClient
from lp.services.osutils import override_environ
from lp.services.propertycache import clear_property_cache
from lp.services.webapp.authorization import check_permission
@@ -159,6 +165,7 @@
)
from lp.testing.dbuser import dbuser
from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.fakemethod import FakeMethod
from lp.testing.layers import (
CeleryBranchWriteJobLayer,
CeleryBzrsyncdJobLayer,
@@ -3293,6 +3300,150 @@
self.assertRaises(BadUrl, db_stacked.getBzrBranch)
+class TestBranchGetBlob(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_default_rev_unscanned(self):
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture(
+ file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+ blob = branch.getBlob('src/README.txt')
+ self.assertEqual('Some text', blob)
+ self.assertEqual(
+ [((branch.unique_name, 'src'), {'rev': None})],
+ hosting_fixture.getInventory.calls)
+ self.assertEqual(
+ [((branch.unique_name, 'some-file-id'), {'rev': None})],
+ hosting_fixture.getBlob.calls)
+ self.assertEqual({}, getUtility(IMemcacheClient)._cache)
+
+ def test_default_rev_scanned(self):
+ branch = self.factory.makeBranch()
+ removeSecurityProxy(branch).last_scanned_id = 'scanned-id'
+ hosting_fixture = self.useFixture(BranchHostingFixture(
+ file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+ blob = branch.getBlob('src/README.txt')
+ self.assertEqual('Some text', blob)
+ self.assertEqual(
+ [((branch.unique_name, 'src'), {'rev': 'scanned-id'})],
+ hosting_fixture.getInventory.calls)
+ self.assertEqual(
+ [((branch.unique_name, 'some-file-id'), {'rev': 'scanned-id'})],
+ hosting_fixture.getBlob.calls)
+ key = (
+ 'bazaar.launchpad.dev:bzr-file-list:%s:scanned-id:src' % branch.id)
+ self.assertEqual(
+ json.dumps({'README.txt': 'some-file-id'}),
+ getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+ def test_with_rev(self):
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture(
+ file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+ blob = branch.getBlob('src/README.txt', revision_id='some-rev')
+ self.assertEqual('Some text', blob)
+ self.assertEqual(
+ [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+ hosting_fixture.getInventory.calls)
+ self.assertEqual(
+ [((branch.unique_name, 'some-file-id'), {'rev': 'some-rev'})],
+ hosting_fixture.getBlob.calls)
+ key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+ self.assertEqual(
+ json.dumps({'README.txt': 'some-file-id'}),
+ getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+ def test_cached_inventory(self):
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture(
+ blob=b'Some text'))
+ key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+ getUtility(IMemcacheClient).set(
+ key.encode('UTF-8'), json.dumps({'README.txt': 'some-file-id'}))
+ blob = branch.getBlob('src/README.txt', revision_id='some-rev')
+ self.assertEqual('Some text', blob)
+ self.assertEqual([], hosting_fixture.getInventory.calls)
+ self.assertEqual(
+ [((branch.unique_name, 'some-file-id'), {'rev': 'some-rev'})],
+ hosting_fixture.getBlob.calls)
+
+ def test_disable_memcache(self):
+ self.useFixture(FeatureFixture(
+ {'code.bzr.blob.disable_memcache': 'on'}))
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture(
+ file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+ key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+ getUtility(IMemcacheClient).set(key.encode('UTF-8'), '{}')
+ blob = branch.getBlob('src/README.txt', revision_id='some-rev')
+ self.assertEqual('Some text', blob)
+ self.assertEqual(
+ [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+ hosting_fixture.getInventory.calls)
+ self.assertEqual(
+ '{}', getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+ def test_file_at_root_of_branch(self):
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture(
+ file_list={'README.txt': 'some-file-id'}, blob=b'Some text'))
+ blob = branch.getBlob('README.txt', revision_id='some-rev')
+ self.assertEqual('Some text', blob)
+ self.assertEqual(
+ [((branch.unique_name, ''), {'rev': 'some-rev'})],
+ hosting_fixture.getInventory.calls)
+ self.assertEqual(
+ [((branch.unique_name, 'some-file-id'), {'rev': 'some-rev'})],
+ hosting_fixture.getBlob.calls)
+ key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:' % branch.id
+ self.assertEqual(
+ json.dumps({'README.txt': 'some-file-id'}),
+ getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+ def test_file_not_in_directory(self):
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture(file_list={}))
+ self.assertRaises(
+ BranchFileNotFound, branch.getBlob,
+ 'src/README.txt', revision_id='some-rev')
+ self.assertEqual(
+ [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+ hosting_fixture.getInventory.calls)
+ self.assertEqual([], hosting_fixture.getBlob.calls)
+ key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+ self.assertEqual(
+ '{}', getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+ def test_missing_directory(self):
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture())
+ hosting_fixture.getInventory = FakeMethod(
+ failure=BranchFileNotFound(
+ branch.unique_name, filename='src', rev='some-rev'))
+ self.assertRaises(
+ BranchFileNotFound, branch.getBlob,
+ 'src/README.txt', revision_id='some-rev')
+ self.assertEqual(
+ [((branch.unique_name, 'src'), {'rev': 'some-rev'})],
+ hosting_fixture.getInventory.calls)
+ self.assertEqual([], hosting_fixture.getBlob.calls)
+ key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+ self.assertEqual(
+ 'null', getUtility(IMemcacheClient).get(key.encode('UTF-8')))
+
+ def test_cached_missing_directory(self):
+ branch = self.factory.makeBranch()
+ hosting_fixture = self.useFixture(BranchHostingFixture())
+ key = 'bazaar.launchpad.dev:bzr-file-list:%s:some-rev:src' % branch.id
+ getUtility(IMemcacheClient).set(key.encode('UTF-8'), 'null')
+ self.assertRaises(
+ BranchFileNotFound, branch.getBlob,
+ 'src/README.txt', revision_id='some-rev')
+ self.assertEqual([], hosting_fixture.getInventory.calls)
+ self.assertEqual([], hosting_fixture.getBlob.calls)
+
+
class TestBranchUnscan(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py 2018-05-17 14:25:53 +0000
+++ lib/lp/code/tests/helpers.py 2018-05-17 14:25:53 +0000
@@ -306,14 +306,33 @@
class BranchHostingFixture(fixtures.Fixture):
"""A fixture that temporarily registers a fake Bazaar hosting client."""
- def __init__(self, diff=None, inventory=None, blob=None):
+ def __init__(self, diff=None, inventory=None, file_list=None, blob=None,
+ disable_memcache=True):
self.create = FakeMethod()
self.getDiff = FakeMethod(result=diff or {})
- self.getInventory = FakeMethod(result=inventory or {})
+ if inventory is None:
+ if file_list is not None:
+ # Simple common case.
+ inventory = {
+ "filelist": [
+ {"filename": filename, "file_id": file_id}
+ for filename, file_id in file_list.items()],
+ }
+ else:
+ inventory = {"filelist": []}
+ self.getInventory = FakeMethod(result=inventory)
self.getBlob = FakeMethod(result=blob)
+ self.disable_memcache = disable_memcache
def _setUp(self):
self.useFixture(ZopeUtilityFixture(self, IBranchHostingClient))
+ if self.disable_memcache:
+ # Most tests that involve Branch.getBlob don't want to cache the
+ # result: doing so requires more time-consuming test setup and
+ # makes it awkward to repeat the same call with different
+ # responses. For convenience, we make it easy to disable that
+ # here.
+ self.memcache_fixture = self.useFixture(MemcacheFixture())
class GitHostingFixture(fixtures.Fixture):
=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py 2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/browser/snap.py 2018-05-17 14:25:53 +0000
@@ -23,7 +23,6 @@
copy_field,
use_template,
)
-import yaml
from zope.component import getUtility
from zope.error.interfaces import IErrorReportingUtility
from zope.interface import Interface
@@ -44,6 +43,7 @@
from lp.app.browser.lazrjs import InlinePersonEditPickerWidget
from lp.app.browser.tales import format_link
from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.app.errors import NotFoundError
from lp.app.interfaces.informationtype import IInformationType
from lp.app.widgets.itemswidgets import (
LabeledMultiCheckBoxWidget,
@@ -52,10 +52,6 @@
)
from lp.buildmaster.interfaces.processor import IProcessorSet
from lp.code.browser.widgets.gitref import GitRefWidget
-from lp.code.errors import (
- GitRepositoryBlobNotFound,
- GitRepositoryScanFault,
- )
from lp.code.interfaces.gitref import IGitRef
from lp.registry.enums import VCSType
from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -83,6 +79,8 @@
from lp.snappy.browser.widgets.storechannels import StoreChannelsWidget
from lp.snappy.interfaces.snap import (
CannotAuthorizeStoreUploads,
+ CannotFetchSnapcraftYaml,
+ CannotParseSnapcraftYaml,
ISnap,
ISnapSet,
NoSuchSnap,
@@ -424,34 +422,19 @@
@property
def initial_values(self):
store_name = None
- if self.has_snappy_distro_series and IGitRef.providedBy(self.context):
+ if self.has_snappy_distro_series:
# Try to extract Snap store name from snapcraft.yaml file.
try:
- paths = (
- 'snap/snapcraft.yaml',
- 'snapcraft.yaml',
- '.snapcraft.yaml',
- )
- for i, path in enumerate(paths):
- try:
- blob = self.context.repository.getBlob(
- path, self.context.name)
- break
- except GitRepositoryBlobNotFound:
- if i == len(paths) - 1:
- raise
- # Beware of unsafe yaml.load()!
- store_name = yaml.safe_load(blob).get('name')
- except GitRepositoryScanFault:
- log.exception("Failed to get Snap manifest from Git %s",
- self.context.unique_name)
- except (AttributeError, yaml.YAMLError):
- # Ignore parsing errors from invalid, user-supplied YAML
+ snapcraft_data = getUtility(ISnapSet).getSnapcraftYaml(
+ self.context, logger=log)
+ except (NotFoundError, CannotFetchSnapcraftYaml,
+ CannotParseSnapcraftYaml):
pass
- except Exception as e:
- log.exception(
- "Failed to extract name from Snap manifest at Git %s: %s",
- self.context.unique_name, unicode(e))
+ else:
+ try:
+ store_name = snapcraft_data.get('name')
+ except AttributeError:
+ pass
store_series = getUtility(ISnappySeriesSet).getAll().first()
if store_series.preferred_distro_series is not None:
=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py 2018-05-07 05:25:27 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py 2018-05-17 14:25:53 +0000
@@ -25,7 +25,6 @@
HTTMock,
)
from mechanize import LinkNotFoundError
-import mock
from pymacaroons import Macaroon
import pytz
import soupmatchers
@@ -43,10 +42,13 @@
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.interfaces.processor import IProcessorSet
from lp.code.errors import (
- GitRepositoryBlobNotFound,
+ BranchHostingFault,
GitRepositoryScanFault,
)
-from lp.code.tests.helpers import GitHostingFixture
+from lp.code.tests.helpers import (
+ BranchHostingFixture,
+ GitHostingFixture,
+ )
from lp.registry.enums import PersonVisibility
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
@@ -260,7 +262,7 @@
MatchesTagText(content, "store_upload"))
def test_create_new_snap_git(self):
- self.useFixture(GitHostingFixture(blob=""))
+ self.useFixture(GitHostingFixture(blob=b""))
[git_ref] = self.factory.makeGitRefs()
source_display = git_ref.display_name
browser = self.getViewBrowser(
@@ -508,74 +510,57 @@
self.assertContentEqual(
["386", "amd64"], [proc.name for proc in snap.processors])
- def test_initial_name_extraction_git_snap_snapcraft_yaml(self):
- def getBlob(filename, *args, **kwargs):
- if filename == "snap/snapcraft.yaml":
- return "name: test-snap"
- else:
- raise GitRepositoryBlobNotFound("dummy", filename)
-
- [git_ref] = self.factory.makeGitRefs()
- git_ref.repository.getBlob = getBlob
- view = create_initialized_view(git_ref, "+new-snap")
- initial_values = view.initial_values
- self.assertIn('store_name', initial_values)
- self.assertEqual('test-snap', initial_values['store_name'])
-
- def test_initial_name_extraction_git_plain_snapcraft_yaml(self):
- def getBlob(filename, *args, **kwargs):
- if filename == "snapcraft.yaml":
- return "name: test-snap"
- else:
- raise GitRepositoryBlobNotFound("dummy", filename)
-
- [git_ref] = self.factory.makeGitRefs()
- git_ref.repository.getBlob = getBlob
- view = create_initialized_view(git_ref, "+new-snap")
- initial_values = view.initial_values
- self.assertIn('store_name', initial_values)
- self.assertEqual('test-snap', initial_values['store_name'])
-
- def test_initial_name_extraction_git_dot_snapcraft_yaml(self):
- def getBlob(filename, *args, **kwargs):
- if filename == ".snapcraft.yaml":
- return "name: test-snap"
- else:
- raise GitRepositoryBlobNotFound("dummy", filename)
-
- [git_ref] = self.factory.makeGitRefs()
- git_ref.repository.getBlob = getBlob
- view = create_initialized_view(git_ref, "+new-snap")
- initial_values = view.initial_values
- self.assertIn('store_name', initial_values)
- self.assertEqual('test-snap', initial_values['store_name'])
-
- def test_initial_name_extraction_git_repo_error(self):
- [git_ref] = self.factory.makeGitRefs()
- git_ref.repository.getBlob = FakeMethod(failure=GitRepositoryScanFault)
- view = create_initialized_view(git_ref, "+new-snap")
- initial_values = view.initial_values
- self.assertIn('store_name', initial_values)
- self.assertIsNone(initial_values['store_name'])
-
- def test_initial_name_extraction_git_invalid_data(self):
- for invalid_result in (None, 123, '', '[][]', '#name:test', ']'):
- [git_ref] = self.factory.makeGitRefs()
- git_ref.repository.getBlob = FakeMethod(result=invalid_result)
- view = create_initialized_view(git_ref, "+new-snap")
- initial_values = view.initial_values
- self.assertIn('store_name', initial_values)
- self.assertIsNone(initial_values['store_name'])
-
- def test_initial_name_extraction_git_safe_yaml(self):
- [git_ref] = self.factory.makeGitRefs()
- git_ref.repository.getBlob = FakeMethod(result='Malicious YAML!')
- view = create_initialized_view(git_ref, "+new-snap")
- with mock.patch('yaml.load') as unsafe_load:
- with mock.patch('yaml.safe_load') as safe_load:
- view.initial_values
- self.assertEqual(0, unsafe_load.call_count)
- self.assertEqual(1, safe_load.call_count)
+ def test_initial_name_extraction_bzr_success(self):
+ self.useFixture(BranchHostingFixture(
+ file_list={"snapcraft.yaml": "file-id"}, blob=b"name: test-snap"))
+ branch = self.factory.makeBranch()
+ view = create_initialized_view(branch, "+new-snap")
+ initial_values = view.initial_values
+ self.assertIn('store_name', initial_values)
+ self.assertEqual('test-snap', initial_values['store_name'])
+
+ def test_initial_name_extraction_bzr_error(self):
+ self.useFixture(BranchHostingFixture()).getInventory = FakeMethod(
+ failure=BranchHostingFault)
+ branch = self.factory.makeBranch()
+ view = create_initialized_view(branch, "+new-snap")
+ initial_values = view.initial_values
+ self.assertIn('store_name', initial_values)
+ self.assertIsNone(initial_values['store_name'])
+
+ def test_initial_name_extraction_bzr_no_name(self):
+ self.useFixture(BranchHostingFixture(
+ file_list={"snapcraft.yaml": "file-id"}, blob=b"some: nonsense"))
+ branch = self.factory.makeBranch()
+ view = create_initialized_view(branch, "+new-snap")
+ initial_values = view.initial_values
+ self.assertIn('store_name', initial_values)
+ self.assertIsNone(initial_values['store_name'])
+
+ def test_initial_name_extraction_git_success(self):
+ self.useFixture(GitHostingFixture(blob=b"name: test-snap"))
+ [git_ref] = self.factory.makeGitRefs()
+ view = create_initialized_view(git_ref, "+new-snap")
+ initial_values = view.initial_values
+ self.assertIn('store_name', initial_values)
+ self.assertEqual('test-snap', initial_values['store_name'])
+
+ def test_initial_name_extraction_git_error(self):
+ self.useFixture(GitHostingFixture()).getBlob = FakeMethod(
+ failure=GitRepositoryScanFault)
+ [git_ref] = self.factory.makeGitRefs()
+ view = create_initialized_view(git_ref, "+new-snap")
+ initial_values = view.initial_values
+ self.assertIn('store_name', initial_values)
+ self.assertIsNone(initial_values['store_name'])
+
+ def test_initial_name_extraction_git_no_name(self):
+ self.useFixture(GitHostingFixture(blob=b"some: nonsense"))
+ [git_ref] = self.factory.makeGitRefs()
+ view = create_initialized_view(git_ref, "+new-snap")
+ initial_values = view.initial_values
+ self.assertIn('store_name', initial_values)
+ self.assertIsNone(initial_values['store_name'])
class TestSnapAdminView(BaseTestSnapView):
=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py 2018-05-07 05:25:27 +0000
+++ lib/lp/snappy/interfaces/snap.py 2018-05-17 14:25:53 +0000
@@ -9,7 +9,9 @@
'BadSnapSearchContext',
'BadSnapSource',
'CannotAuthorizeStoreUploads',
+ 'CannotFetchSnapcraftYaml',
'CannotModifySnapProcessor',
+ 'CannotParseSnapcraftYaml',
'CannotRequestAutoBuilds',
'DuplicateSnapName',
'ISnap',
@@ -237,6 +239,14 @@
"because %s is not set." % field)
+class CannotFetchSnapcraftYaml(Exception):
+ """Launchpad cannot fetch this snap package's snapcraft.yaml."""
+
+
+class CannotParseSnapcraftYaml(Exception):
+ """Launchpad cannot parse this snap package's snapcraft.yaml."""
+
+
class ISnapView(Interface):
"""`ISnap` attributes that require launchpad.View permission."""
@@ -787,6 +797,22 @@
def preloadDataForSnaps(snaps, user):
"""Load the data related to a list of snap packages."""
+ def getSnapcraftYaml(context, logger=None):
+ """Fetch a package's snapcraft.yaml from code hosting, if possible.
+
+ :param context: Either an `ISnap` or the source branch for a snap
+ package.
+ :param logger: An optional logger.
+
+ :return: The package's parsed snapcraft.yaml.
+ :raises NotFoundError: if this package has no snapcraft.yaml.
+ :raises CannotFetchSnapcraftYaml: if it was not possible to fetch
+ snapcraft.yaml from the code hosting backend for some other
+ reason.
+ :raises CannotParseSnapcraftYaml: if the fetched snapcraft.yaml
+ cannot be parsed.
+ """
+
def makeAutoBuilds(logger=None):
"""Create and return automatic builds for stale snap packages.
=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py 2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/model/snap.py 2018-05-17 14:25:53 +0000
@@ -32,6 +32,7 @@
Storm,
Unicode,
)
+import yaml
from zope.component import (
getAdapter,
getUtility,
@@ -42,7 +43,10 @@
from lp.app.browser.tales import DateTimeFormatterAPI
from lp.app.enums import PRIVATE_INFORMATION_TYPES
-from lp.app.errors import IncompatibleArguments
+from lp.app.errors import (
+ IncompatibleArguments,
+ NotFoundError,
+ )
from lp.app.interfaces.security import IAuthorization
from lp.buildmaster.enums import BuildStatus
from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
@@ -50,6 +54,12 @@
from lp.buildmaster.model.buildfarmjob import BuildFarmJob
from lp.buildmaster.model.buildqueue import BuildQueue
from lp.buildmaster.model.processor import Processor
+from lp.code.errors import (
+ BranchFileNotFound,
+ BranchHostingFault,
+ GitRepositoryBlobNotFound,
+ GitRepositoryScanFault,
+ )
from lp.code.interfaces.branch import IBranch
from lp.code.interfaces.branchcollection import (
IAllBranches,
@@ -110,7 +120,9 @@
BadSnapSearchContext,
BadSnapSource,
CannotAuthorizeStoreUploads,
+ CannotFetchSnapcraftYaml,
CannotModifySnapProcessor,
+ CannotParseSnapcraftYaml,
CannotRequestAutoBuilds,
DuplicateSnapName,
ISnap,
@@ -923,6 +935,52 @@
list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
person_ids, need_validity=True))
+ def getSnapcraftYaml(self, context, logger=None):
+ """See `ISnapSet`."""
+ if ISnap.providedBy(context):
+ context = context.source
+ try:
+ paths = (
+ "snap/snapcraft.yaml",
+ "snapcraft.yaml",
+ ".snapcraft.yaml",
+ )
+ for path in paths:
+ try:
+ if IBranch.providedBy(context):
+ blob = context.getBlob(path)
+ else:
+ blob = context.repository.getBlob(path, context.name)
+ break
+ except (BranchFileNotFound, GitRepositoryBlobNotFound):
+ pass
+ else:
+ msg = "Cannot find snapcraft.yaml in %s"
+ if logger is not None:
+ logger.exception(msg, context.unique_name)
+ raise NotFoundError(msg % context.unique_name)
+ except (BranchHostingFault, GitRepositoryScanFault) as e:
+ msg = "Failed to get snap manifest from %s"
+ if logger is not None:
+ logger.exception(msg, context.unique_name)
+ raise CannotFetchSnapcraftYaml(
+ "%s: %s" % (msg % context.unique_name, e))
+
+ try:
+ snapcraft_data = yaml.safe_load(blob)
+ except Exception as e:
+ # Don't bother logging parsing errors from user-supplied YAML.
+ raise CannotParseSnapcraftYaml(
+ "Cannot parse snapcraft.yaml from %s: %s" %
+ (context.unique_name, e))
+
+ if not isinstance(snapcraft_data, dict):
+ raise CannotParseSnapcraftYaml(
+ "The top level of snapcraft.yaml from %s is not a mapping" %
+ context.unique_name)
+
+ return snapcraft_data
+
@staticmethod
def _findStaleSnaps():
"""See `ISnapSet`."""
=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py 2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/tests/test_snap.py 2018-05-17 14:25:53 +0000
@@ -14,6 +14,7 @@
import json
from urlparse import urlsplit
+from fixtures import MockPatch
from httmock import (
all_requests,
HTTMock,
@@ -46,6 +47,16 @@
from lp.buildmaster.interfaces.processor import IProcessorSet
from lp.buildmaster.model.buildfarmjob import BuildFarmJob
from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.code.errors import (
+ BranchFileNotFound,
+ BranchHostingFault,
+ GitRepositoryBlobNotFound,
+ GitRepositoryScanFault,
+ )
+from lp.code.tests.helpers import (
+ BranchHostingFixture,
+ GitHostingFixture,
+ )
from lp.registry.enums import PersonVisibility
from lp.registry.interfaces.distribution import IDistributionSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
@@ -65,7 +76,9 @@
from lp.services.webapp.interfaces import OAuthPermission
from lp.snappy.interfaces.snap import (
BadSnapSearchContext,
+ CannotFetchSnapcraftYaml,
CannotModifySnapProcessor,
+ CannotParseSnapcraftYaml,
ISnap,
ISnapSet,
ISnapView,
@@ -978,6 +991,145 @@
[snaps[0], snaps[2], snaps[4], snaps[6]],
getUtility(ISnapSet).findByURLPrefixes(prefixes, owner=owners[0]))
+ def test_getSnapcraftYaml_bzr_snap_snapcraft_yaml(self):
+ def getInventory(unique_name, dirname, *args, **kwargs):
+ if dirname == "snap":
+ return {"filelist": [{
+ "filename": "snapcraft.yaml", "file_id": "some-file-id",
+ }]}
+ else:
+ raise BranchFileNotFound("dummy", dirname)
+
+ self.useFixture(BranchHostingFixture(
+ blob=b"name: test-snap")).getInventory = getInventory
+ branch = self.factory.makeBranch()
+ self.assertEqual(
+ {"name": "test-snap"},
+ getUtility(ISnapSet).getSnapcraftYaml(branch))
+
+ def test_getSnapcraftYaml_bzr_plain_snapcraft_yaml(self):
+ def getInventory(unique_name, dirname, *args, **kwargs):
+ if dirname == "":
+ return {"filelist": [{
+ "filename": "snapcraft.yaml", "file_id": "some-file-id",
+ }]}
+ else:
+ raise BranchFileNotFound("dummy", dirname)
+
+ self.useFixture(BranchHostingFixture(
+ blob=b"name: test-snap")).getInventory = getInventory
+ branch = self.factory.makeBranch()
+ self.assertEqual(
+ {"name": "test-snap"},
+ getUtility(ISnapSet).getSnapcraftYaml(branch))
+
+ def test_getSnapcraftYaml_bzr_dot_snapcraft_yaml(self):
+ def getInventory(unique_name, dirname, *args, **kwargs):
+ if dirname == "":
+ return {"filelist": [{
+ "filename": ".snapcraft.yaml", "file_id": "some-file-id",
+ }]}
+ else:
+ raise BranchFileNotFound("dummy", dirname)
+
+ self.useFixture(BranchHostingFixture(
+ blob=b"name: test-snap")).getInventory = getInventory
+ branch = self.factory.makeBranch()
+ self.assertEqual(
+ {"name": "test-snap"},
+ getUtility(ISnapSet).getSnapcraftYaml(branch))
+
+ def test_getSnapcraftYaml_bzr_error(self):
+ self.useFixture(BranchHostingFixture()).getInventory = FakeMethod(
+ failure=BranchHostingFault)
+ branch = self.factory.makeBranch()
+ self.assertRaises(
+ CannotFetchSnapcraftYaml,
+ getUtility(ISnapSet).getSnapcraftYaml, branch)
+
+ def test_getSnapcraftYaml_git_snap_snapcraft_yaml(self):
+ def getBlob(path, filename, *args, **kwargs):
+ if filename == "snap/snapcraft.yaml":
+ return b"name: test-snap"
+ else:
+ raise GitRepositoryBlobNotFound("dummy", filename)
+
+ self.useFixture(GitHostingFixture()).getBlob = getBlob
+ [git_ref] = self.factory.makeGitRefs()
+ self.assertEqual(
+ {"name": "test-snap"},
+ getUtility(ISnapSet).getSnapcraftYaml(git_ref))
+
+ def test_getSnapcraftYaml_git_plain_snapcraft_yaml(self):
+ def getBlob(path, filename, *args, **kwargs):
+ if filename == "snapcraft.yaml":
+ return b"name: test-snap"
+ else:
+ raise GitRepositoryBlobNotFound("dummy", filename)
+
+ self.useFixture(GitHostingFixture()).getBlob = getBlob
+ [git_ref] = self.factory.makeGitRefs()
+ self.assertEqual(
+ {"name": "test-snap"},
+ getUtility(ISnapSet).getSnapcraftYaml(git_ref))
+
+ def test_getSnapcraftYaml_git_dot_snapcraft_yaml(self):
+ def getBlob(path, filename, *args, **kwargs):
+ if filename == ".snapcraft.yaml":
+ return b"name: test-snap"
+ else:
+ raise GitRepositoryBlobNotFound("dummy", filename)
+
+ self.useFixture(GitHostingFixture()).getBlob = getBlob
+ [git_ref] = self.factory.makeGitRefs()
+ self.assertEqual(
+ {"name": "test-snap"},
+ getUtility(ISnapSet).getSnapcraftYaml(git_ref))
+
+ def test_getSnapcraftYaml_git_error(self):
+ self.useFixture(GitHostingFixture()).getBlob = FakeMethod(
+ failure=GitRepositoryScanFault)
+ [git_ref] = self.factory.makeGitRefs()
+ self.assertRaises(
+ CannotFetchSnapcraftYaml,
+ getUtility(ISnapSet).getSnapcraftYaml, git_ref)
+
+ def test_getSnapcraftYaml_snap_bzr(self):
+ self.useFixture(BranchHostingFixture(
+ file_list={"snapcraft.yaml": "some-file-id"},
+ blob=b"name: test-snap"))
+ branch = self.factory.makeBranch()
+ snap = self.factory.makeSnap(branch=branch)
+ self.assertEqual(
+ {"name": "test-snap"}, getUtility(ISnapSet).getSnapcraftYaml(snap))
+
+ def test_getSnapcraftYaml_snap_git(self):
+ self.useFixture(GitHostingFixture(blob=b"name: test-snap"))
+ [git_ref] = self.factory.makeGitRefs()
+ snap = self.factory.makeSnap(git_ref=git_ref)
+ self.assertEqual(
+ {"name": "test-snap"}, getUtility(ISnapSet).getSnapcraftYaml(snap))
+
+ def test_getSnapcraftYaml_invalid_data(self):
+ hosting_fixture = self.useFixture(GitHostingFixture())
+ for invalid_result in (None, 123, b"", b"[][]", b"#name:test", b"]"):
+ [git_ref] = self.factory.makeGitRefs()
+ hosting_fixture.getBlob = FakeMethod(result=invalid_result)
+ self.assertRaises(
+ CannotParseSnapcraftYaml,
+ getUtility(ISnapSet).getSnapcraftYaml, git_ref)
+
+ def test_getSnapcraftYaml_safe_yaml(self):
+ self.useFixture(GitHostingFixture(blob=b"Malicious YAML!"))
+ [git_ref] = self.factory.makeGitRefs()
+ unsafe_load = self.useFixture(MockPatch("yaml.load"))
+ safe_load = self.useFixture(MockPatch("yaml.safe_load"))
+ self.assertRaises(
+ CannotParseSnapcraftYaml,
+ getUtility(ISnapSet).getSnapcraftYaml, git_ref)
+ self.assertEqual(0, unsafe_load.mock.call_count)
+ self.assertEqual(1, safe_load.mock.call_count)
+
def test__findStaleSnaps(self):
# Stale; not built automatically.
self.factory.makeSnap(is_stale=True)
Follow ups
