Add handling for GIT_CHECKOUT variable to skip checkout

This is a general Merge Request template. Consider to choose a template from the list above if it will match your case more.

What does this MR do?

Handles GIT_CHECKOUT variable as discussed in #2011 (closed). This allows not doing a git checkout on the repository. It defaults to true.

Why was this MR needed?

#2011 (closed)

Are there points in the code the reviewer needs to double check?

Where do I add documentation for this?

Does this MR meet the acceptance criteria?

• Documentation created/updated
• Tests
  • Added for this feature/bug
  • All builds are passing
• Branch has no merge conflicts with master (if you do - rebase it please)

What are the relevant issue numbers?

Closes #2011 (closed)

marked as a Work In Progress

changed the description

Thanks @Rukenshia - this looks pretty good to me. Just the one comment. People will be expecting GIT_CHECKOUT=0 to work as well as GIT_CHECKOUT=false.

resolved all discussions

@nick.thomas thanks for your comment, I've updated the MR. Maybe you can help me figuring out why the tests fail, though. Right now I can't even test master locally, I'm getting similar errors to the CI.

unmarked as a Work In Progress

changed milestone to %9.3

added improvement label

I've added the w.Notice @tmaczukin. Is it fine like that now or do you still want me to add aliases for GitCheckoutTrue and GitCheckoutFalse?

@Rukenshia As for the failing tests - the assertion is looking for Checking out as string, while Runner is generating Checking out _SHORT_SHA_HERE_ as. Change assertions to assert.Contains(t, "Checking out", out)/assert.NotContains(t, "Checking out", out) or to assert.Regexp(t, "Checkint out [a-f0-9]+ as", out)/assert.NotRegexp(t, "Checkint out [a-f0-9]+ as", out) and it should work.

@Rukenshia If we're leaving the true/false as returned value, then I don't see a need to turning it into constants. Just make a checkout on true or print a notice on false.

Also if we're now printing a notice about skipping checkout then let's update the tests :)

resolved all discussions

Added that in the previous commit already @tmaczukin, I've now updated the other tests to use the RegEx, thanks for that.

Regardless of that, where should I add documentation for this? I assume straight into the ci/yaml/README.md in gitlab-ce?

@Rukenshia Yup, and I think it will fit best between Git strrategy and Git submodule strategy descriptions.

After creating the MR to GitLab CE please leave a cross-reference between MRs :)

@Rukenshia One more thing. While I'm looking now on the tests I don't think there is any advantage of having:

build.Runner.PreCloneScript = "echo pre-clone-script"
build.Variables = append(build.Variables, common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"})
build.Variables = append(build.Variables, common.JobVariable{Key: "GIT_CHECKOUT", Value: "false"})

out, err := runBuildReturningOutput(t, build)
assert.NoError(t, err)
assert.Contains(t, out, "Cloning repository")

out, err = runBuildReturningOutput(t, build)
assert.NoError(t, err)
assert.Contains(t, out, "Fetching changes")

out, err = runBuildReturningOutput(t, build)
assert.NoError(t, err)
assert.Contains(t, out, "Skippping Git checkout")

assert.Contains(t, out, "pre-clone-script")

instead of:

build.Runner.PreCloneScript = "echo pre-clone-script"
build.Variables = append(build.Variables, common.JobVariable{Key: "GIT_STRATEGY", Value: "fetch"})
build.Variables = append(build.Variables, common.JobVariable{Key: "GIT_CHECKOUT", Value: "false"})

out, err := runBuildReturningOutput(t, build)
assert.NoError(t, err)
assert.Contains(t, out, "Cloning repository")
assert.Contains(t, out, "Fetching changes")
assert.Contains(t, out, "Skippping Git checkout")
assert.Contains(t, out, "pre-clone-script")

since: 1) we're already testing the existence of pre-clone-script string once after last build run, 2) all required strings should exists even if we execute the build once, 3) we're already doing tests as I proposed above for other strategies.

Could you change all four tests that you're updating/adding?

@tmaczukin I've updated the tests. I wasn't too sure why there previously where multiple calls to runBuildReturningOutput, so I just left it that way. All tests should be changed now

@Rukenshia One more little change. I remember why there was a double call of runBUildReturningOutput() in fetch and clone strategies test. This was to make sure that with fetch strategy we have a clone if the repository was not cloned already and then a fetch, and for the clone strategy to make sure that on both calls we're doing a clone. Please update these two tests and I'm merging this:

func TestBuildWithGitStrategyClone(t *testing.T) {
	onEachShell(t, func(t *testing.T, shell string) {
		successfulBuild, err := common.GetSuccessfulBuild()
		assert.NoError(t, err)
		build, cleanup := newBuild(t, successfulBuild, shell)
		defer cleanup()

		build.Runner.PreCloneScript = "echo pre-clone-script"
		build.Variables = append(build.Variables, common.JobVariable{Key: "GIT_STRATEGY", Value: "clone"})

		out, err := runBuildReturningOutput(t, build)
		assert.NoError(t, err)
		assert.Contains(t, out, "Cloning repository")

		out, err := runBuildReturningOutput(t, build)
		assert.NoError(t, err)
		assert.Contains(t, out, "Cloning repository")
		assert.Regexp(t, "Checking out [a-f0-9]+ as", out)
		assert.Contains(t, out, "pre-clone-script")
	})
}

func TestBuildWithGitStrategyCloneNoCheckout(t *testing.T) {
	onEachShell(t, func(t *testing.T, shell string) {
		successfulBuild, err := common.GetSuccessfulBuild()
		assert.NoError(t, err)
		build, cleanup := newBuild(t, successfulBuild, shell)
		defer cleanup()

		build.Runner.PreCloneScript = "echo pre-clone-script"
		build.Variables = append(build.Variables, common.JobVariable{Key: "GIT_STRATEGY", Value: "clone"})
		build.Variables = append(build.Variables, common.JobVariable{Key: "GIT_CHECKOUT", Value: "false"})

		out, err := runBuildReturningOutput(t, build)
		assert.NoError(t, err)
		assert.Contains(t, out, "Cloning repository")

		out, err := runBuildReturningOutput(t, build)
		assert.NoError(t, err)
		assert.Contains(t, out, "Cloning repository")
		assert.Contains(t, out, "Skippping Git checkout")
		assert.Contains(t, out, "pre-clone-script")
	})
}

Done @tmaczukin.

Excellent! Thanks for your work @Rukenshia!

approved this merge request

@tmaczukin not sure why the build is failing, I tried rerunning the job once already but it's still failing. Any idea?

https://twitter.com/gitlabstatus/status/867786722935001089 - we have problems with our Shared Runners. Let's get back to this when it will be resolved - then please restart the job and when the pipeline will be finished this MR will be merged automatically :)

merged