launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === 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-06-20 16:04:57 +0000
> @@ -725,6 +738,40 @@
>          """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.
> +        url = urlsplit(self.repository_url)
> +        if (url.hostname == "github.com" and
> +                len(url.path.strip("/").split("/")) == 2):
> +            repo_path = 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.

Do we know what it does in the face of ambiguity?

> +                        quote(re.sub(r"^refs/(?:heads|tags)/", "", self.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(
> +                        self.repository_url, filename, rev=self.path)
> +                else:
> +                    raise GitRepositoryScanFault(
> +                        "Failed to get file from Git repository at %s: %s" %
> +                        (self.repository_url, str(e)))
> +            return response.content

Can you factor this out into a fetch_blob_from_github or similar?

> +        raise GitRepositoryBlobUnsupportedRemote(self.repository_url)
> +
>      @property
>      def recipes(self):
>          """See `IHasRecipes`."""
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2018-06-20 16:04:57 +0000
> +++ lib/lp/snappy/model/snap.py	2018-06-20 16:04:57 +0000
> @@ -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.

This is going to be expensive. We'll need to ensure that SnapArch is populated and behaves sanely.

> +            snapcraft_data = {}
>          # Sort by Processor.id for determinism.  This is chosen to be the
>          # same order as in BinaryPackageBuildSet.createForSource, to
>          # minimise confusion.
> 
> === modified file 'lib/lp/snappy/tests/test_snap.py'
> --- lib/lp/snappy/tests/test_snap.py	2018-06-20 16:04:57 +0000
> +++ lib/lp/snappy/tests/test_snap.py	2018-06-20 16:04:57 +0000
> @@ -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"])

Not toeing the Cambridge line, I see.

> +
>      def test_requestAutoBuilds(self):
>          # requestAutoBuilds creates new builds for all configured
>          # architectures with appropriate parameters.


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-ref-remote-blob/+merge/348089
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.