Support GitHub Pull Request refspec #1658
Conversation
gps/vcs_repo.go
Outdated
| cmd := commandContext( | ||
| ctx, | ||
| "git", | ||
| "fetch", | ||
| "--tags", | ||
| "--prune", | ||
| r.RemoteLocation, | ||
| fmt.Sprintf("+refs/pull/*:refs/pull/%x/*", h.Sum(nil)), |
There was a problem hiding this comment.
If refs/pull doesn't exist, because the git provider is not github, will this hurt anything? Or just be ignored?
There was a problem hiding this comment.
Hi @carolynvs,
I've no clue. I didn't test it and didn't want to in case it didn't work :)
However, I will test this and update the PR if necessary.
There was a problem hiding this comment.
Hi @carolynvs,
Good news everyone!
$ git fetch -v --progress origin +refs/notreal/*:refs/notreal/origin/* && echo $?
0It appears if the ref structure does not exist, Git simply ignores it.
There was a problem hiding this comment.
Hi @carolynvs,
Just to prove it, here it is again with the pull ref spec:
$ git fetch -v --progress origin +refs/pull/*:refs/pull/origin/* && echo $?
POST git-upload-pack (gzip 4377 to 2270 bytes)
remote: Counting objects: 131, done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 131 (delta 76), reused 76 (delta 76), pack-reused 52
Receiving objects: 100% (131/131), 49.20 KiB | 0 bytes/s, done.
Resolving deltas: 100% (77/77), completed with 66 local objects.
From https://github.com/thecodeteam/gocsi
* [new ref] refs/pull/10/head -> refs/pull/origin/10/head
* [new ref] refs/pull/10/merge -> refs/pull/origin/10/merge
* [new ref] refs/pull/11/head -> refs/pull/origin/11/head
* [new ref] refs/pull/12/head -> refs/pull/origin/12/head
* [new ref] refs/pull/13/head -> refs/pull/origin/13/head
* [new ref] refs/pull/14/head -> refs/pull/origin/14/head
* [new ref] refs/pull/15/head -> refs/pull/origin/15/head
* [new ref] refs/pull/16/head -> refs/pull/origin/16/head
* [new ref] refs/pull/17/head -> refs/pull/origin/17/head
* [new ref] refs/pull/18/head -> refs/pull/origin/18/head
* [new ref] refs/pull/19/head -> refs/pull/origin/19/head
* [new ref] refs/pull/2/head -> refs/pull/origin/2/head
* [new ref] refs/pull/20/head -> refs/pull/origin/20/head
* [new ref] refs/pull/20/merge -> refs/pull/origin/20/merge
* [new ref] refs/pull/21/head -> refs/pull/origin/21/head
* [new ref] refs/pull/23/head -> refs/pull/origin/23/head
* [new ref] refs/pull/23/merge -> refs/pull/origin/23/merge
* [new ref] refs/pull/27/head -> refs/pull/origin/27/head
* [new ref] refs/pull/28/head -> refs/pull/origin/28/head
* [new ref] refs/pull/29/head -> refs/pull/origin/29/head
* [new ref] refs/pull/3/head -> refs/pull/origin/3/head
* [new ref] refs/pull/30/head -> refs/pull/origin/30/head
* [new ref] refs/pull/31/head -> refs/pull/origin/31/head
* [new ref] refs/pull/32/head -> refs/pull/origin/32/head
* [new ref] refs/pull/35/head -> refs/pull/origin/35/head
* [new ref] refs/pull/36/head -> refs/pull/origin/36/head
* [new ref] refs/pull/37/head -> refs/pull/origin/37/head
* [new ref] refs/pull/38/head -> refs/pull/origin/38/head
* [new ref] refs/pull/4/head -> refs/pull/origin/4/head
* [new ref] refs/pull/40/head -> refs/pull/origin/40/head
* [new ref] refs/pull/40/merge -> refs/pull/origin/40/merge
* [new ref] refs/pull/41/head -> refs/pull/origin/41/head
* [new ref] refs/pull/42/head -> refs/pull/origin/42/head
* [new ref] refs/pull/43/head -> refs/pull/origin/43/head
* [new ref] refs/pull/44/head -> refs/pull/origin/44/head
* [new ref] refs/pull/45/head -> refs/pull/origin/45/head
* [new ref] refs/pull/46/head -> refs/pull/origin/46/head
* [new ref] refs/pull/47/head -> refs/pull/origin/47/head
* [new ref] refs/pull/48/head -> refs/pull/origin/48/head
* [new ref] refs/pull/49/head -> refs/pull/origin/49/head
* [new ref] refs/pull/5/head -> refs/pull/origin/5/head
* [new ref] refs/pull/50/head -> refs/pull/origin/50/head
* [new ref] refs/pull/51/head -> refs/pull/origin/51/head
* [new ref] refs/pull/52/head -> refs/pull/origin/52/head
* [new ref] refs/pull/53/head -> refs/pull/origin/53/head
* [new ref] refs/pull/54/head -> refs/pull/origin/54/head
* [new ref] refs/pull/55/head -> refs/pull/origin/55/head
* [new ref] refs/pull/56/head -> refs/pull/origin/56/head
* [new ref] refs/pull/57/head -> refs/pull/origin/57/head
* [new ref] refs/pull/57/merge -> refs/pull/origin/57/merge
* [new ref] refs/pull/58/head -> refs/pull/origin/58/head
* [new ref] refs/pull/59/head -> refs/pull/origin/59/head
* [new ref] refs/pull/59/merge -> refs/pull/origin/59/merge
* [new ref] refs/pull/6/head -> refs/pull/origin/6/head
* [new ref] refs/pull/65/head -> refs/pull/origin/65/head
* [new ref] refs/pull/66/head -> refs/pull/origin/66/head
* [new ref] refs/pull/67/head -> refs/pull/origin/67/head
* [new ref] refs/pull/68/head -> refs/pull/origin/68/head
* [new ref] refs/pull/69/head -> refs/pull/origin/69/head
* [new ref] refs/pull/7/head -> refs/pull/origin/7/head
* [new ref] refs/pull/70/head -> refs/pull/origin/70/head
* [new ref] refs/pull/71/head -> refs/pull/origin/71/head
* [new ref] refs/pull/71/merge -> refs/pull/origin/71/merge
* [new ref] refs/pull/72/head -> refs/pull/origin/72/head
* [new ref] refs/pull/73/head -> refs/pull/origin/73/head
* [new ref] refs/pull/74/head -> refs/pull/origin/74/head
* [new ref] refs/pull/74/merge -> refs/pull/origin/74/merge
* [new ref] refs/pull/75/head -> refs/pull/origin/75/head
* [new ref] refs/pull/76/head -> refs/pull/origin/76/head
* [new ref] refs/pull/77/head -> refs/pull/origin/77/head
* [new ref] refs/pull/78/head -> refs/pull/origin/78/head
* [new ref] refs/pull/79/head -> refs/pull/origin/79/head
* [new ref] refs/pull/8/head -> refs/pull/origin/8/head
* [new ref] refs/pull/80/head -> refs/pull/origin/80/head
* [new ref] refs/pull/81/head -> refs/pull/origin/81/head
* [new ref] refs/pull/82/head -> refs/pull/origin/82/head
* [new ref] refs/pull/83/head -> refs/pull/origin/83/head
* [new ref] refs/pull/85/head -> refs/pull/origin/85/head
* [new ref] refs/pull/9/head -> refs/pull/origin/9/head
0There was a problem hiding this comment.
Thanks for trying that out!
I have a bug right now that needs this, I'll give it a go and report back.
There was a problem hiding this comment.
@akutz I'm not quite sure what I should be putting into my Gopkg.toml in order to say that I want a pull request. Is it "pull/origin/X/head"? Some hash?
There was a problem hiding this comment.
Hi @carolynvs,
I'm blocked on this as well.
I'm not quite sure what I should be putting into my Gopkg.toml in order to say that I want a pull request. Is it "pull/origin/X/head"? Some hash?
The source is the URI of the repo to which the PR was submitted. Then you just set revision = "COMMIT_ID" like any other commit revision. You can figure out the commit ID of a PR commit by looking at the Travis-CI build log or just clone your PR locally and find the head commit ID: git fetch +refs/pull/*:refs/pull/REMOTE_NAME/* and then do a reflog on that ref spec.
There was a problem hiding this comment.
Hi @carolynvs,
See rexray/gocsi#71 for my PR that uses this and this part of the script that uses those variables:
# Refer to https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md
# for detailed Gopkg.toml documentation.
#
# Refer to https://github.com/toml-lang/toml for detailed TOML docs.
[[constraint]]
name = "github.com/container-storage-interface/spec"
branch = "master"
[[constraint]]
name = "github.com/thecodeteam/gocsi"
EOF
if [ "$GOCSI_DEP_BRANCH" != "" ]; then
echo " branch = \"$GOCSI_DEP_BRANCH\"" >> Gopkg.toml
elif [ "$GOCSI_DEP_REVISION" != "" ]; then
echo " revision = \"$GOCSI_DEP_REVISION\"" >> Gopkg.toml
elif [ "$GOCSI_DEP_VERSION" != "" ]; then
echo " version = \"$GOCSI_DEP_VERSION\"" >> Gopkg.toml
else
echo " branch = \"master\"" >> Gopkg.toml
fi
if [ "$GOCSI_DEP_SOURCE" != "" ]; then
echo " source = \"$GOCSI_DEP_SOURCE\"" >> Gopkg.toml
fi
fiThere was a problem hiding this comment.
Hi @carolynvs,
Is it "pull/origin/X/head"?
That's an issue. @sdboyer or someone made it such that the source field is pattern matched to match a Go package. You cannot put anything interesting as source other than (https?://)?[\w\d\.]+ (or something like that, maybe SSH/git works, I haven't tried). It does exclude file URIs, non-standard URIs, etc. It's frustrating. See #1581 and #1583.
|
Is it really a good idea to add dependencies on a specific commit that doesn't exist in the actual repo and is a moving target (i.e., will change on every push to |
|
Perhaps. Perhaps not. Either way it’s not a specific commit as a PR can have multiple commits. Also, it’s not changes to master but changes to the PR or changes to the branch that’s the target of the PR (usually master). All that said, it’s a no-op on failure, fails fast, and works. I’m open to a better solution. |
|
I wonder if it would be feasible to treat these like branches, rather than revisions, and find the latest commit and put that into the |
|
Oh there is definitely a cleaner way to do this with much better understanding of dep and additional code. This was straight forward and simple. |
|
By the way, I don’t know that this isn’t a good solution. It’s fetching one or more branches in a non standard ref namespace. Currently dep fetches all branches in the standard ref namespace. All this patch does is ensure dep also fetches the branches that include PR commits. After some additional thought I actually stand by this patch as it is consistent with existing logic and simply adds a new refspec to the fetch command, which, as I stated, already implicitly fetches all of “+refs/HEADS/:refs/HEADS/origin/”, the default refspec. |
|
So @jmank88, You could treat these as branches, but you’d still need to fetch the PR refspec. I suggest y’all read up on refspecs in the linked issue. I linked that to the SCM topic on Git refspecs. Unless you fetch the “pull” refspec from GH, you cannot reference any of its refs, as branches or revisions or otherwise. |
|
@akutz I am having trouble telling if you see a way to enable putting something other than the exact revision in the manifest? It's not a blocker, just curious if we can make this even easier to use. FWIW, I have no issue with the extra refspec, and do see value in referencing a pull request. Here is an example of why I need it: In the Kubernetes Service Catalog project we wanted to validate in CI that when a downstream consumer of our library uses dep to vendor our client library that it will work. Just two days ago, this test proactively found a breaking change in another library that let us know we needed to tweak our manifest to keep things working smoothly for consumers. It's really hard to test however, because in the PR where I fixed the manifest, when I run that test, dep is still hitting the master branch, not the changes in the PR. I want to be able to update the test manifest during a CI run to use the pull request instead. |
|
Hi @carolynvs,
The issue is that the revision will change every time you push to or rebase the PR. Unless you're dynamically generating the As I stated earlier, refspecs are a part of Git (as I'm sure all three of you are aware). All this patch does is add a new refspec to the following fetch command: $ git fetch --tags --prune REMOTE_LOCATION \
+refs/HEADS/*:refs/HEADS/origin/* \
+refs/tags/*:refs/tags/origin/*Oh wait, that's the not the command $ cd /tmp
$ git clone https://github.com/golang/dep
Cloning into 'dep'...
remote: Counting objects: 18644, done.
remote: Compressing objects: 100% (38/38), done.
remote: Total 18644 (delta 5), reused 13 (delta 0), pack-reused 18603
Receiving objects: 100% (18644/18644), 9.25 MiB | 7.94 MiB/s, done.
Resolving deltas: 100% (11031/11031), done.
$ cd dep
$ git fetch --tags --prune
$ git show-ref --head
063d03889f6be8391be87143b907e1e63c461191 HEAD
063d03889f6be8391be87143b907e1e63c461191 refs/heads/master
063d03889f6be8391be87143b907e1e63c461191 refs/remotes/origin/HEAD
11b12653823b27304d7e860bd8597e094af4c031 refs/remotes/origin/changelog-v0.3.3
000c56ce6a6f6a7fb0befc041e2089f31319c37e refs/remotes/origin/cleanupFSTests
bf2afd9031d1b670bb22011cbe430554ce10559f refs/remotes/origin/gh-pages
e1f5c5f69d41d1a6d86732970cb415695f41654e refs/remotes/origin/import-during-solve
063d03889f6be8391be87143b907e1e63c461191 refs/remotes/origin/master
142735b5947d4948766edce0a3522639c23b4b67 refs/remotes/origin/prune-release-prep
05c40eba7fa5512c3a161e4e9df6c8fefde75158 refs/tags/v0.1.0
6a17782ed25ba4ccd2adf191990cc32e65c3934c refs/tags/v0.2.0
44a454bac09f6239aaf9350c00dfb61bf3257711 refs/tags/v0.2.1
7a91b794bbfbf1f3b8b79823799316451127801b refs/tags/v0.3.0
83789e236d7ff64c82ee8392005455fc1ec1983b refs/tags/v0.3.1
8ddfc8afb2d520d41997ebddd921b52152706c01 refs/tags/v0.3.2
6d95d0d6ac152023bf56b7293f5cae3792936c98 refs/tags/v0.4.0
37d9ea0ac16f0e0a05afc3b60e1ac8c364b6c329 refs/tags/v0.4.1You can see that after cloning $ git fetch origin +refs/pull/*:refs/pull/origin/*
$ git show-ref --head
063d03889f6be8391be87143b907e1e63c461191 HEAD
063d03889f6be8391be87143b907e1e63c461191 refs/heads/master
f7999207706e81f490561d86ec2331cbe939e664 refs/pull/origin/1/head
bbc601799e1d8808a4c705986f07d8e17567da05 refs/pull/origin/1000/head
7571fe60f3ebf2ec18c54db3ed1e595013e91463 refs/pull/origin/1000/merge
b90c0e90746ea1b2e82c0f76855407768fd19deb refs/pull/origin/1004/head
eb40858a46ef95703cf7797027c543e10c982c61 refs/pull/origin/1004/merge
814ee6bde244d3e0eadb9b7c8e8559f591141d11 refs/pull/origin/1005/head
394b995c4d12409c1ab7d524d430541e36ff535c refs/pull/origin/1009/head
7e35fb8f96868b53504450da2d8dea904c44dae5 refs/pull/origin/1012/head
...As you can see, all this patch does is introduce a new, explicit refspec to |
|
Hi @carolynvs, All, I see that I didn't answer your question but rather I read your question as an extension of the earlier objections/feedback. My apologies. I can do what you asked like this: However, that is obviously not desirable. I tried the following as well: $ git checkout refs/pull/origin/1327/head
Note: checking out 'refs/pull/origin/1327/head'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:
git checkout -b <new-branch-name>
HEAD is now at e14275ae... renameByCopy: retry os.RemoveAllSo if you set the branch to the explicit refspec you're able to checkout in a detached state. |
|
Hi @carolynvs, My guess is that unless |
That is what I planned to do. 😀 In a PR change the manifest to reference the current PR and then run dep to test changes made to the manifest introduced in that PR. I haven't had time to try your patch yet, but using just the revision isn't a burden for me in this case. I was just asking about a different syntax out of curiosity and figure out what's possible. I totally get how refspec works and what's going down under the hood, and am not trying to get you to defend the PR. Apologies if I came off that way! I should have time to try out this patch today. |
|
Hi @carolynvs,
I just misread your comment due to earlier feedback. Which was valid feedback too by the way! Hopefully I am correct about refspecs, in which case maybe I was able to shed some light on the reason I created the patch this way. If I'm off base and misunderstand things, which is likely the case, then hopefully yourself or others can help me learn a cleaner way to solve this issue. I'm fine with whatever -- I really just want to achieve the result -- I care not so much how we get there :) |
|
Why not just set the constraint to point to your pull request branch directly, potentially with source pointing to your fork if it’s not the same repo?
|
|
Hi @calmh, Because the PR branch isn't what is being built. The commits in the |
|
Hi @calmh, The Travis-CI environment variable reference illustrates the different commits and sources involved in a PR build. For more on Github PRs and the |
|
Hi @carolynvs,
FWIW, I have tested this patch locally and it works as advertised. Depending on the momentum of this PR I may even update the Travis config for the affected project so I can build my forked version of |
I don't control our release cadence, so I recommend that you do just this. Don't wait for our next tagged release. But yes, you are right I want to see this get merged so I can use it too. So I'm pushing to validate and merge quickly. 😇 |
|
Hi @calmh, By the way:
Aside from my comment above regarding what a PR commit actually represents, I also tried doing something that was supported, and still is, in Glide: specifying a |
|
Hi @carolynvs, By the way, I hope we're not letting our desire to see this functionality folded into Perhaps an environment variable is defined to indicate a PR build is occurring and |
|
Hi @carolynvs, So I updated my project to use the custom dep, and the build is encountering an error when referencing the PR revision: Solving failure: No versions of github.com/thecodeteam/gocsi met constraints:
5276bd5bc31c832348a08202263d34644b32047c: unable to update checked out version: fatal: reference is not a tree: 5276bd5bc31c832348a08202263d34644b32047c
: command failed: [git checkout 5276bd5bc31c832348a08202263d34644b32047c]: exit status 128Could this error is related to #216? I know the equivalent command succeeds locally. I'm attempting to do a similar |
|
Hi @carolynvs, I figured it out. The issue is with the way it currently prunes upon fetch. Since there is no corresponding remote tracking branch for the PR refs, they are dropped upon fetch. When I comment out the |
This patch introduces support for settings a dep constraint's revision to a commit that exists in a GitHub pull request. Issue golang#1583 outlines how GitHub PRs use a non-standard Git refspec, `+refs/pull/PR_ID`, to store the commit built by CI systems such as Travis-CI. This patch includes a new refspec when Git's VCS handler performs a fetch operation. The refspec `+refs/pull/*:refs/pull/%x/*` is used where `%x` is an MD5 hash of the constraint's configured remote location. This ensures that if the remote location is changed, the refspec will point to a new location locally. This unblocks build failures occurring in rexray/gocsi#71 and rexray/gocsi#80. cc @sbezverk @sdboyer
akutz
force-pushed
the
feature/fetch-gh-pulls
branch
from
ea1ce3a to
c790045
Compare
|
Hi @carolynvs, The issue wasn't even pruning as it turns out. The issue revealed itself to be my lack of testing the two situations where |
|
Hi @carolynvs / @sdboyer, End-to-end Travis-CI GoCSI build with custom dep that references PR commits works! |
|
@carolynvs any more feedback here or is this good to go? |
|
Any progress on getting this merged? |
|
Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks! |
This patch introduces support for setting a dep constraint's revision to a commit that exists in a GitHub pull request. Issue #1583 outlines how GitHub PRs use a non-standard Git refspec,
+refs/pull/PR_ID, to store the commit built by CI systems such as Travis-CI. This patch includes a new refspec when Git's VCS handler performs a fetch operation.The refspec
+refs/pull/*:refs/pull/%x/*is used where%xis an MD5 hash of the constraint's configured remote location. This ensures that if the remote location is changed, the refspec will point to a new location locally.This unblocks build failures occurring in rexray/gocsi#71 and rexray/gocsi#80.
cc @sbezverk @sdboyer