diff --git a/pkg/git/cloner.go b/pkg/git/cloner.go index 40c61ffba..465fdb1d1 100644 --- a/pkg/git/cloner.go +++ b/pkg/git/cloner.go @@ -25,23 +25,19 @@ import ( ) // Cloner is a function that can clone a git repo. -type Cloner func(url string) (*RepoSpec, error) +type Cloner func(repoSpec *RepoSpec) error // ClonerUsingGitExec uses a local git install, as opposed // to say, some remote API, to obtain a local clone of // a remote repo. -func ClonerUsingGitExec(spec string) (*RepoSpec, error) { +func ClonerUsingGitExec(repoSpec *RepoSpec) error { gitProgram, err := exec.LookPath("git") if err != nil { - return nil, errors.Wrap(err, "no 'git' program on path") - } - repoSpec, err := NewRepoSpecFromUrl(spec) - if err != nil { - return nil, err + return errors.Wrap(err, "no 'git' program on path") } repoSpec.cloneDir, err = fs.NewTmpConfirmedDir() if err != nil { - return nil, err + return err } cmd := exec.Command( gitProgram, @@ -52,17 +48,28 @@ func ClonerUsingGitExec(spec string) (*RepoSpec, error) { cmd.Stdout = &out err = cmd.Run() if err != nil { - return nil, errors.Wrapf(err, "trouble cloning %s", spec) + return errors.Wrapf(err, "trouble cloning %s", repoSpec.raw) } if repoSpec.ref == "" { - return repoSpec, nil + return nil } cmd = exec.Command(gitProgram, "checkout", repoSpec.ref) cmd.Dir = repoSpec.cloneDir.String() err = cmd.Run() if err != nil { - return nil, errors.Wrapf( + return errors.Wrapf( err, "trouble checking out href %s", repoSpec.ref) } - return repoSpec, nil + return nil +} + +// DoNothingCloner returns a cloner that only sets +// cloneDir field in the repoSpec. It's assumed that +// the cloneDir is associated with some fake filesystem +// used in a test. +func DoNothingCloner(dir fs.ConfirmedDir) Cloner { + return func(rs *RepoSpec) error { + rs.cloneDir = dir + return nil + } } diff --git a/pkg/loader/fileloader.go b/pkg/loader/fileloader.go index 11138eb17..97da3a117 100644 --- a/pkg/loader/fileloader.go +++ b/pkg/loader/fileloader.go @@ -114,44 +114,45 @@ func (l *fileLoader) Root() string { } func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader { - l, err := newLoaderAtConfirmedDir( - path, fSys, nil, git.ClonerUsingGitExec) + root, err := demandDirectoryRoot(fSys, path) if err != nil { log.Fatalf("unable to make loader at '%s'; %v", path, err) } - return l + return newLoaderAtConfirmedDir( + root, fSys, nil, git.ClonerUsingGitExec) } // newLoaderAtConfirmedDir returns a new fileLoader with given root. func newLoaderAtConfirmedDir( - possibleRoot string, fSys fs.FileSystem, - referrer *fileLoader, cloner git.Cloner) (*fileLoader, error) { - if possibleRoot == "" { - return nil, fmt.Errorf( - "loader root cannot be empty") - } - root, f, err := fSys.CleanedAbs(possibleRoot) - if err != nil { - return nil, fmt.Errorf( - "absolute path error in '%s' : %v", possibleRoot, err) - } - if f != "" { - return nil, fmt.Errorf( - "got file '%s', but '%s' must be a directory to be a root", - f, possibleRoot) - } - if referrer != nil { - if err := referrer.errIfArgEqualOrHigher(root); err != nil { - return nil, err - } - } + root fs.ConfirmedDir, fSys fs.FileSystem, + referrer *fileLoader, cloner git.Cloner) *fileLoader { return &fileLoader{ root: root, referrer: referrer, fSys: fSys, cloner: cloner, cleaner: func() error { return nil }, - }, nil + } +} + +// Assure that the given path is in fact a directory. +func demandDirectoryRoot( + fSys fs.FileSystem, path string) (fs.ConfirmedDir, error) { + if path == "" { + return "", fmt.Errorf( + "loader root cannot be empty") + } + d, f, err := fSys.CleanedAbs(path) + if err != nil { + return "", fmt.Errorf( + "absolute path error in '%s' : %v", path, err) + } + if f != "" { + return "", fmt.Errorf( + "got file '%s', but '%s' must be a directory to be a root", + f, path) + } + return d, nil } // New returns a new Loader, rooted relative to current loader, @@ -160,26 +161,34 @@ func (l *fileLoader) New(path string) (ifc.Loader, error) { if path == "" { return nil, fmt.Errorf("new root cannot be empty") } - if git.IsRepoUrl(path) { - // Avoid cycles. - if err := l.errIfPreviouslySeenUri(path); err != nil { + repoSpec, err := git.NewRepoSpecFromUrl(path) + if err == nil { + // Treat this as git repo clone request. + if err := l.errIfRepoCycle(repoSpec); err != nil { return nil, err } - return newLoaderAtGitClone(path, l.fSys, l.referrer, l.cloner) + return newLoaderAtGitClone(repoSpec, l.fSys, l.referrer, l.cloner) } if filepath.IsAbs(path) { return nil, fmt.Errorf("new root '%s' cannot be absolute", path) } + root, err := demandDirectoryRoot(l.fSys, l.root.Join(path)) + if err != nil { + return nil, err + } + if err := l.errIfArgEqualOrHigher(root); err != nil { + return nil, err + } return newLoaderAtConfirmedDir( - l.root.Join(path), l.fSys, l, l.cloner) + root, l.fSys, l, l.cloner), nil } // newLoaderAtGitClone returns a new Loader pinned to a temporary // directory holding a cloned git repo. func newLoaderAtGitClone( - uri string, fSys fs.FileSystem, + repoSpec *git.RepoSpec, fSys fs.FileSystem, referrer *fileLoader, cloner git.Cloner) (ifc.Loader, error) { - repoSpec, err := cloner(uri) + err := cloner(repoSpec) if err != nil { return nil, err } @@ -187,6 +196,10 @@ func newLoaderAtGitClone( if err != nil { return nil, err } + // We don't know that the path requested in repoSpec + // is a directory until we actually clone it and look + // inside. That just happened, hence the error check + // is here. if f != "" { return nil, fmt.Errorf( "'%s' refers to file '%s'; expecting directory", @@ -221,17 +234,17 @@ func (l *fileLoader) errIfArgEqualOrHigher( // I.e. Allow a distinction between git URI with // path foo and tag bar and a git URI with the same // path but a different tag? -// TODO(monopole): Use parsed data instead of looking at Raw(). -func (l *fileLoader) errIfPreviouslySeenUri(uri string) error { - if strings.HasPrefix(l.repoSpec.Raw(), uri) { +func (l *fileLoader) errIfRepoCycle(newRepoSpec *git.RepoSpec) error { + // TODO(monopole): Use parsed data instead of Raw(). + if strings.HasPrefix(l.repoSpec.Raw(), newRepoSpec.Raw()) { return fmt.Errorf( "cycle detected: URI '%s' referenced by previous URI '%s'", - uri, l.repoSpec.Raw()) + newRepoSpec.Raw(), l.repoSpec.Raw()) } if l.referrer == nil { return nil } - return l.referrer.errIfPreviouslySeenUri(uri) + return l.referrer.errIfRepoCycle(newRepoSpec) } // Load returns content of file at the given relative path, diff --git a/pkg/loader/fileloader_test.go b/pkg/loader/fileloader_test.go index bf27c487b..f02552b8f 100644 --- a/pkg/loader/fileloader_test.go +++ b/pkg/loader/fileloader_test.go @@ -64,25 +64,6 @@ func MakeFakeFs(td []testData) fs.FileSystem { return fSys } -func TestNewLoaderAtConfirmedDir_DemandsDirectory(t *testing.T) { - fSys := MakeFakeFs(testCases) - _, err := newLoaderAtConfirmedDir("/foo", fSys, nil, nil) - if err != nil { - t.Fatalf("Unexpected error - a directory should work.") - } - _, err = newLoaderAtConfirmedDir("/foo/project", fSys, nil, nil) - if err != nil { - t.Fatalf("Unexpected error - a directory should work.") - } - _, err = newLoaderAtConfirmedDir("/foo/project/fileA.yaml", fSys, nil, nil) - if err == nil { - t.Fatalf("Expected error - a file should not work.") - } - if !strings.Contains(err.Error(), "must be a directory to be a root") { - t.Fatalf("unexpected err: %v", err) - } -} - func TestLoaderLoad(t *testing.T) { l1 := NewFileLoaderAtRoot(MakeFakeFs(testCases)) if "/" != l1.Root() { @@ -351,23 +332,6 @@ func TestSplit(t *testing.T) { } } -// makeFakeGitCloner returns a cloner that ignores the -// URL argument and returns a path in a fake file system -// that should already hold the 'repo' contents. -func makeFakeGitCloner(t *testing.T, fSys fs.FileSystem, coRoot string) git.Cloner { - if !fSys.IsDir(coRoot) { - t.Fatalf("expecting a directory at '%s'", coRoot) - } - return func(url string) (*git.RepoSpec, error) { - _, path := splitOnNthSlash(url, 3) - if !fSys.IsDir(coRoot + "/" + path) { - t.Fatalf("expecting a directory at '%s'/'%s'", - coRoot, path) - } - return git.NewRepoSpec(url, fs.ConfirmedDir(coRoot), path), nil - } -} - func TestNewLoaderAtGitClone(t *testing.T) { rootUrl := "github.com/someOrg/someRepo" pathInRepo := "foo/base" @@ -385,9 +349,14 @@ func TestNewLoaderAtGitClone(t *testing.T) { []byte(` whatever `)) + + repoSpec, err := git.NewRepoSpecFromUrl(url) + if err != nil { + t.Fatalf("unexpected err: %v\n", err) + } l, err := newLoaderAtGitClone( - url, fSys, nil, - makeFakeGitCloner(t, fSys, coRoot)) + repoSpec, fSys, nil, + git.DoNothingCloner(fs.ConfirmedDir(coRoot))) if err != nil { t.Fatalf("unexpected err: %v\n", err) } diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index 5e55ddfc1..53de6553a 100644 --- a/pkg/loader/loader.go +++ b/pkg/loader/loader.go @@ -24,11 +24,16 @@ import ( ) // NewLoader returns a Loader. -func NewLoader(root string, fSys fs.FileSystem) (ifc.Loader, error) { - if git.IsRepoUrl(root) { +func NewLoader(path string, fSys fs.FileSystem) (ifc.Loader, error) { + repoSpec, err := git.NewRepoSpecFromUrl(path) + if err == nil { return newLoaderAtGitClone( - root, fSys, nil, git.ClonerUsingGitExec) + repoSpec, fSys, nil, git.ClonerUsingGitExec) + } + root, err := demandDirectoryRoot(fSys, path) + if err != nil { + return nil, err } return newLoaderAtConfirmedDir( - root, fSys, nil, git.ClonerUsingGitExec) + root, fSys, nil, git.ClonerUsingGitExec), nil }