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

[Colin Watson]

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

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.

[William Grant (wgrant)]

review: Approve (code)

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py	2018-06-21 13:45:19 +0000
+++ lib/lp/code/errors.py	2018-07-12 18:20:18 +0000
@@ -36,6 +36,7 @@
     'DiffNotFound',
     'GitDefaultConflict',
     'GitRepositoryBlobNotFound',
+    'GitRepositoryBlobUnsupportedRemote',
     'GitRepositoryCreationException',
     'GitRepositoryCreationFault',
     'GitRepositoryCreationForbidden',
@@ -453,6 +454,18 @@
         return message
 
 
+class GitRepositoryBlobUnsupportedRemote(Exception):
+    """Raised when trying to fetch a blob from an unsupported remote host."""
+
+    def __init__(self, repository_url):
+        super(GitRepositoryBlobUnsupportedRemote, self).__init__()
+        self.repository_url = repository_url
+
+    def __str__(self):
+        return "Cannot fetch blob from external Git repository at %s" % (
+            self.repository_url)
+
+
 class GitRepositoryDeletionFault(Exception):
     """Raised when there is a fault deleting a repository."""
 

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2017-11-06 09:32:45 +0000
+++ lib/lp/code/interfaces/gitref.py	2018-07-12 18:20:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git reference ("ref") interfaces."""
@@ -382,6 +382,13 @@
 
     has_commits = Attribute("Whether this reference has any commits.")
 
+    def getBlob(filename):
+        """Get a blob by file name from this reference.
+
+        :param filename: Relative path of a file in the repository.
+        :return: A binary string with the blob content.
+        """
+
 
 class IGitRefBatchNavigator(ITableBatchNavigator):
     pass

=== modified file 'lib/lp/code/model/githosting.py'
--- lib/lp/code/model/githosting.py	2018-03-31 16:02:54 +0000
+++ lib/lp/code/model/githosting.py	2018-07-12 18:20:18 +0000
@@ -8,6 +8,7 @@
     'GitHostingClient',
     ]
 
+import base64
 import json
 import sys
 from urllib import quote
@@ -223,7 +224,7 @@
                 raise GitRepositoryScanFault(
                     "Failed to get file from Git repository: %s" % unicode(e))
         try:
-            blob = response["data"].decode("base64")
+            blob = base64.b64decode(response["data"].encode("UTF-8"))
             if len(blob) != response["size"]:
                 raise GitRepositoryScanFault(
                     "Unexpected size (%s vs %s)" % (

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2017-11-06 09:32:45 +0000
+++ lib/lp/code/model/gitref.py	2018-07-12 18:20:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -12,11 +12,16 @@
 from datetime import datetime
 from functools import partial
 import json
-from urllib import quote_plus
+import re
+from urllib import (
+    quote,
+    quote_plus,
+    )
 from urlparse import urlsplit
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 import pytz
+import requests
 from storm.locals import (
     DateTime,
     Int,
@@ -42,6 +47,9 @@
     )
 from lp.code.errors import (
     BranchMergeProposalExists,
+    GitRepositoryBlobNotFound,
+    GitRepositoryBlobUnsupportedRemote,
+    GitRepositoryScanFault,
     InvalidBranchMergeProposal,
     )
 from lp.code.event.branchmergeproposal import (
@@ -53,6 +61,7 @@
     )
 from lp.code.interfaces.gitcollection import IAllGitRepositories
 from lp.code.interfaces.githosting import IGitHostingClient
+from lp.code.interfaces.gitlookup import IGitLookup
 from lp.code.interfaces.gitref import (
     IGitRef,
     IGitRefRemoteSet,
@@ -69,6 +78,7 @@
 from lp.services.database.stormbase import StormBase
 from lp.services.features import getFeatureFlag
 from lp.services.memcache.interfaces import IMemcacheClient
+from lp.services.timeout import urlfetch
 from lp.services.webapp.interfaces import ILaunchBag
 
 
@@ -387,6 +397,10 @@
                     commit["sha1"])
         return commits
 
+    def getBlob(self, filename):
+        """See `IGitRef`."""
+        return self.repository.getBlob(filename, rev=self.path)
+
     @property
     def recipes(self):
         """See `IHasRecipes`."""
@@ -634,6 +648,32 @@
         return ref
 
 
+def _fetch_blob_from_github(repository_url, ref_path, filename):
+    repo_path = urlsplit(repository_url).path.strip("/")
+    if repo_path.endswith(".git"):
+        repo_path = repo_path[:len(".git")]
+    try:
+        response = urlfetch(
+            "https://raw.githubusercontent.com/%s/%s/%s" % (
+                repo_path,
+                # GitHub supports either branch or tag names here, but both
+                # must be shortened.  (If both a branch and a tag exist with
+                # the same name, it appears to pick the branch.)
+                quote(re.sub(r"^refs/(?:heads|tags)/", "", ref_path)),
+                quote(filename)),
+            use_proxy=True)
+    except requests.RequestException as e:
+        if (e.response is not None and
+                e.response.status_code == requests.codes.NOT_FOUND):
+            raise GitRepositoryBlobNotFound(
+                repository_url, filename, rev=ref_path)
+        else:
+            raise GitRepositoryScanFault(
+                "Failed to get file from Git repository at %s: %s" %
+                (repository_url, str(e)))
+    return response.content
+
+
 @implementer(IGitRef)
 @provider(IGitRefRemoteSet)
 class GitRefRemote(GitRefMixin):
@@ -725,6 +765,26 @@
         """See `IGitRef`."""
         return []
 
+    def getBlob(self, filename):
+        """See `IGitRef`."""
+        # In general, we can't easily get hold of a blob from a remote
+        # repository without cloning the whole thing.  In future we might
+        # dispatch a build job or a code import or something like that to do
+        # so.  For now, we just special-case some providers where we know
+        # how to fetch a blob on its own.
+        repository = getUtility(IGitLookup).getByUrl(self.repository_url)
+        if repository is not None:
+            # This is one of our own repositories.  Doing this by URL seems
+            # gratuitously complex, but apparently we already have some
+            # examples of this on production.
+            return repository.getBlob(filename, rev=self.path)
+        url = urlsplit(self.repository_url)
+        if (url.hostname == "github.com" and
+                len(url.path.strip("/").split("/")) == 2):
+            return _fetch_blob_from_github(
+                self.repository_url, self.path, filename)
+        raise GitRepositoryBlobUnsupportedRemote(self.repository_url)
+
     @property
     def recipes(self):
         """See `IHasRecipes`."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2018-07-10 11:26:57 +0000
