Fix ref spec for default Git context

[crazy-max]

Fixes #336

cc. @hanazuki

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

[hanazuki]

@crazy-max Thank you for the implementation!
I tested it with the push and schedule events, and it worked as expected: it checks out GITHUB_SHA, for which the run is triggered instead of the tip of GITHU_REF.

For the pull_request, as intended by the code, the action checks out the merge commit of the base branch and the topic branch.
To explain this case to other users, let us assume we have a pull request that merges topic into master, and two commits A and B are pushed to topic (as described in the diagram).

   (master)
      v
X <-- Y
       \
        \
         A <-- B
               ^
            (topic)

A pull_request event is fired for the merge commit of the base branch and each commit pushed to the topic branch. In this case, they are A+Y and B+Y. But this implementation is designed to always check out the merge commit of the base branch and the tip of the topic branch, i.e., B+Y.

I suppose @crazy-max designed so because docker buildx build does not support checking out A+Y by specifying its commit SHA1. Since actions/checkout@v2 can check out A+Y directly, perhaps we can later relax this restriction by improving buildx, buildkit, or some other component (But I didn't investigate them further yet).

So, I think this patch implements the current best possible approach.

[crazy-max]

@hanazuki

I suppose @crazy-max designed so because docker buildx build does not support checking out A+Y by specifying its commit SHA1.

Yes indeed it's not possible atm to specify the commit because the current parser does not support it. Git URLs accept the context configuration in their fragment section, separated by a colon (:) which in our case would be interpreted as a subdir (not a refspec) and therefore would failed:

docker buildx build https://github.com/docker/rootfs.git#+6adaa44ee2074ea02066d5a9625394722131a1f3:refs/pull/6/merge

Same applies for classic docker build.

cc. @tonistiigi @thaJeztah

[thaJeztah]

@crazy-max is this a bug in how buildkit gets the reference? looking at the docker build docs, specifying myrepo.git#mybranch, should be equivalent to refs/heads/mybranch, correct?

Yes indeed it's not possible atm to specify the commit because the current parser does not support it

So it is possible to specify a commit, but I guess in this case the commit is not in the main repository? Otherwise
myrepo.git#<commit> should work

[crazy-max]

@thaJeztah

Not a bug actually but I think the parser could be enhanced to allow more complex refspec. Could be useful in our case to be able to check out A+Y as @hanazuki demonstrates.

[hanazuki]

I think that is not a bug or a lack of feature in the context URL parser, but a corner case in how the builder checks out the given reference

Here is a pull request with two commits pushed on top of the master branch: hanazuki/build-push-action-test2#1. The commits in my previous diagram correspond to these SHA1's in this case:

  A = f816db91ffa3a4a93829616040173ad8478ffc74 https://github.com/hanazuki/build-push-action-test2/runs/2427348592
A+Y = 02992a9474f4ce08274eb849428d4877bc45caca https://github.com/hanazuki/build-push-action-test2/runs/2427378367
  B = fac0dda8fdf991c6f12fa7b409e0773d27310c31 https://github.com/hanazuki/build-push-action-test2/runs/2427354980
B+Y = 8435bb2233da72790c6379702b3376e7b43a341b https://github.com/hanazuki/build-push-action-test2/runs/2427355087

For B+Y, both of the following commands succeed:

# ✔️ git fetch
git init testBY
cd testBY
git remote add origin https://github.com/hanazuki/build-push-action-test2.git
git fetch origin 8435bb2233da72790c6379702b3376e7b43a341b
git checkout FETCH_HEAD

# ✔️ docker buildx build
docker buildx build https://github.com/hanazuki/build-push-action-test2.git#8435bb2233da72790c6379702b3376e7b43a341b

But for A+Y, docker buildx build fails while git fetch succeeds:

# ✔️ git fetch
git init testAY
cd testAY
git remote add origin https://github.com/hanazuki/build-push-action-test2.git
git fetch origin 02992a9474f4ce08274eb849428d4877bc45caca
git checkout FETCH_HEAD

# ❌ docker buildx build
docker buildx build https://github.com/hanazuki/build-push-action-test2.git#02992a9474f4ce08274eb849428d4877bc45caca

[hanazuki]

Just noticed that plain (non-buildkit) docker build can check out A+Y by SHA1 in this case.

docker build https://github.com/hanazuki/build-push-action-test2.git#8435bb2233da72790c6379702b3376e7b43a341b
docker build https://github.com/hanazuki/build-push-action-test2.git#02992a9474f4ce08274eb849428d4877bc45caca

[hanazuki]

For record, my testing environment is a Debian sid box with the following pieces of software:

10:32 kasumi@dev% docker version
Client:
 Version:           20.10.5+dfsg1
 API version:       1.41
 Go version:        go1.15.9
 Git commit:        55c4c88
 Built:             Thu Apr  8 23:54:54 2021
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server:
 Engine:
  Version:          20.10.5+dfsg1
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.15.8
  Git commit:       363e9a8
  Built:            Tue Mar  9 16:53:21 2021
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.4.4~ds1
  GitCommit:        1.4.4~ds1-2
 runc:
  Version:          1.0.0~rc93+ds1
  GitCommit:        1.0.0~rc93+ds1-3
 docker-init:
  Version:          0.19.0
  GitCommit:
10:32 kasumi@dev% docker buildx version
github.com/docker/buildx v0.5.1 11057da37336192bfc57d81e02359ba7ba848e4a
10:32 kasumi@dev% docker buildx inspect default
Name:   default
Driver: docker

Nodes:
Name:      default
Endpoint:  default
Status:    running
Platforms: linux/amd64, linux/arm64, linux/riscv64, linux/ppc64le, linux/386, linux/arm/v7, linux/arm/v6, linux/s390x
10:32 kasumi@dev% git --version
git version 2.31.1

[crazy-max]

@hanazuki

I think that is not a bug or a lack of feature in the context URL parser, but a corner case in how the builder checks out the given reference

Yes that's right, good point!

Just noticed that plain (non-buildkit) docker build can check out A+Y by SHA1 in this case.

Just tested myself and it works for me too.

[crazy-max]

@hanazuki

Since actions/checkout@v2 can check out A+Y directly, perhaps we can later relax this restriction by improving buildx, buildkit, or some other component (But I didn't investigate them further yet).

This might be linked to this code in buildkit. WDYT @tonistiigi?