From 74cc1c031c3232cf4a47411d21a56da8cacaa3d3 Mon Sep 17 00:00:00 2001 From: Yunchi Luo Date: Thu, 8 Sep 2022 13:08:02 -0400 Subject: [PATCH] fixes for kverey's comments --- api/internal/git/repospec.go | 36 ++++++------ api/internal/git/repospec_test.go | 65 ++++++++++++++++++++- api/krusty/remoteloader_test.go | 94 +++++++++++++++++++++++++++---- api/resource/origin.go | 2 +- 4 files changed, 168 insertions(+), 29 deletions(-) diff --git a/api/internal/git/repospec.go b/api/internal/git/repospec.go index 3feaaa252f..ee15b95ce4 100644 --- a/api/internal/git/repospec.go +++ b/api/internal/git/repospec.go @@ -87,7 +87,7 @@ func NewRepoSpecFromURL(n string) (*RepoSpec, error) { return nil, fmt.Errorf("uri looks like abs path: %s", n) } host, orgRepo, path, gitRef, gitSubmodules, suffix, gitTimeout := parseGitURL(n) - if orgRepo == "" && !strings.HasPrefix(host, fileScheme) { + if orgRepo == "" { return nil, fmt.Errorf("url lacks orgRepo: %s", n) } if host == "" { @@ -119,23 +119,13 @@ func parseGitURL(n string) ( path, gitRef, gitTimeout, gitSubmodules = peelQuery(n[index+len(gitDelimiter)+len(orgRepo):]) return } - if strings.HasPrefix(n, fileScheme) { - idx := strings.Index(n, gitSuffix) - if idx == -1 { - // invalid, must have .git somewhere - return - } - host += n[:idx+len(gitSuffix)] - n = n[idx+len(gitSuffix):] - if len(n) > 0 && n[0] == '/' { - n = n[1:] - } - path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) - return - } host, n = parseHostSpec(n) - gitSuff = gitSuffix + isLocal := strings.HasPrefix(host, "file://") + if !isLocal { + gitSuff = gitSuffix + } if strings.Contains(n, gitSuffix) { + gitSuff = gitSuffix index := strings.Index(n, gitSuffix) orgRepo = n[0:index] n = n[index+len(gitSuffix):] @@ -146,6 +136,20 @@ func parseGitURL(n string) ( return } + if isLocal { + if idx := strings.Index(n, "//"); idx > 0 { + orgRepo = n[:idx] + n = n[idx+2:] + } else if q := strings.Index(n, "?"); q != -1 { + orgRepo = n[:q] + n = n[q:] + } else { + orgRepo = n + } + path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) + return + } + i := strings.Index(n, "/") if i < 1 { path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) diff --git a/api/internal/git/repospec_test.go b/api/internal/git/repospec_test.go index 95d9aaf089..df74a95101 100644 --- a/api/internal/git/repospec_test.go +++ b/api/internal/git/repospec_test.go @@ -288,16 +288,77 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { }, { name: "t15", + input: "https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?submodules=0&ref=v1.0.6&timeout=300", + cloneSpec: "https://github.com/kubernetes-sigs/kustomize.git", + absPath: notCloned.Join("/examples/multibases/dev"), + repoSpec: RepoSpec{ + Host: "https://github.com/", + OrgRepo: "kubernetes-sigs/kustomize", + Path: "/examples/multibases/dev/", + Ref: "v1.0.6", + GitSuffix: ".git", + }, + }, + { + name: "t16", input: "file://a/b/c/someRepo.git/somepath?ref=someBranch", cloneSpec: "file://a/b/c/someRepo.git", absPath: notCloned.Join("somepath"), repoSpec: RepoSpec{ - Host: "file://a/b/c/someRepo.git", - OrgRepo: "", + Host: "file://", + OrgRepo: "a/b/c/someRepo", + Path: "somepath", + Ref: "someBranch", + GitSuffix: ".git", + }, + }, + { + name: "t17", + input: "file://a/b/c/someRepo//somepath?ref=someBranch", + cloneSpec: "file://a/b/c/someRepo", + absPath: notCloned.Join("somepath"), + repoSpec: RepoSpec{ + Host: "file://", + OrgRepo: "a/b/c/someRepo", Path: "somepath", Ref: "someBranch", }, }, + { + name: "t18", + input: "file://a/b/c/someRepo?ref=someBranch", + cloneSpec: "file://a/b/c/someRepo", + absPath: notCloned.String(), + repoSpec: RepoSpec{ + Host: "file://", + OrgRepo: "a/b/c/someRepo", + Ref: "someBranch", + }, + }, + { + name: "t19", + input: "file:///a/b/c/someRepo?ref=someBranch", + cloneSpec: "file:///a/b/c/someRepo", + absPath: notCloned.String(), + repoSpec: RepoSpec{ + Host: "file://", + OrgRepo: "/a/b/c/someRepo", + Ref: "someBranch", + }, + }, + { + name: "t20", + input: "ssh://git@github.com/kubernetes-sigs/kustomize//examples/multibases/dev?ref=v1.0.6", + cloneSpec: "git@github.com:kubernetes-sigs/kustomize.git", + absPath: notCloned.Join("examples/multibases/dev"), + repoSpec: RepoSpec{ + Host: "git@github.com:", + OrgRepo: "kubernetes-sigs/kustomize", + Path: "/examples/multibases/dev", + Ref: "v1.0.6", + GitSuffix: ".git", + }, + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { diff --git a/api/krusty/remoteloader_test.go b/api/krusty/remoteloader_test.go index 182adfb399..189db0cdb1 100644 --- a/api/krusty/remoteloader_test.go +++ b/api/krusty/remoteloader_test.go @@ -17,6 +17,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sigs.k8s.io/kustomize/api/krusty" + "sigs.k8s.io/kustomize/api/loader" "sigs.k8s.io/kustomize/api/resmap" "sigs.k8s.io/kustomize/kyaml/filesys" "sigs.k8s.io/kustomize/kyaml/yaml" @@ -107,6 +108,7 @@ spec: - image: nginx:1.7.9 name: nginx ` + var simpleBuildWithNginx2 = strings.ReplaceAll(simpleBuild, "nginx:1.7.9", "nginx:2") var multibaseDevExampleBuild = strings.ReplaceAll(simpleBuild, "myapp-pod", "dev-myapp-pod") repos := createGitRepos(t) @@ -140,7 +142,7 @@ resources: - "file://$ROOT/simple.git?ref=change-image" `, - expected: strings.ReplaceAll(simpleBuild, "nginx:1.7.9", "nginx:2"), + expected: simpleBuildWithNginx2, }, { // Version is the same as ref @@ -149,13 +151,29 @@ resources: resources: - file://$ROOT/simple.git?version=change-image `, - expected: strings.ReplaceAll(simpleBuild, "nginx:1.7.9", "nginx:2"), + expected: simpleBuildWithNginx2, }, { name: "has submodule", kustomization: ` resources: - file://$ROOT/with-submodule.git/submodule +`, + expected: simpleBuild, + }, + { + name: "has timeout", + kustomization: ` +resources: +- file://$ROOT/simple.git?timeout=500 +`, + expected: simpleBuild, + }, + { + name: "triple slash absolute path", + kustomization: ` +resources: +- file:///$ROOT/simple.git `, expected: simpleBuild, }, @@ -165,7 +183,7 @@ resources: resources: - file://$ROOT/with-submodule.git/submodule?submodules=0 `, - err: "unable to find", + err: "unable to find one of 'kustomization.yaml', 'kustomization.yml' or 'Kustomization' in directory", }, { name: "has origin annotation", @@ -192,10 +210,10 @@ spec: `, }, { - name: "has ref path and origin annotation", + name: "has ref path timeout and origin annotation", kustomization: ` resources: -- file://$ROOT/multibase.git/dev?version=main +- file://$ROOT/multibase.git/dev?version=main&timeout=500 buildMetadata: [originAnnotations] `, expected: `apiVersion: v1 @@ -215,6 +233,14 @@ spec: name: nginx `, }, + { + name: "repo does not exist ", + kustomization: ` +resources: +- file:///not/a/real/repo +`, + err: "fatal: '/not/a/real/repo' does not appear to be a git repository", + }, } for _, test := range tests { @@ -248,17 +274,26 @@ spec: func TestRemoteLoad_RemoteProtocols(t *testing.T) { // Slow remote tests with long timeouts. - // TODO: Maybe they should retry too. + // TODO: If these end up flaking, they should retry. If not, remove this TODO. tests := []struct { name string kustomization string err string + errT error + beforeTest func(t *testing.T, ) }{ { name: "https", kustomization: ` resources: -- https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?submodules=0&ref=v1.0.6&timeout=300 +- https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?submodules=0&ref=kustomize%2Fv4.5.7&timeout=300 +`, + }, + { + name: "git double-colon https", + kustomization: ` +resources: +- git::https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?submodules=0&ref=kustomize%2Fv4.5.7&timeout=300 `, }, { @@ -270,17 +305,53 @@ namePrefix: dev- `, }, { - name: "ssh", + name: "ssh", + beforeTest: configureGitSSHCommand, kustomization: ` resources: -- ssh://git@github.com/kubernetes-sigs/kustomize/examples/multibases/dev?submodules=0&ref=v1.0.6&timeout=300 +- git@github.com/kubernetes-sigs/kustomize/examples/multibases/dev?submodules=0&ref=kustomize%2Fv4.5.7&timeout=300 `, }, + { + name: "ssh with colon", + beforeTest: configureGitSSHCommand, + kustomization: ` +resources: +- git@github.com:kubernetes-sigs/kustomize/examples/multibases/dev?submodules=0&ref=kustomize%2Fv4.5.7&timeout=300 +`, + }, + { + name: "ssh without username", + beforeTest: configureGitSSHCommand, + kustomization: ` +resources: +- github.com/kubernetes-sigs/kustomize/examples/multibases/dev?submodules=0&ref=kustomize%2Fv4.5.7&timeout=300 +`, + }, + { + name: "ssh scheme", + beforeTest: configureGitSSHCommand, + kustomization: ` +resources: +- ssh://git@github.com/kubernetes-sigs/kustomize/examples/multibases/dev?submodules=0&ref=kustomize%2Fv4.5.7&timeout=300 +`, + }, + { + name: "http error", + kustomization: ` +resources: +- https://github.com/thisisa404.yaml +`, + err: "accumulating resources: accumulating resources from 'https://github.com/thisisa404.yaml': HTTP Error: status code 404 (Not Found)", + errT: loader.ErrHTTP, + }, } - configureGitSSHCommand(t) for _, test := range tests { t.Run(test.name, func(t *testing.T) { + if test.beforeTest != nil { + test.beforeTest(t) + } fSys, tmpDir := createKustDir(t, test.kustomization) b := krusty.MakeKustomizer(krusty.MakeDefaultOptions()) @@ -293,6 +364,9 @@ resources: if err != nil { assert.Contains(t, err.Error(), test.err) } + if test.errT != nil { + assert.ErrorIs(t, err, test.errT) + } } else { assert.NoError(t, err) if err == nil { diff --git a/api/resource/origin.go b/api/resource/origin.go index 3f4cfe3406..f0a4ae75d5 100644 --- a/api/resource/origin.go +++ b/api/resource/origin.go @@ -52,7 +52,7 @@ func (origin *Origin) Append(path string) *Origin { originCopy := origin.Copy() repoSpec, err := git.NewRepoSpecFromURL(path) if err == nil { - originCopy.Repo = repoSpec.Host + repoSpec.OrgRepo + originCopy.Repo = repoSpec.CloneSpec() absPath := repoSpec.AbsPath() path = absPath[strings.Index(absPath[1:], "/")+1:][1:] originCopy.Path = ""