+++ lib/lp/code/model/gitrepository.py	2018-07-12 18:20:18 +0000
@@ -1082,7 +1082,8 @@
     def getBlob(self, filename, rev=None):
         """See `IGitRepository`."""
         hosting_client = getUtility(IGitHostingClient)
-        return hosting_client.getBlob(self.getInternalPath(), filename, rev)
+        return hosting_client.getBlob(
+            self.getInternalPath(), filename, rev=rev)
 
     def getDiff(self, old, new):
         """See `IGitRepository`."""

=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py	2018-07-10 11:27:47 +0000
+++ lib/lp/code/model/tests/test_gitref.py	2018-07-12 18:20:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Git references."""
@@ -14,7 +14,9 @@
 import hashlib
 import json
 
+from bzrlib import urlutils
 import pytz
+import responses
 from testtools.matchers import (
     ContainsDict,
     EndsWith,
@@ -29,8 +31,14 @@
 from lp.app.enums import InformationType
 from lp.app.interfaces.informationtype import IInformationType
 from lp.app.interfaces.launchpad import IPrivacy
-from lp.code.errors import InvalidBranchMergeProposal
+from lp.code.errors import (
+    GitRepositoryBlobNotFound,
+    GitRepositoryBlobUnsupportedRemote,
+    GitRepositoryScanFault,
+    InvalidBranchMergeProposal,
+    )
 from lp.code.tests.helpers import GitHostingFixture
+from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
 from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.webapp.interfaces import OAuthPermission
@@ -271,6 +279,106 @@
             ]))
 
 
+class TestGitRefGetBlob(TestCaseWithFactory):
+    """Tests for retrieving blobs from a Git reference."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_local(self):
+        [ref] = self.factory.makeGitRefs()
+        hosting_fixture = self.useFixture(GitHostingFixture(blob=b"foo"))
+        self.assertEqual(b"foo", ref.getBlob("dir/file"))
+        self.assertEqual(
+            [((ref.repository.getInternalPath(), "dir/file"),
+              {"rev": ref.path})],
+            hosting_fixture.getBlob.calls)
+
+    def test_local_by_url(self):
+        # It's possible (though not encouraged) to retrieve files from
+        # self-hosted repositories by URL.
+        [ref] = self.factory.makeGitRefs()
+        hosting_fixture = self.useFixture(GitHostingFixture(blob=b"foo"))
+        https_ref = self.factory.makeGitRefRemote(
+            repository_url=ref.repository.git_https_url, path=ref.path)
+        anon_ref = self.factory.makeGitRefRemote(
+            repository_url=urlutils.join(
+                config.codehosting.git_anon_root,
+                ref.repository.shortened_path),
+            path=ref.path)
+        self.assertEqual(b"foo", https_ref.getBlob("dir/file"))
+        self.assertEqual(b"foo", anon_ref.getBlob("dir/file"))
+        internal_path = ref.repository.getInternalPath()
+        expected_calls = [
+            ((internal_path, "dir/file"), {"rev": ref.path}),
+            ((internal_path, "dir/file"), {"rev": ref.path}),
+            ]
+        self.assertEqual(expected_calls, hosting_fixture.getBlob.calls)
+
+    @responses.activate
+    def test_remote_github_branch(self):
+        ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name",
+            path="refs/heads/path")
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/path/dir/file",
+            body=b"foo")
+        self.assertEqual(b"foo", ref.getBlob("dir/file"))
+
+    @responses.activate
+    def test_remote_github_tag(self):
+        ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name",
+            path="refs/tags/1.0")
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/1.0/dir/file",
+            body=b"foo")
+        self.assertEqual(b"foo", ref.getBlob("dir/file"))
+
+    @responses.activate
+    def test_remote_github_HEAD(self):
+        ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name", path="HEAD")
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/dir/file",
+            body=b"foo")
+        self.assertEqual(b"foo", ref.getBlob("dir/file"))
+
+    @responses.activate
+    def test_remote_github_404(self):
+        ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name", path="HEAD")
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/dir/file",
+            status=404)
+        self.assertRaises(GitRepositoryBlobNotFound, ref.getBlob, "dir/file")
+
+    @responses.activate
+    def test_remote_github_error(self):
+        ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name", path="HEAD")
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/dir/file",
+            status=500)
+        self.assertRaises(GitRepositoryScanFault, ref.getBlob, "dir/file")
+
+    def test_remote_github_bad_repository_path(self):
+        ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name/extra")
+        self.assertRaises(
+            GitRepositoryBlobUnsupportedRemote, ref.getBlob, "dir/file")
+
+    def test_remote_unknown_host(self):
+        ref = self.factory.makeGitRefRemote(
+            repository_url="https://example.com/foo")
+        self.assertRaises(
+            GitRepositoryBlobUnsupportedRemote, ref.getBlob, "dir/file")
+
+
 class TestGitRefCreateMergeProposal(TestCaseWithFactory):
     """Exercise all the code paths for creating a merge proposal."""
 

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2018-06-15 13:21:14 +0000
+++ lib/lp/snappy/interfaces/snap.py	2018-07-12 18:20:18 +0000
@@ -248,6 +248,10 @@
 class CannotFetchSnapcraftYaml(Exception):
     """Launchpad cannot fetch this snap package's snapcraft.yaml."""
 
