Merge lp:~maxiberta/launchpad/snap-name-extraction into lp:launchpad

Proposed by Maximiliano Bertacchini on 2016-05-18
Status: Merged
Merged at revision: 18076
Proposed branch: lp:~maxiberta/launchpad/snap-name-extraction
Merge into: lp:launchpad
Prerequisite: lp:~cjwatson/launchpad/snap-store-add-edit-views
Diff against target: 151 lines (+63/-1)
3 files modified
lib/lp/snappy/browser/snap.py (+23/-0)
lib/lp/snappy/browser/tests/test_snap.py (+39/-1)
setup.py (+1/-0)
To merge this branch: bzr merge lp:~maxiberta/launchpad/snap-name-extraction
Reviewer Review Type Date Requested Status
Celso Providelo (community) Approve on 2016-05-23
Colin Watson 2016-05-18 Approve on 2016-05-19
Review via email: mp+295114@code.launchpad.net

This proposal supersedes a proposal from 2016-05-17.

Commit message

Extract Snap.store_name from snapcraft.yaml (Git only).

Description of the change

Extract Snap.store_name from snapcraft.yaml (Git only).

To post a comment you must log in.
Maximiliano Bertacchini (maxiberta) wrote : Posted in a previous version of this proposal

Should mark https://code.launchpad.net/~cjwatson/launchpad/snap-store-add-edit-views/+merge/294524 as prerequisite? Will probably conflict (just a little); but I thought maybe this change is small enough to be merged directly to devel first.

Colin Watson (cjwatson) wrote : Posted in a previous version of this proposal

You should set a prerequisite branch if and only if you actually branched from the unmerged branch in question; otherwise you will get very confusing results.

However, you really will need to redo this on top of snap-store-add-edit-views, I'm afraid. The reason is that Snap.name is unique per person, but the same name in snapcraft.yaml may be associated with a snap that exists in multiple series. Defaulting name from what's in snapcraft.yaml will therefore fill in an initial form value that fails validation in some cases, and we shouldn't do that. We must only use this to pick a default for Snap.store_name, not Snap.name.

review: Resubmit
Colin Watson (cjwatson) :
review: Approve
Maximiliano Bertacchini (maxiberta) wrote :

Replied to inline comment. Thanks

Colin Watson (cjwatson) :
Celso Providelo (cprov) wrote :

Thanks Maxi,

We have discussed alternatives for the mock/call-counting strategy for testing the fact we are using yaml.safe_load(). Turns out that actually checking a malicious yaml content can't cause any harm was too involving for this test. So I am happy with the current state.

We also mentioned that, theoretically, we should be able to warn users in the form about repositories missing sufficient (only name for now) "snapcraft.yaml" content and prevent future errors. However, what we have right now is more friendly to initial setups (empty repo, for instance)

