From 82fc94d9304f4292a1ac0256d87ad36acefa52a2 Mon Sep 17 00:00:00 2001 From: wwade <2576056+wwade@users.noreply.github.com> Date: Tue, 19 Oct 2021 19:27:15 -0700 Subject: [PATCH] git: accept explicit commit hash for git context (#1765) * git: accept explicit commit hash for git context When checking out code from non-github repositories, the typical assumptions may not be valid, e.g. that the only interesting non-branch commits have ref names starting with refs/pull. A specific example is fetching an un-merged commit from a gerrit repository by commit hash. This change just looks at the second part of the git context path and checks if it's a SHA commit hash, and if so, will fetch and check out this commit after cloning the repository. Sample context argument: https://github.repo/project#e1772f228e06d15facdf175e5385e265b57068c0 * ci: fix test script to recognize any non-zero exit as an error hack/linter.sh didn't properly install golangci-lint in hack/bin as I already have another version of golangci-lint on my PATH, but then it failed to execute because it was looking for it specifically in hack/bin. When the executable is not found, the exit code is 127 instead of 1, and so test.sh ignored the error. Two fixes: 1. `test.sh`: - Use `if (script) ...` instead of assigning / checking a result variable to determine if each validation script passed or failed. 2. `hack/linter.sh`: - Instead of checking for golangci-lint on the path, just specifically check for an executable file (`test -x`) in the expected location. Co-authored-by: Wade Carpenter --- hack/linter.sh | 8 +-- integration/integration_test.go | 91 ++++++++++++++++++++++++++++----- pkg/buildcontext/git.go | 25 ++++++--- scripts/test.sh | 10 ++-- 4 files changed, 102 insertions(+), 32 deletions(-) diff --git a/hack/linter.sh b/hack/linter.sh index 1bd7a4a822..c07cafcff5 100755 --- a/hack/linter.sh +++ b/hack/linter.sh @@ -16,12 +16,12 @@ set -e -o pipefail -DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" +DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd ) BIN=${DIR}/bin -if ! [ -x "$(command -v golangci-lint)" ]; then +if [ ! -x "${BIN}/golangci-lint" ]; then echo "Installing GolangCI-Lint" - ${DIR}/install_golint.sh -b ${BIN} v1.23.7 + "${DIR}/install_golint.sh" -b "${BIN}" v1.23.7 fi -${BIN}/golangci-lint run +"${BIN}/golangci-lint" run diff --git a/integration/integration_test.go b/integration/integration_test.go index bfe0c20843..ba18c86b2e 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -30,6 +30,10 @@ import ( "testing" "time" + "github.com/go-git/go-git/v5" + gitConfig "github.com/go-git/go-git/v5/config" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/storage/memory" "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/v1/daemon" "github.com/pkg/errors" @@ -205,22 +209,59 @@ func TestRun(t *testing.T) { } } -func getGitRepo() string { - var branch, repoSlug string - if os.Getenv("TRAVIS_PULL_REQUEST") != "" { +func findSHA(ref plumbing.ReferenceName, refs []*plumbing.Reference) (string, error) { + for _, ref2 := range refs { + if ref.String() == ref2.Name().String() { + return ref2.Hash().String(), nil + } + } + return "", errors.New("no ref found") +} + +// getBranchSHA get a SHA commit hash for the given repo url and branch ref name. +func getBranchSHA(t *testing.T, url, branch string) string { + repo := "https://" + url + c := &gitConfig.RemoteConfig{URLs: []string{repo}} + remote := git.NewRemote(memory.NewStorage(), c) + refs, err := remote.List(&git.ListOptions{}) + if err != nil { + t.Fatalf("list remote %s#%s: %s", repo, branch, err) + } + commit, err := findSHA(plumbing.NewBranchReferenceName(branch), refs) + if err != nil { + t.Fatalf("findSHA %s#%s: %s", repo, branch, err) + } + return commit +} + +func getBranchAndURL() (branch, url string) { + var repoSlug string + if _, ok := os.LookupEnv("TRAVIS_PULL_REQUEST"); ok { branch = "master" repoSlug = os.Getenv("TRAVIS_REPO_SLUG") log.Printf("Travis CI Pull request source repo: %s branch: %s\n", repoSlug, branch) - } else { + } else if _, ok := os.LookupEnv("TRAVIS_BRANCH"); ok { branch = os.Getenv("TRAVIS_BRANCH") repoSlug = os.Getenv("TRAVIS_REPO_SLUG") log.Printf("Travis CI repo: %s branch: %s\n", repoSlug, branch) + } else { + branch = "master" + repoSlug = "GoogleContainerTools/kaniko" } - return "github.com/" + repoSlug + "#refs/heads/" + branch + url = "github.com/" + repoSlug + return } -func TestGitBuildcontext(t *testing.T) { - repo := getGitRepo() +func getGitRepo(t *testing.T, explicit bool) string { + branch, url := getBranchAndURL() + if explicit { + return url + "#" + getBranchSHA(t, url, branch) + } + return url + "#refs/heads/" + branch +} + +func testGitBuildcontextHelper(t *testing.T, repo string) { + t.Log("testGitBuildcontextHelper repo", repo) dockerfile := fmt.Sprintf("%s/%s/Dockerfile_test_run_2", integrationPath, dockerfilesPath) // Build with docker @@ -257,8 +298,32 @@ func TestGitBuildcontext(t *testing.T) { checkContainerDiffOutput(t, diff, expected) } +// TestGitBuildcontext explicitly names the master branch +// Example: +// git://github.com/myuser/repo#refs/heads/master +func TestGitBuildcontext(t *testing.T) { + repo := getGitRepo(t, false) + testGitBuildcontextHelper(t, repo) +} + +// TestGitBuildcontextNoRef builds without any commit / branch reference +// Example: +// git://github.com/myuser/repo +func TestGitBuildcontextNoRef(t *testing.T) { + _, repo := getBranchAndURL() + testGitBuildcontextHelper(t, repo) +} + +// TestGitBuildcontextExplicitCommit uses an explicit commit hash instead of named reference +// Example: +// git://github.com/myuser/repo#b873088c4a7b60bb7e216289c58da945d0d771b6 +func TestGitBuildcontextExplicitCommit(t *testing.T) { + repo := getGitRepo(t, true) + testGitBuildcontextHelper(t, repo) +} + func TestGitBuildcontextSubPath(t *testing.T) { - repo := getGitRepo() + repo := getGitRepo(t, false) dockerfile := "Dockerfile_test_run_2" // Build with docker @@ -267,8 +332,8 @@ func TestGitBuildcontextSubPath(t *testing.T) { append([]string{ "build", "-t", dockerImage, - "-f", dockerfile, - repo + ":" + filepath.Join(integrationPath, dockerfilesPath), + "-f", filepath.Join(integrationPath, dockerfilesPath, dockerfile), + repo, })...) out, err := RunCommandWithoutTest(dockerCmd) if err != nil { @@ -302,7 +367,7 @@ func TestGitBuildcontextSubPath(t *testing.T) { } func TestBuildViaRegistryMirrors(t *testing.T) { - repo := getGitRepo() + repo := getGitRepo(t, false) dockerfile := fmt.Sprintf("%s/%s/Dockerfile_registry_mirror", integrationPath, dockerfilesPath) // Build with docker @@ -342,7 +407,7 @@ func TestBuildViaRegistryMirrors(t *testing.T) { } func TestBuildWithLabels(t *testing.T) { - repo := getGitRepo() + repo := getGitRepo(t, false) dockerfile := fmt.Sprintf("%s/%s/Dockerfile_test_label", integrationPath, dockerfilesPath) testLabel := "mylabel=myvalue" @@ -385,7 +450,7 @@ func TestBuildWithLabels(t *testing.T) { } func TestBuildWithHTTPError(t *testing.T) { - repo := getGitRepo() + repo := getGitRepo(t, false) dockerfile := fmt.Sprintf("%s/%s/Dockerfile_test_add_404", integrationPath, dockerfilesPath) // Build with docker diff --git a/pkg/buildcontext/git.go b/pkg/buildcontext/git.go index 30bf918cfd..e35b1dbe9d 100644 --- a/pkg/buildcontext/git.go +++ b/pkg/buildcontext/git.go @@ -17,6 +17,7 @@ limitations under the License. package buildcontext import ( + "errors" "fmt" "os" "strings" @@ -66,8 +67,14 @@ func (g *Git) UnpackTarFromBuildContext() (string, error) { SingleBranch: g.opts.GitSingleBranch, RecurseSubmodules: getRecurseSubmodules(g.opts.GitRecurseSubmodules), } + var fetchRef string if len(parts) > 1 { - if !strings.HasPrefix(parts[1], "refs/pull/") { + if plumbing.IsHash(parts[1]) || !strings.HasPrefix(parts[1], "refs/pull/") { + // Handle any non-branch refs separately. First, clone the repo HEAD, and + // then fetch and check out the fetchRef. + fetchRef = parts[1] + } else { + // Branches will be cloned directly. options.ReferenceName = plumbing.ReferenceName(parts[1]) } } @@ -82,23 +89,25 @@ func (g *Git) UnpackTarFromBuildContext() (string, error) { logrus.Debugf("Getting source from reference %s", options.ReferenceName) r, err := git.PlainClone(directory, false, &options) - if err != nil { return directory, err } - if len(parts) > 1 && strings.HasPrefix(parts[1], "refs/pull/") { - + if fetchRef != "" { err = r.Fetch(&git.FetchOptions{ RemoteName: "origin", - RefSpecs: []config.RefSpec{config.RefSpec(parts[1] + ":" + parts[1])}, + RefSpecs: []config.RefSpec{config.RefSpec(fetchRef + ":" + fetchRef)}, }) - if err != nil { + if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { return directory, err } } + checkoutRef := fetchRef if len(parts) > 2 { + checkoutRef = parts[2] + } + if checkoutRef != "" { // ... retrieving the commit being pointed by HEAD _, err := r.Head() if err != nil { @@ -112,13 +121,13 @@ func (g *Git) UnpackTarFromBuildContext() (string, error) { // ... checking out to desired commit err = w.Checkout(&git.CheckoutOptions{ - Hash: plumbing.NewHash(parts[2]), + Hash: plumbing.NewHash(checkoutRef), }) if err != nil { return directory, err } } - return directory, err + return directory, nil } func getGitReferenceName(directory string, url string, branch string) (plumbing.ReferenceName, error) { diff --git a/scripts/test.sh b/scripts/test.sh index 883aee5b24..d9439a7218 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -37,15 +37,11 @@ fail=0 for s in "${scripts[@]}" do echo "RUN ${s}" - set +e - ./$s - result=$? - set -e - if [[ $result -eq 1 ]]; then + if "./${s}"; then + echo -e "${GREEN}PASSED${RESET} ${s}" + else echo -e "${RED}FAILED${RESET} ${s}" fail=1 - else - echo -e "${GREEN}PASSED${RESET} ${s}" fi done exit $fail