+    def __init__(self, message, unsupported_remote=False):
+        super(CannotFetchSnapcraftYaml, self).__init__(message)
+        self.unsupported_remote = unsupported_remote
+
 
 class CannotParseSnapcraftYaml(Exception):
     """Launchpad cannot parse this snap package's snapcraft.yaml."""

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2018-07-12 09:12:37 +0000
+++ lib/lp/snappy/model/snap.py	2018-07-12 18:20:18 +0000
@@ -60,6 +60,7 @@
     BranchFileNotFound,
     BranchHostingFault,
     GitRepositoryBlobNotFound,
+    GitRepositoryBlobUnsupportedRemote,
     GitRepositoryScanFault,
     )
 from lp.code.interfaces.branch import IBranch
@@ -553,8 +554,17 @@
     def requestBuildsFromJob(self, requester, archive, pocket, channels=None,
                              logger=None):
         """See `ISnap`."""
-        snapcraft_data = removeSecurityProxy(
-            getUtility(ISnapSet).getSnapcraftYaml(self))
+        try:
+            snapcraft_data = removeSecurityProxy(
+                getUtility(ISnapSet).getSnapcraftYaml(self))
+        except CannotFetchSnapcraftYaml as e:
+            if not e.unsupported_remote:
+                raise
+            # The only reason we can't fetch the file is because we don't
+            # support fetching from this repository's host.  In this case
+            # the best thing is to fall back to building for all supported
+            # architectures.
+            snapcraft_data = {}
         # Sort by Processor.id for determinism.  This is chosen to be the
         # same order as in BinaryPackageBuildSet.createForSource, to
         # minimise confusion.
