launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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.

It appears to pick the branch, which is probably best for us too.  I've expanded the comment to document that.

> +                        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

Done.

> +        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.

How so?

I checked the existing Snap rows on staging.  There were a number that have git_repository_url set to something on git.launchpad.net, which is weird but permitted, so I extended GitRefRemote.getBlob to handle that case too.  Other than that, there's exactly one row, which refers to gist.github.com and thus almost certainly doesn't work.  What population do you suggest that might help?

Also, I'm pretty sure that SnapArch is indeed populated:

  launchpad_staging=> SELECT COUNT(*) FROM snap LEFT JOIN snaparch ON snap.id = snaparch.snap WHERE snap.git_repository_url IS NOT NULL AND snaparch.snap IS NULL;
   count
  -------
       0
  (1 row)

> +            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"])

Indeed :-)

> +
>      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.