Parameterize git clone depth and fix disabling submodules #1729

silverlyra · 2019-12-10T21:46:17Z

Changes

Adds a new depth param to git resources to control the --depth flag to git fetch. When set to '0', shallow fetches are disabled entirely. Git Pipeline Resources should allow depth to be specified #1450
Fixes the submodule param so that it can be meaningfully set to false. git-init: Setting submodules to false has no effect #1727

Submitter Checklist

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

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

### Features

- The clone depth of `git` resources can now be set with a new `depth` parameter. Shallow clones can be disabled entirely by setting depth to `'0'`. #1450

### Fixes

- Initializing git submodules can now be turned off by setting the `submodules` param to `'false'`. #1727

tekton-robot · 2019-12-10T21:46:33Z

Hi @silverlyra. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

vdemeester

On nit, otherwise looks good 👍

vdemeester · 2019-12-11T08:36:31Z

cmd/git-init/main.go

 )

+func init() {


nit: any reason for this to be in init() and not on top of main() ; the end result should be the same :)

I find this more intuitive than sticking it in main. Most uses of the flag package I’ve seen don’t use the …Var forms and just live inside a top-level var block. Those calls run before main and stand out visually from the code in main – to me, this felt like a more consistent and minimal refactor than adding them to main.

tekton-robot · 2019-12-11T08:39:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

imjasonh

Thanks for fixing this!

It could be useful to add a YAML example (which would be run in CI as a test) that demonstrates submodule fetch and custom depth. That would have helped us catch this bug originally.

dibyom · 2019-12-11T15:26:34Z

/ok-to-test

imjasonh · 2019-12-11T15:46:29Z

pkg/apis/pipeline/v1alpha1/git_resource_test.go

