Merge lp:~cjwatson/launchpad/git-ref-remote-blob into lp:launchpad

Proposed by Colin Watson on 2018-06-15
Status: Merged
Merged at revision: 18730
Proposed branch: lp:~cjwatson/launchpad/git-ref-remote-blob
Merge into: lp:launchpad
Prerequisite: lp:~cjwatson/launchpad/snap-request-builds
Diff against target: 499 lines (+249/-15)
9 files modified
lib/lp/code/errors.py (+13/-0)
lib/lp/code/interfaces/gitref.py (+8/-1)
lib/lp/code/model/githosting.py (+2/-1)
lib/lp/code/model/gitref.py (+62/-2)
lib/lp/code/model/gitrepository.py (+2/-1)
lib/lp/code/model/tests/test_gitref.py (+110/-2)
lib/lp/snappy/interfaces/snap.py (+4/-0)
lib/lp/snappy/model/snap.py (+15/-6)
lib/lp/snappy/tests/test_snap.py (+33/-2)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-ref-remote-blob
Reviewer Review Type Date Requested Status
William Grant code 2018-06-15 Approve on 2018-07-13
Review via email: mp+348089@code.launchpad.net

Commit message

Support fetching snapcraft.yaml from external repositories hosted on GitHub.

Description of the change

This is undeniably a hack, but it's pretty well-contained and it solves the problem at hand (making it viable for build.snapcraft.io to use Snap.requestBuilds) in a pretty lightweight way. A general solution would have many more moving parts.

To post a comment you must log in.
18677. By Colin Watson on 2018-06-18

Fall back to all supported architectures when requesting builds from an unsupported external repository.

18678. By Colin Watson on 2018-06-20

Make GitRefRemote.getBlob exception handling a bit more robust.

William Grant (wgrant) :
review: Approve (code)
18679. By Colin Watson on 2018-07-12

Document GitHub's behaviour with branch/tag ambiguity.

18680. By Colin Watson on 2018-07-12

Factor out a _fetch_blob_from_github function.

18681. By Colin Watson on 2018-07-12

Handle fetching blobs from git.launchpad.net by URL.

