-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce optional "refspec" to git PipelineResource, refactor #2320
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hi @ad22. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
/kind feature |
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
The following is the coverage report on pkg/.
|
/test pull-tekton-pipeline-integration-tests |
The following is the coverage report on pkg/.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I think this looks good, I just left a few style comments re: commenting and around the code structure with relation to our existing tests.
pkg/git/git.go
Outdated
checkoutParam = spec.Revision | ||
} | ||
|
||
fetchArgs = append(fetchArgs, "origin", "--update-head-ok") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't recognize this flag and looked it up in the docs, which say:
By default git fetch refuses to update the head which corresponds to the current branch. This flag disables the check. This is purely for the internal use for git pull to communicate with git fetch, and unless you are implementing your own Porcelain you are not supposed to use it.
I'm not sure what Porcelain is in this context but it would be great to include a note here on what the flag does and why we need to use it given that it's mostly intended for internal-to-git usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment w/ explanation. Required because git init checks out the master branch by default.
test/git_checkout_test.go
Outdated
t.Parallel() | ||
|
||
// Pair of revision, refspec | ||
var pairs = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach we take elsewhere in our testing is to build a table of test cases with descriptions, inputs, and expected outputs (if any).
For example, I'd approach this test as follows:
for _, tc := range []struct{
description string
revision string
refspec string
}{{
description: "Fetch refs/tags/v0.1.0 alongside master and checkout the master branch",
revision: "master",
refspec: "refs/tags/v0.1.0:refs/tags/v0.1.0 refs/heads/master:refs/heads/master",
}, {
description: "Checkout specific revision from refs/pull/1009/head's commit chain",
revision: ""968d5d37a61bfb85426c885dc1090c1cc4b33436"",
refspec: "refs/pull/1009/head",
}, {
// ...and so on...
}} {
t.Run(tc.description, func(t *testing.T) {
// test body
})
}
The benefit is that this moves the comments into test names, which get logged on failure, and it adds a clean field name for each of the entries in your current pairs list. It would also match more closely what we're doing elsewhere, which is nice from a pure code-style perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbwsg Thanks for the explanation. That's a great way to organize the test cases. I've updated the PR.
test/v1alpha1/git_checkout_test.go
Outdated
t.Parallel() | ||
|
||
// Pair of revision, refspec | ||
var pairs = []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here as in the other test file: it would be great to see this turned into a table with field names.
The following is the coverage report on pkg/.
|
test/v1alpha1/git_checkout_test.go
Outdated
knativetest.CleanupOnInterrupt(func() { tearDown(t, c, namespace) }, t.Logf) | ||
defer tearDown(t, c, namespace) | ||
|
||
t.Logf("Creating Git PipelineResource %s", gitSourceResourceName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to log in the failure case with the t.Fatalf
but these kinds of status lines can make test logs quite noisy to trace through when looking at verbose output. I suggest removing the use of t.Logf
unless absolutely necessary. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the log entries in the latest squashed commit.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thank you for your contribution. Please note that we do not auto-squash commits on merge, so I would ask you if you could squash your commits down to one or two. |
@@ -30,7 +30,7 @@ var ( | |||
) | |||
|
|||
const ( | |||
// PipelineResourceTypeGit indicates that this source is a GitHub repo. | |||
// PipelineResourceTypeGit indicates that this source is a Git repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
// The --force parameter tells git-fetch that its ok to update an existing HEAD in a | ||
// non-fast-forward manner (though this cannot be possible on initial fetch, it can help | ||
// when the refspec specifies the same destination twice) | ||
fetchArgs = append(fetchArgs, "origin", "--update-head-ok", "--force") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to limit the use of --update-head-ok
to the case where the revision is "master"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the value in putting an additional conditional for that case. This is (guaranteed to be) the first fetch command run after git-init, so its implied that
a. it will always be the master branch, unless git-init changes behavior
b. we are not expecting any unintended updates to existing refs
flag.StringVar(&fetchSpec.Path, "path", "", "Path of directory under which Git repository will be copied") | ||
flag.BoolVar(&fetchSpec.SSLVerify, "sslVerify", true, "Enable/Disable SSL verification in the git config") | ||
flag.BoolVar(&submodules, "submodules", true, "Initialize and fetch Git submodules") | ||
flag.BoolVar(&fetchSpec.Submodules, "submodules", true, "Initialize and fetch Git submodules") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change does not seem to be needed for this patch, but I guess it makes sense anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually needed for the patch, to pass the submodule setting to git.go and prevent --recurse-submodules from being appended to the fetch command when
the user specified submodule: "false"
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
@ad22 needs a rebase, sorry 🙏 |
The git pipeline resource does not allow for any use case beyond a basic ref fetch, and is especially problematic for servers that don't support direct SHA fetches, which means that the user cannot check out specific commit hashes not pointed to by a named ref. The user is also not able to check out other refs alongside (example, the tag history). Current implementation also has a few corner cases, such as checking out/reset --hard on the master branch after git-init, running git reset --hard when checkout fails and so on. This leads to inconsistent behavior (see tektoncd#2282). The end result is that several users might end up creating their own custom containers for fetching and checking out what they need. This change makes the git resource much more extensible, via an optional refspec parameter. This change should be fully backward compatible w.r.t to revision parameter EXCEPT the ability to fetch git commit short hashes (according to the e2e tests) - Adds ability to checkout older commit hashes on a particular ref chain (when fetch via commit SHA is not enabled on the server) - Adds ability to fetch several other refs (for example, refs/tags) alongside the main revision - Provides resolution for tektoncd#2282 - If the user needs to checkout a branch, they must specify the appropriate refspec and revision as the ref of the branch. There was no way to do this in the original implementation - Fixes bug reported in tektoncd#2282 by ensuring the revision is always checked out on a detached HEAD when specified w/o refspec - Fixes bug - depth parameter is not passed to git submodule update, leading to unnecessary data transfer - Fixes bug --recurse-submodules is appended to the fetch command even when the user specified submodule: "false", leading to unnecessary data transfer - Fixes bug tektoncd#1843 - git init ssh symlink is failing Addresses tektoncd#2282, tektoncd#1843
The following is the coverage report on pkg/.
|
Rebased and added minor fix for #1843 |
The following is the coverage report on pkg/.
|
/test pull-tekton-pipeline-build-tests |
I'm going to merge this now and if there are any outstanding comments that need to be addressed we can tackle them in a follow-up. Thanks for the PR @ad22 ! /lgtm |
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
Add refspec support to git task corresponding to tektoncd/pipeline#2320 Add a new batch merge task (tektoncd/pipeline#508) that re-uses the git-init container and subsequently performs the requested batch merge mode on top of the checked out revision
Changes
The git pipeline resource does not allow for any use case beyond a basic ref fetch, and is especially problematic for servers that don't support direct SHA fetches, which means that
the user cannot check out specific commit hashes not pointed to by a named ref. The user
is also not able to check out other refs alongside (example, the tag history). Current implementation also has some unexpected behavior, such as checking out/reset --hard
on the master branch after git-init (see #2282), running git pull without depth when checkout fails and so on . The end result is that several users might end up creating their own custom containers for fetching and checking out what they need. This change makes the git resource much more extensible, via an optional refspec parameter.
Addresses #2282
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes
This is my first time contributing to this repository, please let me know if I missed something. Thanks!