+		desc        string
+		gitResource *v1alpha1.GitResource
+		want        corev1.Container
+	}{


nit: If you collapse this { and the next one, you can save a couple horizontal lines, and a vertical tabwidth:

for _, tc := range []struct { // struct fields } {{ // case 1 }, { // case 2 }} { t.Run(...) }

imjasonh · 2019-12-11T15:47:42Z

pkg/apis/pipeline/v1alpha1/git_resource_test.go

+			Revision:   "test",
+			GitImage:   "override-with-git:latest",
+			Submodules: true,
+			Depth:      0,


What actually happens when you clone with --depth=0? Is this a case we should catch and fail in validation? Should we reject negative values?

(It's probably fine to just pass through whatever and let git fail as it wants, but it can be useful to avoid running a container that we know will fail -- this validation also doesn't have to happen in this change)

cmd/git-init/main.go

imjasonh · 2019-12-11T15:50:23Z

pkg/git/git.go

-	if _, err := run(logger, "", "fetch", "--depth=1", "--recurse-submodules=yes", "origin", revision); err != nil {
+
+	fetchArgs := []string{"fetch", "--recurse-submodules=yes"}
+	if spec.Depth > 0 {


Ah I see, we just ignore any value <= 0. This seems like it could surprise users, I think we should reject it upfront instead. 🤔

Or at least log that we're ignoring it? Also probably worth logging that we couldn't parse the depth as a uint, and that we're falling back to the default.

I think we should reject [depth of 0]

The original issue asking for depth control (#1450) isn't super clearly written, but my interpretation is that the author was asking for a way to turn off shallow clones entirely:

actual behavior: Git resources are always shallow cloned

git fetch --depth 0 is an error, so I’m overloading it to mean “full depth”. But I agree it’s subtle; how about I add a separate param shallow (defaulting to true) as the mechanism to opt into a full clone?

Also probably worth logging that we couldn't parse the depth as a uint, and that we're falling back to the default.

Unrecognized values for submodules are currently swallowed without logging anything – I kept this consistent with what’s already there, but I agree it’s not user-friendly. What if we have NewGitResource return an error in these cases? Would that be triggered from the validating webhook?

Yeah that should bubble up to webhook validation.

FWIW I think this change is pretty much just fine without all this extra validation, if you want to get this in and fix the obvious regression first, then add better validation later.

that would be fine by me 😎

Fixes #1727. In #1531, the submodules parameter was added, but it has no influence on the generated git-init invocation.

silverlyra · 2019-12-11T18:56:28Z

Can I get some 👀 on the CI output? I think all the failures I’m seeing are spurious.

imjasonh · 2019-12-11T19:05:55Z

Yeah most of these are definitely unrelated. The unit test failures should be fixed by #1709 I think (though I thought #1731 would fix it...), and the link checker failure is just annoyingly flaky today.

However, there's a relevant error helpfully tucked deeeep within the integration test logs:

I1211 18:21:28.622] 2019/12/11 18:21:28 Unexpected error running "go build": exit status 2
I1211 18:21:28.623] # github.com/tektoncd/pipeline/pkg/git
I1211 18:21:28.624] pkg/git/git.go:80:33: cannot use spec.Depth (type uint) as type string in append

Which comes from here: https://github.com/tektoncd/pipeline/pull/1729/files#diff-4de8c4734d5ffab0d7fd8835bf8b3421R80

If you fix that we should be able to check that e2e tests aren't broken, then we'll just be waiting for unit tests and link-checker to start behaving.

Sorry for the noise, and thanks for sticking with it 💪

This lets users control the shallow clone depth, or disable shallow clones entirely by setting depth to 0. Some build processes expect to be able to access Git history. Increasing depth also helps users re-run builds of commits that may no longer be at the head of a branch without falling back to the full `git pull` code path. Closes #1450.

silverlyra · 2019-12-11T19:58:31Z

/retest

silverlyra · 2019-12-11T22:18:26Z

/test pull-tekton-pipeline-build-tests

silverlyra · 2019-12-11T22:26:11Z

🎉 Other than the link check failure, tests are now passing 😁

vdemeester · 2019-12-12T08:37:59Z

/test pull-tekton-pipeline-build-tests

imjasonh · 2019-12-12T15:24:00Z

/test pull-tekton-pipeline-build-tests

imjasonh · 2019-12-12T16:49:51Z

/test pull-tekton-pipeline-build-tests

imjasonh · 2019-12-12T16:54:20Z

/lgtm

🎉

Fabian-K · 2020-02-19T12:54:41Z

Hi @silverlyra, we have a pipeline that relies on the entire repo to be cloned. With tekton 0.9 we therefore added a step git fetch --unshallow as it cloned by default with (I think) depth = 1.

After upgrading to 0.10.1 (and without specifying any value for depth) this fails now with fatal: --unshallow on a complete repository does not make sense. It looks like the default behavior is now a full clone. Is that intentional or does it make sense to change the default behavior back to 1?
Thanks, Fabian

Update: I don´t know what happened yesterday but I´m not able to reproduce it anymore. I´ll raise a issue it if i see it again. Sorry!

tekton-robot requested review from abayer and a user December 10, 2019 21:46

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2019

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Dec 10, 2019

vdemeester approved these changes Dec 11, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2019

imjasonh reviewed Dec 11, 2019

View reviewed changes

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 11, 2019

imjasonh reviewed Dec 11, 2019

View reviewed changes

Lyra Naeseth added 2 commits December 11, 2019 10:07

Fix git resource submodule test descriptions

8ddf9b0

Respect submodules: false in git resource

12df70d

Fixes #1727. In #1531, the submodules parameter was added, but it has no influence on the generated git-init invocation.

tekton-robot assigned imjasonh Dec 12, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2019

tekton-robot merged commit 24c2131 into tektoncd:master Dec 12, 2019

silverlyra mentioned this pull request Jan 6, 2020

Fix passing -submodules flag to git-init #1813

Merged

3 tasks

vdemeester mentioned this pull request Jan 15, 2020

Git "depth" value is ignored #1871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameterize git clone depth and fix disabling submodules #1729

Parameterize git clone depth and fix disabling submodules #1729

silverlyra commented Dec 10, 2019 •

edited

Loading

tekton-robot commented Dec 10, 2019

vdemeester left a comment

vdemeester Dec 11, 2019

silverlyra Dec 11, 2019

tekton-robot commented Dec 11, 2019

imjasonh left a comment

dibyom commented Dec 11, 2019

imjasonh Dec 11, 2019

imjasonh Dec 11, 2019

imjasonh Dec 11, 2019

silverlyra Dec 11, 2019

imjasonh Dec 11, 2019

silverlyra Dec 11, 2019

silverlyra commented Dec 11, 2019

imjasonh commented Dec 11, 2019

silverlyra commented Dec 11, 2019

silverlyra commented Dec 11, 2019

silverlyra commented Dec 11, 2019

vdemeester commented Dec 12, 2019

imjasonh commented Dec 12, 2019

imjasonh commented Dec 12, 2019

imjasonh commented Dec 12, 2019

Fabian-K commented Feb 19, 2020 •

edited

Loading

		)

		func init() {

Parameterize git clone depth and fix disabling submodules #1729

Parameterize git clone depth and fix disabling submodules #1729

Conversation

silverlyra commented Dec 10, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Dec 10, 2019

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Dec 11, 2019

imjasonh left a comment

Choose a reason for hiding this comment

dibyom commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverlyra commented Dec 11, 2019

imjasonh commented Dec 11, 2019

silverlyra commented Dec 11, 2019

silverlyra commented Dec 11, 2019

silverlyra commented Dec 11, 2019

vdemeester commented Dec 12, 2019

imjasonh commented Dec 12, 2019

imjasonh commented Dec 12, 2019

imjasonh commented Dec 12, 2019

Fabian-K commented Feb 19, 2020 • edited Loading

silverlyra commented Dec 10, 2019 •

edited

Loading

Fabian-K commented Feb 19, 2020 •

edited

Loading