Colin Watson (cjwatson) :
William Grant (wgrant) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/code/errors.py'
2--- lib/lp/code/errors.py 2018-06-21 13:45:19 +0000
3+++ lib/lp/code/errors.py 2018-07-12 18:20:18 +0000
4@@ -36,6 +36,7 @@
5 'DiffNotFound',
6 'GitDefaultConflict',
7 'GitRepositoryBlobNotFound',
8+ 'GitRepositoryBlobUnsupportedRemote',
9 'GitRepositoryCreationException',
10 'GitRepositoryCreationFault',
11 'GitRepositoryCreationForbidden',
12@@ -453,6 +454,18 @@
13 return message
14
15
16+class GitRepositoryBlobUnsupportedRemote(Exception):
17+ """Raised when trying to fetch a blob from an unsupported remote host."""
18+
19+ def __init__(self, repository_url):
20+ super(GitRepositoryBlobUnsupportedRemote, self).__init__()
21+ self.repository_url = repository_url
22+
23+ def __str__(self):
24+ return "Cannot fetch blob from external Git repository at %s" % (
25+ self.repository_url)
26+
27+
28 class GitRepositoryDeletionFault(Exception):
29 """Raised when there is a fault deleting a repository."""
30
31
32=== modified file 'lib/lp/code/interfaces/gitref.py'
33--- lib/lp/code/interfaces/gitref.py 2017-11-06 09:32:45 +0000
34+++ lib/lp/code/interfaces/gitref.py 2018-07-12 18:20:18 +0000
35@@ -1,4 +1,4 @@
36-# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
37+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
38 # GNU Affero General Public License version 3 (see the file LICENSE).
39
40 """Git reference ("ref") interfaces."""
41@@ -382,6 +382,13 @@
42
43 has_commits = Attribute("Whether this reference has any commits.")
44
45+ def getBlob(filename):
46+ """Get a blob by file name from this reference.
47+
48+ :param filename: Relative path of a file in the repository.
49+ :return: A binary string with the blob content.
50+ """
51+
52
53 class IGitRefBatchNavigator(ITableBatchNavigator):
54 pass
55
56=== modified file 'lib/lp/code/model/githosting.py'
57--- lib/lp/code/model/githosting.py 2018-03-31 16:02:54 +0000
58+++ lib/lp/code/model/githosting.py 2018-07-12 18:20:18 +0000
59@@ -8,6 +8,7 @@
60 'GitHostingClient',
61 ]
62
63+import base64
64 import json
65 import sys
66 from urllib import quote
67@@ -223,7 +224,7 @@
68 raise GitRepositoryScanFault(
69 "Failed to get file from Git repository: %s" % unicode(e))
70 try:
71- blob = response["data"].decode("base64")
72+ blob = base64.b64decode(response["data"].encode("UTF-8"))
73 if len(blob) != response["size"]:
74 raise GitRepositoryScanFault(
75 "Unexpected size (%s vs %s)" % (
76
77=== modified file 'lib/lp/code/model/gitref.py'
78--- lib/lp/code/model/gitref.py 2017-11-06 09:32:45 +0000
79+++ lib/lp/code/model/gitref.py 2018-07-12 18:20:18 +0000
80@@ -1,4 +1,4 @@
81-# Copyright 2015-2017 Canonical Ltd. This software is licensed under the
82+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
83 # GNU Affero General Public License version 3 (see the file LICENSE).
84
85 __metaclass__ = type
86@@ -12,11 +12,16 @@
87 from datetime import datetime
88 from functools import partial
89 import json
90-from urllib import quote_plus
91+import re
92+from urllib import (
93+ quote,
94+ quote_plus,
95+ )
96 from urlparse import urlsplit
97
98 from lazr.lifecycle.event import ObjectCreatedEvent
99 import pytz
100+import requests
101 from storm.locals import (
102 DateTime,
103 Int,
104@@ -42,6 +47,9 @@
105 )
106 from lp.code.errors import (
107 BranchMergeProposalExists,
108+ GitRepositoryBlobNotFound,
109+ GitRepositoryBlobUnsupportedRemote,
110+ GitRepositoryScanFault,
111 InvalidBranchMergeProposal,
112 )
113 from lp.code.event.branchmergeproposal import (
114@@ -53,6 +61,7 @@
115 )
116 from lp.code.interfaces.gitcollection import IAllGitRepositories
117 from lp.code.interfaces.githosting import IGitHostingClient
118+from lp.code.interfaces.gitlookup import IGitLookup
119 from lp.code.interfaces.gitref import (
120 IGitRef,
121 IGitRefRemoteSet,
122@@ -69,6 +78,7 @@
123 from lp.services.database.stormbase import StormBase
124 from lp.services.features import getFeatureFlag
125 from lp.services.memcache.interfaces import IMemcacheClient
126+from lp.services.timeout import urlfetch
127 from lp.services.webapp.interfaces import ILaunchBag
128
129
130@@ -387,6 +397,10 @@
131 commit["sha1"])
132 return commits
133
134+ def getBlob(self, filename):
135+ """See `IGitRef`."""
136+ return self.repository.getBlob(filename, rev=self.path)
137+
138 @property
139 def recipes(self):
140 """See `IHasRecipes`."""
141@@ -634,6 +648,32 @@
142 return ref
143
144
145+def _fetch_blob_from_github(repository_url, ref_path, filename):
146+ repo_path = urlsplit(repository_url).path.strip("/")
147+ if repo_path.endswith(".git"):
148+ repo_path = repo_path[:len(".git")]
149+ try:
150+ response = urlfetch(
151+ "https://raw.githubusercontent.com/%s/%s/%s" % (
152+ repo_path,
153+ # GitHub supports either branch or tag names here, but both
154+ # must be shortened. (If both a branch and a tag exist with
155+ # the same name, it appears to pick the branch.)
156+ quote(re.sub(r"^refs/(?:heads|tags)/", "", ref_path)),
157+ quote(filename)),
158+ use_proxy=True)
159+ except requests.RequestException as e:
160+ if (e.response is not None and
161+ e.response.status_code == requests.codes.NOT_FOUND):
162+ raise GitRepositoryBlobNotFound(
163+ repository_url, filename, rev=ref_path)
164+ else:
165+ raise GitRepositoryScanFault(
166+ "Failed to get file from Git repository at %s: %s" %
167+ (repository_url, str(e)))
168+ return response.content
169+
170+
171 @implementer(IGitRef)
172 @provider(IGitRefRemoteSet)
173 class GitRefRemote(GitRefMixin):
174@@ -725,6 +765,26 @@
175 """See `IGitRef`."""
176 return []
177
178+ def getBlob(self, filename):
179+ """See `IGitRef`."""
180+ # In general, we can't easily get hold of a blob from a remote
181+ # repository without cloning the whole thing. In future we might
182+ # dispatch a build job or a code import or something like that to do
183+ # so. For now, we just special-case some providers where we know
184+ # how to fetch a blob on its own.
185+ repository = getUtility(IGitLookup).getByUrl(self.repository_url)
186+ if repository is not None:
187+ # This is one of our own repositories. Doing this by URL seems
188+ # gratuitously complex, but apparently we already have some
189+ # examples of this on production.
190+ return repository.getBlob(filename, rev=self.path)
191+ url = urlsplit(self.repository_url)
192+ if (url.hostname == "github.com" and
193+ len(url.path.strip("/").split("/")) == 2):
194+ return _fetch_blob_from_github(
195+ self.repository_url, self.path, filename)
196+ raise GitRepositoryBlobUnsupportedRemote(self.repository_url)
197+
198 @property
199 def recipes(self):
200 """See `IHasRecipes`."""
201
202=== modified file 'lib/lp/code/model/gitrepository.py'
203--- lib/lp/code/model/gitrepository.py 2018-07-10 11:26:57 +0000
204+++ lib/lp/code/model/gitrepository.py 2018-07-12 18:20:18 +0000
205@@ -1082,7 +1082,8 @@
206 def getBlob(self, filename, rev=None):
207 """See `IGitRepository`."""
208 hosting_client = getUtility(IGitHostingClient)
209- return hosting_client.getBlob(self.getInternalPath(), filename, rev)
210+ return hosting_client.getBlob(
211+ self.getInternalPath(), filename, rev=rev)
212
213 def getDiff(self, old, new):
214 """See `IGitRepository`."""
215
216=== modified file 'lib/lp/code/model/tests/test_gitref.py'
217--- lib/lp/code/model/tests/test_gitref.py 2018-07-10 11:27:47 +0000
218+++ lib/lp/code/model/tests/test_gitref.py 2018-07-12 18:20:18 +0000
219@@ -1,4 +1,4 @@
220-# Copyright 2015-2017 Canonical Ltd. This software is licensed under the
221+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
222 # GNU Affero General Public License version 3 (see the file LICENSE).
223
224 """Tests for Git references."""
225@@ -14,7 +14,9 @@
226 import hashlib
227 import json
228
229+from bzrlib import urlutils
230 import pytz
231+import responses
232 from testtools.matchers import (
233 ContainsDict,
234 EndsWith,
235@@ -29,8 +31,14 @@
236 from lp.app.enums import InformationType
237 from lp.app.interfaces.informationtype import IInformationType
238 from lp.app.interfaces.launchpad import IPrivacy
239-from lp.code.errors import InvalidBranchMergeProposal
240+from lp.code.errors import (
241+ GitRepositoryBlobNotFound,
242+ GitRepositoryBlobUnsupportedRemote,
243+ GitRepositoryScanFault,
244+ InvalidBranchMergeProposal,
245+ )
246 from lp.code.tests.helpers import GitHostingFixture
247+from lp.services.config import config
248 from lp.services.features.testing import FeatureFixture
249 from lp.services.memcache.interfaces import IMemcacheClient
250 from lp.services.webapp.interfaces import OAuthPermission
251@@ -271,6 +279,106 @@
252 ]))
253
254
255+class TestGitRefGetBlob(TestCaseWithFactory):
256+ """Tests for retrieving blobs from a Git reference."""
257+
258+ layer = LaunchpadFunctionalLayer
259+
260+ def test_local(self):
261+ [ref] = self.factory.makeGitRefs()
262+ hosting_fixture = self.useFixture(GitHostingFixture(blob=b"foo"))
263+ self.assertEqual(b"foo", ref.getBlob("dir/file"))
264+ self.assertEqual(
265+ [((ref.repository.getInternalPath(), "dir/file"),
266+ {"rev": ref.path})],
267+ hosting_fixture.getBlob.calls)
268+
269+ def test_local_by_url(self):
270+ # It's possible (though not encouraged) to retrieve files from
271+ # self-hosted repositories by URL.
272+ [ref] = self.factory.makeGitRefs()
273+ hosting_fixture = self.useFixture(GitHostingFixture(blob=b"foo"))
274+ https_ref = self.factory.makeGitRefRemote(
275+ repository_url=ref.repository.git_https_url, path=ref.path)
276+ anon_ref = self.factory.makeGitRefRemote(
277+ repository_url=urlutils.join(
278+ config.codehosting.git_anon_root,
279+ ref.repository.shortened_path),
280+ path=ref.path)
281+ self.assertEqual(b"foo", https_ref.getBlob("dir/file"))
282+ self.assertEqual(b"foo", anon_ref.getBlob("dir/file"))
283+ internal_path = ref.repository.getInternalPath()
284+ expected_calls = [
285+ ((internal_path, "dir/file"), {"rev": ref.path}),
286+ ((internal_path, "dir/file"), {"rev": ref.path}),
287+ ]
288+ self.assertEqual(expected_calls, hosting_fixture.getBlob.calls)
289+
290+ @responses.activate
291+ def test_remote_github_branch(self):
292+ ref = self.factory.makeGitRefRemote(
293+ repository_url="https://github.com/owner/name",
294+ path="refs/heads/path")
295+ responses.add(
296+ "GET",
297+ "https://raw.githubusercontent.com/owner/name/path/dir/file",
298+ body=b"foo")
299+ self.assertEqual(b"foo", ref.getBlob("dir/file"))
300+
301+ @responses.activate
302+ def test_remote_github_tag(self):
303+ ref = self.factory.makeGitRefRemote(
304+ repository_url="https://github.com/owner/name",
305+ path="refs/tags/1.0")
306+ responses.add(
307+ "GET",
308+ "https://raw.githubusercontent.com/owner/name/1.0/dir/file",
309+ body=b"foo")
310+ self.assertEqual(b"foo", ref.getBlob("dir/file"))
311+
312+ @responses.activate
313+ def test_remote_github_HEAD(self):
314+ ref = self.factory.makeGitRefRemote(
315+ repository_url="https://github.com/owner/name", path="HEAD")
316+ responses.add(
317+ "GET",
318+ "https://raw.githubusercontent.com/owner/name/HEAD/dir/file",
319+ body=b"foo")
320+ self.assertEqual(b"foo", ref.getBlob("dir/file"))
321+
322+ @responses.activate
323+ def test_remote_github_404(self):
324+ ref = self.factory.makeGitRefRemote(
325+ repository_url="https://github.com/owner/name", path="HEAD")
326+ responses.add(
327+ "GET",
328+ "https://raw.githubusercontent.com/owner/name/HEAD/dir/file",
329+ status=404)
330+ self.assertRaises(GitRepositoryBlobNotFound, ref.getBlob, "dir/file")
331+
332+ @responses.activate
333+ def test_remote_github_error(self):
334+ ref = self.factory.makeGitRefRemote(
335+ repository_url="https://github.com/owner/name", path="HEAD")
336+ responses.add(
337+ "GET",
338+ "https://raw.githubusercontent.com/owner/name/HEAD/dir/file",
339+ status=500)
340+ self.assertRaises(GitRepositoryScanFault, ref.getBlob, "dir/file")
341+
342+ def test_remote_github_bad_repository_path(self):
343+ ref = self.factory.makeGitRefRemote(
344+ repository_url="https://github.com/owner/name/extra")
345+ self.assertRaises(
346+ GitRepositoryBlobUnsupportedRemote, ref.getBlob, "dir/file")
347+
348+ def test_remote_unknown_host(self):
349+ ref = self.factory.makeGitRefRemote(
350+ repository_url="https://example.com/foo")
351+ self.assertRaises(
352+ GitRepositoryBlobUnsupportedRemote, ref.getBlob, "dir/file")
353+
354+
355 class TestGitRefCreateMergeProposal(TestCaseWithFactory):
356 """Exercise all the code paths for creating a merge proposal."""
357
358
359=== modified file 'lib/lp/snappy/interfaces/snap.py'
360--- lib/lp/snappy/interfaces/snap.py 2018-06-15 13:21:14 +0000
361+++ lib/lp/snappy/interfaces/snap.py 2018-07-12 18:20:18 +0000
362@@ -248,6 +248,10 @@
363 class CannotFetchSnapcraftYaml(Exception):
364 """Launchpad cannot fetch this snap package's snapcraft.yaml."""
365
366+ def __init__(self, message, unsupported_remote=False):
367+ super(CannotFetchSnapcraftYaml, self).__init__(message)
368+ self.unsupported_remote = unsupported_remote
369+
370
371 class CannotParseSnapcraftYaml(Exception):
372 """Launchpad cannot parse this snap package's snapcraft.yaml."""
373
374=== modified file 'lib/lp/snappy/model/snap.py'
375--- lib/lp/snappy/model/snap.py 2018-07-12 09:12:37 +0000
376+++ lib/lp/snappy/model/snap.py 2018-07-12 18:20:18 +0000
377@@ -60,6 +60,7 @@
378 BranchFileNotFound,
379 BranchHostingFault,
380 GitRepositoryBlobNotFound,
381+ GitRepositoryBlobUnsupportedRemote,
382 GitRepositoryScanFault,
383 )
384 from lp.code.interfaces.branch import IBranch
385@@ -553,8 +554,17 @@
386 def requestBuildsFromJob(self, requester, archive, pocket, channels=None,
387 logger=None):
388 """See `ISnap`."""
389- snapcraft_data = removeSecurityProxy(
390- getUtility(ISnapSet).getSnapcraftYaml(self))
391+ try:
392+ snapcraft_data = removeSecurityProxy(
393+ getUtility(ISnapSet).getSnapcraftYaml(self))
394+ except CannotFetchSnapcraftYaml as e:
395+ if not e.unsupported_remote:
396+ raise
397+ # The only reason we can't fetch the file is because we don't
398+ # support fetching from this repository's host. In this case
399+ # the best thing is to fall back to building for all supported
400+ # architectures.
401+ snapcraft_data = {}
402 # Sort by Processor.id for determinism. This is chosen to be the
403 # same order as in BinaryPackageBuildSet.createForSource, to
404 # minimise confusion.
405@@ -1037,10 +1047,7 @@
406 )
407 for path in paths:
408 try:
409- if IBranch.providedBy(context):
410- blob = context.getBlob(path)
411- else:
412- blob = context.repository.getBlob(path, context.name)
413+ blob = context.getBlob(path)
414 break
415 except (BranchFileNotFound, GitRepositoryBlobNotFound):
416 pass
417@@ -1049,6 +1056,8 @@
418 if logger is not None:
419 logger.exception(msg, context.unique_name)
420 raise NotFoundError(msg % context.unique_name)
421+ except GitRepositoryBlobUnsupportedRemote as e:
422+ raise CannotFetchSnapcraftYaml(str(e), unsupported_remote=True)
423 except (BranchHostingFault, GitRepositoryScanFault) as e:
424 msg = "Failed to get snap manifest from %s"
425 if logger is not None:
426
427=== modified file 'lib/lp/snappy/tests/test_snap.py'
428--- lib/lp/snappy/tests/test_snap.py 2018-06-15 13:21:14 +0000
429+++ lib/lp/snappy/tests/test_snap.py 2018-07-12 18:20:18 +0000
430@@ -74,6 +74,7 @@
431 from lp.services.job.runner import JobRunner
432 from lp.services.log.logger import BufferLogger
433 from lp.services.propertycache import clear_property_cache
434+from lp.services.timeout import default_timeout
435 from lp.services.webapp.interfaces import OAuthPermission
436 from lp.snappy.interfaces.snap import (
437 BadSnapSearchContext,
438@@ -391,7 +392,7 @@
439 pocket=Equals(PackagePublishingPocket.UPDATES),
440 channels=Equals({"snapcraft": "edge"})))
441
442- def makeRequestBuildsJob(self, arch_tags):
443+ def makeRequestBuildsJob(self, arch_tags, git_ref=None):
444 distro = self.factory.makeDistribution()
445 distroseries = self.factory.makeDistroSeries(distribution=distro)
446 processors = [
447@@ -404,7 +405,8 @@
448 processor=processor)
449 das.addOrUpdateChroot(self.factory.makeLibraryFileAlias(
450 filename="fake_chroot.tar.gz", db_only=True))
451- [git_ref] = self.factory.makeGitRefs()
452+ if git_ref is None:
453+ [git_ref] = self.factory.makeGitRefs()
454 snap = self.factory.makeSnap(
455 git_ref=git_ref, distroseries=distroseries, processors=processors)
456 return getUtility(ISnapRequestBuildsJobSource).create(
457@@ -450,6 +452,20 @@
458 removeSecurityProxy(job.channels))
459 self.assertRequestedBuildsMatch(builds, job, ["mips64el", "riscv64"])
460
461+ def test_requestBuilds_unsupported_remote(self):
462+ # If the snap is based on an external Git repository from which we
463+ # don't support fetching blobs, requestBuildsFromJob falls back to
464+ # requesting builds for all configured architectures.
465+ git_ref = self.factory.makeGitRefRemote(
466+ repository_url="https://example.com/foo.git")
467+ job = self.makeRequestBuildsJob(
468+ ["mips64el", "riscv64"], git_ref=git_ref)
469+ with person_logged_in(job.requester):
470+ builds = job.snap.requestBuildsFromJob(
471+ job.requester, job.archive, job.pocket,
472+ removeSecurityProxy(job.channels))
473+ self.assertRequestedBuildsMatch(builds, job, ["mips64el", "riscv64"])
474+
475 def test_requestAutoBuilds(self):
476 # requestAutoBuilds creates new builds for all configured
477 # architectures with appropriate parameters.
478@@ -1196,6 +1212,21 @@
479 self.assertEqual(
480 {"name": "test-snap"}, getUtility(ISnapSet).getSnapcraftYaml(snap))
481
482+ @responses.activate
483+ def test_getSnapcraftYaml_snap_git_external_github(self):
484+ responses.add(
485+ "GET",
486+ "https://raw.githubusercontent.com/owner/name/HEAD/"
487+ "snap/snapcraft.yaml",
488+ body=b"name: test-snap")
489+ git_ref = self.factory.makeGitRefRemote(
490+ repository_url="https://github.com/owner/name", path="HEAD")
491+ snap = self.factory.makeSnap(git_ref=git_ref)
492+ with default_timeout(1.0):
493+ self.assertEqual(
494+ {"name": "test-snap"},
495+ getUtility(ISnapSet).getSnapcraftYaml(snap))
496+
497 def test_getSnapcraftYaml_invalid_data(self):
498 hosting_fixture = self.useFixture(GitHostingFixture())
499 for invalid_result in (None, 123, b"", b"[][]", b"#name:test", b"]"):