From beb2825f8228265b7ea805e08f303a6844d4912a Mon Sep 17 00:00:00 2001 From: Anna Song Date: Tue, 8 Nov 2022 14:48:14 -0800 Subject: [PATCH] Improve readability of `ifc.Loader`'s `Repo()` method (#4857) * Improve ldr Repo() method readability * Change Repo() implementations and calls * Improve readability of conditions in ldr.New() * Fix details --- api/ifc/ifc.go | 6 +++--- api/internal/localizer/locloader.go | 12 ++++------- api/internal/localizer/locloader_test.go | 4 +--- api/internal/localizer/util.go | 13 ++++++++++-- api/loader/fileloader.go | 10 ++++----- api/loader/fileloader_test.go | 21 ++++++++----------- kustomize/commands/internal/util/util_test.go | 4 ++-- 7 files changed, 35 insertions(+), 35 deletions(-) diff --git a/api/ifc/ifc.go b/api/ifc/ifc.go index 4f47afb0a..4cd62dd59 100644 --- a/api/ifc/ifc.go +++ b/api/ifc/ifc.go @@ -29,9 +29,9 @@ type KvLoader interface { // Loader interface exposes methods to read bytes. type Loader interface { - // Repo returns the repo location and true if this Loader - // was created from a url; otherwise the empty string and false. - Repo() (string, bool) + // Repo returns the repo location if this Loader was created from a url + // or the empty string otherwise. + Repo() string // Root returns the root location for this Loader. Root() string diff --git a/api/internal/localizer/locloader.go b/api/internal/localizer/locloader.go index 76188847f..bc036a531 100644 --- a/api/internal/localizer/locloader.go +++ b/api/internal/localizer/locloader.go @@ -116,18 +116,12 @@ func (ll *locLoader) Load(path string) ([]byte, error) { // New returns a Loader at path if path is a valid localize root. // Otherwise, New returns an error. func (ll *locLoader) New(path string) (ifc.Loader, error) { - repoSpec, err := git.NewRepoSpecFromURL(path) - if err == nil && repoSpec.Ref == "" { - return nil, errors.Errorf("localize remote root '%s' missing ref query string parameter", path) - } - ldr, err := ll.Loader.New(path) if err != nil { return nil, errors.WrapPrefixf(err, "invalid root reference") } - var isRemote bool - if _, isRemote = ldr.Repo(); !isRemote { + if repo := ldr.Repo(); repo == "" { if ll.local && !filesys.ConfirmedDir(ldr.Root()).HasPrefix(ll.args.Scope) { return nil, errors.Errorf("root '%s' outside localize scope '%s'", ldr.Root(), ll.args.Scope) } @@ -135,12 +129,14 @@ func (ll *locLoader) New(path string) (ifc.Loader, error) { return nil, errors.Errorf( "root '%s' references into localize destination '%s'", ldr.Root(), ll.args.NewDir) } + } else if !hasRef(path) { + return nil, errors.Errorf("localize remote root %q missing ref query string parameter", path) } return &locLoader{ fSys: ll.fSys, args: ll.args, Loader: ldr, - local: ll.local && !isRemote, + local: ll.local && ldr.Repo() == "", }, nil } diff --git a/api/internal/localizer/locloader_test.go b/api/internal/localizer/locloader_test.go index 7d0f58858..e2e4a72bb 100644 --- a/api/internal/localizer/locloader_test.go +++ b/api/internal/localizer/locloader_test.go @@ -38,9 +38,7 @@ func checkNewLocLoader(req *require.Assertions, ldr ifc.Loader, args *lclzr.LocA func checkLoader(req *require.Assertions, ldr ifc.Loader, root string) { req.Equal(root, ldr.Root()) - repo, isRemote := ldr.Repo() - req.Equal(false, isRemote) - req.Equal("", repo) + req.Empty(ldr.Repo()) } func checkLocArgs(req *require.Assertions, args *lclzr.LocArgs, target string, scope string, newDir string, fSys filesys.FileSystem) { diff --git a/api/internal/localizer/util.go b/api/internal/localizer/util.go index 9c88b0912..bc14a9552 100644 --- a/api/internal/localizer/util.go +++ b/api/internal/localizer/util.go @@ -16,7 +16,7 @@ import ( // establishScope returns the scope given localize arguments and targetLdr at targetArg func establishScope(scopeArg string, targetArg string, targetLdr ifc.Loader, fSys filesys.FileSystem) (filesys.ConfirmedDir, error) { - if _, isRemote := targetLdr.Repo(); isRemote { + if targetLdr.Repo() != "" { if scopeArg != "" { return "", errors.Errorf("scope '%s' specified for remote localize target '%s'", scopeArg, targetArg) } @@ -65,7 +65,7 @@ func createNewDir(newDirArg string, targetLdr ifc.Loader, spec *git.RepoSpec, fS // and spec of target, which is nil if target is local func defaultNewDir(targetLdr ifc.Loader, spec *git.RepoSpec) string { targetDir := filepath.Base(targetLdr.Root()) - if repo, isRemote := targetLdr.Repo(); isRemote { + if repo := targetLdr.Repo(); repo != "" { // kustomize doesn't download repo into repo-named folder // must find repo folder name from url if repo == targetLdr.Root() { @@ -89,3 +89,12 @@ func urlBase(url string) string { } return cleaned[i+1:] } + +// hasRef checks if repoURL has ref query string parameter +func hasRef(repoURL string) bool { + repoSpec, err := git.NewRepoSpecFromURL(repoURL) + if err != nil { + log.Fatalf("unable to parse validated root url: %s", err.Error()) + } + return repoSpec.Ref != "" +} diff --git a/api/loader/fileloader.go b/api/loader/fileloader.go index 4bdb29652..82f62aff1 100644 --- a/api/loader/fileloader.go +++ b/api/loader/fileloader.go @@ -121,13 +121,13 @@ func NewFileLoaderAtRoot(fSys filesys.FileSystem) *fileLoader { RestrictionRootOnly, fSys, filesys.Separator) } -// Repo returns the absolute path to the repo that contains Root and true -// if this fileLoader was created from a url; otherwise, the empty string and false -func (fl *fileLoader) Repo() (string, bool) { +// Repo returns the absolute path to the repo that contains Root if this fileLoader was created from a url +// or the empty string otherwise. +func (fl *fileLoader) Repo() string { if fl.repoSpec != nil { - return fl.repoSpec.Dir.String(), true + return fl.repoSpec.Dir.String() } - return "", false + return "" } // Root returns the absolute path that is prepended to any diff --git a/api/loader/fileloader_test.go b/api/loader/fileloader_test.go index e22a51097..a13ca62be 100644 --- a/api/loader/fileloader_test.go +++ b/api/loader/fileloader_test.go @@ -95,8 +95,8 @@ func TestLoaderLoad(t *testing.T) { require := require.New(t) l1 := makeLoader() - _, remote := l1.Repo() - require.False(remote) + repo := l1.Repo() + require.Empty(repo) require.Equal("/", l1.Root()) for _, x := range testCases { @@ -110,8 +110,8 @@ func TestLoaderLoad(t *testing.T) { l2, err := l1.New("foo/project") require.NoError(err) - _, remote = l2.Repo() - require.False(remote) + repo = l2.Repo() + require.Empty(repo) require.Equal("/foo/project", l2.Root()) for _, x := range testCases { @@ -361,8 +361,7 @@ whatever repoSpec, fSys, nil, git.DoNothingCloner(filesys.ConfirmedDir(coRoot))) require.NoError(err) - repo, remote := l.Repo() - require.True(remote) + repo := l.Repo() require.Equal(coRoot, repo) require.Equal(coRoot+"/"+pathInRepo, l.Root()) @@ -378,8 +377,7 @@ whatever l2, err := l.New(url) require.NoError(err) - repo, remote = l2.Repo() - require.True(remote) + repo = l2.Repo() require.Equal(coRoot, repo) require.Equal(coRoot+"/"+pathInRepo, l2.Root()) } @@ -435,8 +433,8 @@ func TestLoaderDisallowsLocalBaseFromRemoteOverlay(t *testing.T) { // This is okay. l2, err = l1.New("../base") require.NoError(err) - _, remote := l2.Repo() - require.False(remote) + repo := l2.Repo() + require.Empty(repo) require.Equal(cloneRoot+"/foo/base", l2.Root()) // This is not okay. @@ -462,8 +460,7 @@ func TestLocalLoaderReferencingGitBase(t *testing.T) { l2, err := l1.New("github.com/someOrg/someRepo/foo/base") require.NoError(err) - repo, remote := l2.Repo() - require.True(remote) + repo := l2.Repo() require.Equal(cloneRoot, repo) require.Equal(cloneRoot+"/foo/base", l2.Root()) } diff --git a/kustomize/commands/internal/util/util_test.go b/kustomize/commands/internal/util/util_test.go index 2c1ba0c1e..21c2e70c3 100644 --- a/kustomize/commands/internal/util/util_test.go +++ b/kustomize/commands/internal/util/util_test.go @@ -102,8 +102,8 @@ type fakeLoader struct { path string } -func (l fakeLoader) Repo() (string, bool) { - return "", false +func (l fakeLoader) Repo() string { + return "" } func (l fakeLoader) Root() string { return ""