@@ -1037,10 +1047,7 @@
                 )
             for path in paths:
                 try:
-                    if IBranch.providedBy(context):
-                        blob = context.getBlob(path)
-                    else:
-                        blob = context.repository.getBlob(path, context.name)
+                    blob = context.getBlob(path)
                     break
                 except (BranchFileNotFound, GitRepositoryBlobNotFound):
                     pass
@@ -1049,6 +1056,8 @@
                 if logger is not None:
                     logger.exception(msg, context.unique_name)
                 raise NotFoundError(msg % context.unique_name)
+        except GitRepositoryBlobUnsupportedRemote as e:
+            raise CannotFetchSnapcraftYaml(str(e), unsupported_remote=True)
         except (BranchHostingFault, GitRepositoryScanFault) as e:
             msg = "Failed to get snap manifest from %s"
             if logger is not None:

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2018-06-15 13:21:14 +0000
+++ lib/lp/snappy/tests/test_snap.py	2018-07-12 18:20:18 +0000
@@ -74,6 +74,7 @@
 from lp.services.job.runner import JobRunner
 from lp.services.log.logger import BufferLogger
 from lp.services.propertycache import clear_property_cache
+from lp.services.timeout import default_timeout
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.snappy.interfaces.snap import (
     BadSnapSearchContext,
@@ -391,7 +392,7 @@
             pocket=Equals(PackagePublishingPocket.UPDATES),
             channels=Equals({"snapcraft": "edge"})))
 
-    def makeRequestBuildsJob(self, arch_tags):
+    def makeRequestBuildsJob(self, arch_tags, git_ref=None):
         distro = self.factory.makeDistribution()
         distroseries = self.factory.makeDistroSeries(distribution=distro)
         processors = [
@@ -404,7 +405,8 @@
                 processor=processor)
             das.addOrUpdateChroot(self.factory.makeLibraryFileAlias(
                 filename="fake_chroot.tar.gz", db_only=True))
-        [git_ref] = self.factory.makeGitRefs()
+        if git_ref is None:
+            [git_ref] = self.factory.makeGitRefs()
         snap = self.factory.makeSnap(
             git_ref=git_ref, distroseries=distroseries, processors=processors)
         return getUtility(ISnapRequestBuildsJobSource).create(
@@ -450,6 +452,20 @@
                 removeSecurityProxy(job.channels))
         self.assertRequestedBuildsMatch(builds, job, ["mips64el", "riscv64"])
 
+    def test_requestBuilds_unsupported_remote(self):
+        # If the snap is based on an external Git repository from which we
+        # don't support fetching blobs, requestBuildsFromJob falls back to
+        # requesting builds for all configured architectures.
+        git_ref = self.factory.makeGitRefRemote(
+            repository_url="https://example.com/foo.git")
+        job = self.makeRequestBuildsJob(
+            ["mips64el", "riscv64"], git_ref=git_ref)
+        with person_logged_in(job.requester):
+            builds = job.snap.requestBuildsFromJob(
+                job.requester, job.archive, job.pocket,
+                removeSecurityProxy(job.channels))
+        self.assertRequestedBuildsMatch(builds, job, ["mips64el", "riscv64"])
+
     def test_requestAutoBuilds(self):
         # requestAutoBuilds creates new builds for all configured
         # architectures with appropriate parameters.
@@ -1196,6 +1212,21 @@
         self.assertEqual(
             {"name": "test-snap"}, getUtility(ISnapSet).getSnapcraftYaml(snap))
 
+    @responses.activate
+    def test_getSnapcraftYaml_snap_git_external_github(self):
+        responses.add(
+            "GET",
+            "https://raw.githubusercontent.com/owner/name/HEAD/"
+            "snap/snapcraft.yaml",
+            body=b"name: test-snap")
+        git_ref = self.factory.makeGitRefRemote(
+            repository_url="https://github.com/owner/name", path="HEAD")
+        snap = self.factory.makeSnap(git_ref=git_ref)
+        with default_timeout(1.0):
+            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"]"):