From a6025255246aab21426a51986b7d138a14d9ac90 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 7 Dec 2017 13:44:08 -0800 Subject: [PATCH 1/2] gitutils: fix checking out submodules Signed-off-by: Tonis Tiigi --- builder/remotecontext/git/gitutils.go | 20 +++++- builder/remotecontext/git/gitutils_test.go | 79 +++++++++++++++++----- 2 files changed, 79 insertions(+), 20 deletions(-) diff --git a/builder/remotecontext/git/gitutils.go b/builder/remotecontext/git/gitutils.go index 7bc2268155..b579db9765 100644 --- a/builder/remotecontext/git/gitutils.go +++ b/builder/remotecontext/git/gitutils.go @@ -29,6 +29,10 @@ func Clone(remoteURL string) (string, error) { return "", err } + return cloneGitRepo(repo) +} + +func cloneGitRepo(repo gitRepo) (string, error) { fetch := fetchArgs(repo.remote, repo.ref) root, err := ioutil.TempDir("", "docker-build-git") @@ -50,7 +54,19 @@ func Clone(remoteURL string) (string, error) { return "", errors.Wrapf(err, "error fetching: %s", output) } - return checkoutGit(root, repo.ref, repo.subdir) + root, err = checkoutGit(root, repo.ref, repo.subdir) + if err != nil { + return "", err + } + + cmd := exec.Command("git", "submodule", "update", "--init", "--recursive", "--depth=1") + cmd.Dir = root + output, err := cmd.CombinedOutput() + if err != nil { + return "", errors.Wrapf(err, "error initializing submodules: %s", output) + } + + return root, nil } func parseRemoteURL(remoteURL string) (gitRepo, error) { @@ -96,7 +112,7 @@ func getRefAndSubdir(fragment string) (ref string, subdir string) { } func fetchArgs(remoteURL string, ref string) []string { - args := []string{"fetch", "--recurse-submodules=yes"} + args := []string{"fetch"} if supportsShallowClone(remoteURL) { args = append(args, "--depth", "1") diff --git a/builder/remotecontext/git/gitutils_test.go b/builder/remotecontext/git/gitutils_test.go index c638a498f2..b4c49195f0 100644 --- a/builder/remotecontext/git/gitutils_test.go +++ b/builder/remotecontext/git/gitutils_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "net/url" "os" + "os/exec" "path/filepath" "runtime" "strings" @@ -61,7 +62,7 @@ func TestCloneArgsSmartHttp(t *testing.T) { }) args := fetchArgs(serverURL.String(), "master") - exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} + exp := []string{"fetch", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) } @@ -77,13 +78,13 @@ func TestCloneArgsDumbHttp(t *testing.T) { }) args := fetchArgs(serverURL.String(), "master") - exp := []string{"fetch", "--recurse-submodules=yes", "origin", "master"} + exp := []string{"fetch", "origin", "master"} assert.Equal(t, exp, args) } func TestCloneArgsGit(t *testing.T) { args := fetchArgs("git://github.com/docker/docker", "master") - exp := []string{"fetch", "--recurse-submodules=yes", "--depth", "1", "origin", "master"} + exp := []string{"fetch", "--depth", "1", "origin", "master"} assert.Equal(t, exp, args) } @@ -165,24 +166,55 @@ func TestCheckoutGit(t *testing.T) { _, err = gitWithinDir(gitDir, "checkout", "master") require.NoError(t, err) + // set up submodule + subrepoDir := filepath.Join(root, "subrepo") + _, err = git("init", subrepoDir) + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "config", "user.email", "test@docker.com") + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "config", "user.name", "Docker test") + require.NoError(t, err) + + err = ioutil.WriteFile(filepath.Join(subrepoDir, "subfile"), []byte("subcontents"), 0644) + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "add", "-A") + require.NoError(t, err) + + _, err = gitWithinDir(subrepoDir, "commit", "-am", "Subrepo initial") + require.NoError(t, err) + + cmd := exec.Command("git", "submodule", "add", subrepoDir, "sub") // this command doesn't work with --work-tree + cmd.Dir = gitDir + require.NoError(t, cmd.Run()) + + _, err = gitWithinDir(gitDir, "add", "-A") + require.NoError(t, err) + + _, err = gitWithinDir(gitDir, "commit", "-am", "With submodule") + require.NoError(t, err) + type singleCase struct { - frag string - exp string - fail bool + frag string + exp string + fail bool + submodule bool } cases := []singleCase{ - {"", "FROM scratch", false}, - {"master", "FROM scratch", false}, - {":subdir", "FROM scratch" + eol + "EXPOSE 5000", false}, - {":nosubdir", "", true}, // missing directory error - {":Dockerfile", "", true}, // not a directory error - {"master:nosubdir", "", true}, - {"master:subdir", "FROM scratch" + eol + "EXPOSE 5000", false}, - {"master:../subdir", "", true}, - {"test", "FROM scratch" + eol + "EXPOSE 3000", false}, - {"test:", "FROM scratch" + eol + "EXPOSE 3000", false}, - {"test:subdir", "FROM busybox" + eol + "EXPOSE 5000", false}, + {"", "FROM scratch", false, true}, + {"master", "FROM scratch", false, true}, + {":subdir", "FROM scratch" + eol + "EXPOSE 5000", false, false}, + {":nosubdir", "", true, false}, // missing directory error + {":Dockerfile", "", true, false}, // not a directory error + {"master:nosubdir", "", true, false}, + {"master:subdir", "FROM scratch" + eol + "EXPOSE 5000", false, false}, + {"master:../subdir", "", true, false}, + {"test", "FROM scratch" + eol + "EXPOSE 3000", false, false}, + {"test:", "FROM scratch" + eol + "EXPOSE 3000", false, false}, + {"test:subdir", "FROM busybox" + eol + "EXPOSE 5000", false, false}, } if runtime.GOOS != "windows" { @@ -197,11 +229,22 @@ func TestCheckoutGit(t *testing.T) { for _, c := range cases { ref, subdir := getRefAndSubdir(c.frag) - r, err := checkoutGit(gitDir, ref, subdir) + r, err := cloneGitRepo(gitRepo{remote: gitDir, ref: ref, subdir: subdir}) if c.fail { assert.Error(t, err) continue + } else { + require.NoError(t, err) + if c.submodule { + b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) + require.NoError(t, err) + assert.Equal(t, "subcontents", string(b)) + } else { + _, err := os.Stat(filepath.Join(r, "sub/subfile")) + require.Error(t, err) + require.True(t, os.IsNotExist(err)) + } } b, err := ioutil.ReadFile(filepath.Join(r, "Dockerfile")) From 4ea320cb8ef71c2fc9ee391e2e8be915ba99b17e Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Fri, 8 Dec 2017 11:51:10 -0800 Subject: [PATCH 2/2] gitutils: remove checkout directory on error Signed-off-by: Tonis Tiigi --- builder/remotecontext/git/gitutils.go | 12 +++++++++--- builder/remotecontext/git/gitutils_test.go | 20 ++++++++++---------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/builder/remotecontext/git/gitutils.go b/builder/remotecontext/git/gitutils.go index b579db9765..67cff594ab 100644 --- a/builder/remotecontext/git/gitutils.go +++ b/builder/remotecontext/git/gitutils.go @@ -32,7 +32,7 @@ func Clone(remoteURL string) (string, error) { return cloneGitRepo(repo) } -func cloneGitRepo(repo gitRepo) (string, error) { +func cloneGitRepo(repo gitRepo) (checkoutDir string, err error) { fetch := fetchArgs(repo.remote, repo.ref) root, err := ioutil.TempDir("", "docker-build-git") @@ -40,6 +40,12 @@ func cloneGitRepo(repo gitRepo) (string, error) { return "", err } + defer func() { + if err != nil { + os.RemoveAll(root) + } + }() + if out, err := gitWithinDir(root, "init"); err != nil { return "", errors.Wrapf(err, "failed to init repo at %s: %s", root, out) } @@ -54,7 +60,7 @@ func cloneGitRepo(repo gitRepo) (string, error) { return "", errors.Wrapf(err, "error fetching: %s", output) } - root, err = checkoutGit(root, repo.ref, repo.subdir) + checkoutDir, err = checkoutGit(root, repo.ref, repo.subdir) if err != nil { return "", err } @@ -66,7 +72,7 @@ func cloneGitRepo(repo gitRepo) (string, error) { return "", errors.Wrapf(err, "error initializing submodules: %s", output) } - return root, nil + return checkoutDir, nil } func parseRemoteURL(remoteURL string) (gitRepo, error) { diff --git a/builder/remotecontext/git/gitutils_test.go b/builder/remotecontext/git/gitutils_test.go index b4c49195f0..fd58d6bd98 100644 --- a/builder/remotecontext/git/gitutils_test.go +++ b/builder/remotecontext/git/gitutils_test.go @@ -234,17 +234,17 @@ func TestCheckoutGit(t *testing.T) { if c.fail { assert.Error(t, err) continue - } else { + } + require.NoError(t, err) + defer os.RemoveAll(r) + if c.submodule { + b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) require.NoError(t, err) - if c.submodule { - b, err := ioutil.ReadFile(filepath.Join(r, "sub/subfile")) - require.NoError(t, err) - assert.Equal(t, "subcontents", string(b)) - } else { - _, err := os.Stat(filepath.Join(r, "sub/subfile")) - require.Error(t, err) - require.True(t, os.IsNotExist(err)) - } + assert.Equal(t, "subcontents", string(b)) + } else { + _, err := os.Stat(filepath.Join(r, "sub/subfile")) + require.Error(t, err) + require.True(t, os.IsNotExist(err)) } b, err := ioutil.ReadFile(filepath.Join(r, "Dockerfile"))