review: Approve
Colin Watson (cjwatson) :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/snappy/browser/snap.py'
2--- lib/lp/snappy/browser/snap.py 2016-05-27 16:04:55 +0000
3+++ lib/lp/snappy/browser/snap.py 2016-05-28 00:35:28 +0000
4@@ -25,6 +25,7 @@
5 use_template,
6 )
7 from pymacaroons import Macaroon
8+import yaml
9 from zope.component import getUtility
10 from zope.error.interfaces import IErrorReportingUtility
11 from zope.interface import Interface
12@@ -52,12 +53,14 @@
13 LaunchpadRadioWidget,
14 )
15 from lp.code.browser.widgets.gitref import GitRefWidget
16+from lp.code.errors import GitRepositoryScanFault
17 from lp.code.interfaces.gitref import IGitRef
18 from lp.registry.enums import VCSType
19 from lp.registry.interfaces.pocket import PackagePublishingPocket
20 from lp.services.features import getFeatureFlag
21 from lp.services.helpers import english_list
22 from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
23+from lp.services.scripts import log
24 from lp.services.webapp import (
25 canonical_url,
26 ContextMenu,
27@@ -372,12 +375,32 @@
28
29 @property
30 def initial_values(self):
31+ store_name = None
32+ if self.has_snappy_distro_series and IGitRef.providedBy(self.context):
33+ # Try to extract Snap store name from snapcraft.yaml file.
34+ try:
35+ blob = self.context.repository.getBlob(
36+ 'snapcraft.yaml', self.context.name)
37+ # Beware of unsafe yaml.load()!
38+ store_name = yaml.safe_load(blob).get('name')
39+ except GitRepositoryScanFault:
40+ log.exception("Failed to get Snap manifest from Git %s",
41+ self.context.unique_name)
42+ except (AttributeError, yaml.YAMLError):
43+ # Ignore parsing errors from invalid, user-supplied YAML
44+ pass
45+ except Exception as e:
46+ log.exception(
47+ "Failed to extract name from Snap manifest at Git %s: %s",
48+ self.context.unique_name, unicode(e))
49+
50 # XXX cjwatson 2015-09-18: Hack to ensure that we don't end up
51 # accidentally selecting ubuntu-rtm/14.09 or similar.
52 # ubuntu.currentseries will always be in BuildableDistroSeries.
53 series = getUtility(ILaunchpadCelebrities).ubuntu.currentseries
54 sds_set = getUtility(ISnappyDistroSeriesSet)
55 return {
56+ 'store_name': store_name,
57 'owner': self.user,
58 'store_distro_series': sds_set.getByDistroSeries(series).first(),
59 }
60
61=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
62--- lib/lp/snappy/browser/tests/test_snap.py 2016-05-27 22:05:53 +0000
63+++ lib/lp/snappy/browser/tests/test_snap.py 2016-05-28 00:35:28 +0000
64@@ -24,6 +24,7 @@
65 HTTMock,
66 )
67 from mechanize import LinkNotFoundError
68+import mock
69 from pymacaroons import Macaroon
70 import pytz
71 import soupmatchers
72@@ -40,6 +41,7 @@
73 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
74 from lp.buildmaster.enums import BuildStatus
75 from lp.buildmaster.interfaces.processor import IProcessorSet
76+from lp.code.errors import GitRepositoryScanFault
77 from lp.registry.enums import PersonVisibility
78 from lp.registry.interfaces.pocket import PackagePublishingPocket
79 from lp.registry.interfaces.series import SeriesStatus
80@@ -73,6 +75,7 @@
81 TestCaseWithFactory,
82 time_counter,
83 )
84+from lp.testing.fakemethod import FakeMethod
85 from lp.testing.layers import (
86 DatabaseFunctionalLayer,
87 LaunchpadFunctionalLayer,
88@@ -84,8 +87,8 @@
89 from lp.testing.pages import (
90 extract_text,
91 find_main_content,
92+ find_tag_by_id,
93 find_tags_by_class,
94- find_tag_by_id,
95 get_feedback_messages,
96 )
97 from lp.testing.publication import test_traverse
98@@ -360,6 +363,41 @@
99 }
100 self.assertEqual(expected_args, parse_qs(parsed_location[3]))
101
102+ def test_initial_name_extraction_git(self):
103+ [git_ref] = self.factory.makeGitRefs()
104+ git_ref.repository.getBlob = FakeMethod(result='name: test-snap')
105+ view = create_initialized_view(git_ref, "+new-snap")
106+ initial_values = view.initial_values
107+ self.assertIn('store_name', initial_values)
108+ self.assertEqual('test-snap', initial_values['store_name'])
109+
110+ def test_initial_name_extraction_git_repo_error(self):
111+ [git_ref] = self.factory.makeGitRefs()
112+ git_ref.repository.getBlob = FakeMethod(failure=GitRepositoryScanFault)
113+ view = create_initialized_view(git_ref, "+new-snap")
114+ initial_values = view.initial_values
115+ self.assertIn('store_name', initial_values)
116+ self.assertIsNone(initial_values['store_name'])
117+
118+ def test_initial_name_extraction_git_invalid_data(self):
119+ for invalid_result in (None, 123, '', '[][]', '#name:test', ']'):
120+ [git_ref] = self.factory.makeGitRefs()
121+ git_ref.repository.getBlob = FakeMethod(result=invalid_result)
122+ view = create_initialized_view(git_ref, "+new-snap")
123+ initial_values = view.initial_values
124+ self.assertIn('store_name', initial_values)
125+ self.assertIsNone(initial_values['store_name'])
126+
127+ def test_initial_name_extraction_git_safe_yaml(self):
128+ [git_ref] = self.factory.makeGitRefs()
129+ git_ref.repository.getBlob = FakeMethod(result='Malicious YAML!')
130+ view = create_initialized_view(git_ref, "+new-snap")
131+ with mock.patch('yaml.load') as unsafe_load:
132+ with mock.patch('yaml.safe_load') as safe_load:
133+ initial_values = view.initial_values
134+ self.assertEqual(0, unsafe_load.call_count)
135+ self.assertEqual(1, safe_load.call_count)
136+
137
138 class TestSnapAdminView(BrowserTestCase):
139
140
141=== modified file 'setup.py'
142--- setup.py 2016-05-11 11:23:41 +0000
143+++ setup.py 2016-05-28 00:35:28 +0000
144@@ -83,6 +83,7 @@
145 'python-memcached',
146 'python-openid',
147 'pytz',
148+ 'PyYAML',
149 'rabbitfixture',
150 'requests',
151 'requests-toolbelt',