Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rewrite remoteload_test integration tests #4783

Merged
merged 12 commits into from
Sep 19, 2022
Merged

Rewrite remoteload_test integration tests #4783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a249b04
Better error message when git clone fails
mightyguava Aug 29, 2022
ddc6825
support file:// URLs
mightyguava Aug 31, 2022
185ec88
rewrite remoteload_test
mightyguava Aug 31, 2022
1287caf
lint and test fix
mightyguava Aug 31, 2022
74cc1c0
fixes for kverey's comments
mightyguava Sep 8, 2022
775b7a1
document file:// remote load
mightyguava Sep 8, 2022
0963a2a
replace assert with require where appropriate
mightyguava Sep 8, 2022
3667068
add tests for file:// without git suffix
mightyguava Sep 8, 2022
020fec9
fixes plus pr review from natasha
mightyguava Sep 16, 2022
3217f9a
fixes for review, lint
mightyguava Sep 17, 2022
4c0bf39
revert changes to error handling
mightyguava Sep 17, 2022
215455c
fix skipped test
mightyguava Sep 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions api/internal/git/cloner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,11 @@ func ClonerUsingGitExec(repoSpec *RepoSpec) error {
if err = r.run("init"); err != nil {
return err
}
if err = r.run(
"remote", "add", "origin", repoSpec.CloneSpec()); err != nil {
return err
}
ref := "HEAD"
if repoSpec.Ref != "" {
ref = repoSpec.Ref
}
if err = r.run("fetch", "--depth=1", "origin", ref); err != nil {
if err = r.run("fetch", "--depth=1", repoSpec.CloneSpec(), ref); err != nil {
return err
}
if err = r.run("checkout", "FETCH_HEAD"); err != nil {
Expand Down
4 changes: 2 additions & 2 deletions api/internal/git/gitrunner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ func (r gitRunner) run(args ...string) error {
cmd.String(),
r.duration,
func() error {
_, err := cmd.CombinedOutput()
out, err := cmd.CombinedOutput()
if err != nil {
return errors.Wrapf(err, "git cmd = '%s'", cmd.String())
return errors.Wrapf(err, "failed to run '%s': %s", cmd.String(), string(out))
}
return err
})
Expand Down
21 changes: 19 additions & 2 deletions api/internal/git/repospec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,12 @@ func parseGitURL(n string) (
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):]
Expand All @@ -131,6 +135,19 @@ func parseGitURL(n string) (
return
}

if isLocal {
if idx := strings.Index(n, "//"); idx > 0 {
orgRepo = n[:idx]
n = n[idx+2:]
path, gitRef, gitTimeout, gitSubmodules = peelQuery(n)
return
}
path, gitRef, gitTimeout, gitSubmodules = peelQuery(n)
orgRepo = path
path = ""
return
}

i := strings.Index(n, "/")
if i < 1 {
path, gitRef, gitTimeout, gitSubmodules = peelQuery(n)
Expand Down Expand Up @@ -200,7 +217,7 @@ func parseHostSpec(n string) (string, string) {
// Start accumulating the host part.
for _, p := range []string{
// Order matters here.
"git::", "gh:", "ssh://", "https://", "http://",
"git::", "gh:", "ssh://", "https://", "http://", "file://",
"git@", "github.com:", "github.com/"} {
if len(p) < len(n) && strings.ToLower(n[:len(p)]) == p {
n = n[len(p):]
Expand Down
128 changes: 127 additions & 1 deletion api/internal/git/repospec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestNewRepoSpecFromUrl_Permute(t *testing.T) {
Expand Down Expand Up @@ -61,6 +62,7 @@ func TestNewRepoSpecFromUrl_Permute(t *testing.T) {
rs, err := NewRepoSpecFromURL(uri)
if err != nil {
t.Errorf("problem %v", err)
continue
}
if rs.Host != hostSpec {
bad = append(bad, []string{"host", uri, rs.Host, hostSpec})
Expand Down Expand Up @@ -120,6 +122,7 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) {
repoSpec RepoSpec
cloneSpec string
absPath string
skip string
}{
{
name: "t1",
Expand Down Expand Up @@ -285,11 +288,134 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) {
GitSuffix: ".git",
},
},
{
name: "t15",
input: "https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?ref=v1.0.6",
cloneSpec: "https://github.com/kubernetes-sigs/kustomize.git",
absPath: notCloned.Join("/examples/multibases/dev"),
repoSpec: RepoSpec{
Host: "https://github.com/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember you mentioning in another PR that we should probably rename RepoSpec.Host to RepoSpec.SchemeHost. I still think that's a good suggestion, and we can include it in this PR if you'd like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually thought that it was a behavior change the previous PR or recent PR added. I was wrong. This seems like the Host hasn't meant that for a long time. So I'm actually more comfortable now with doing that in a separate PR haha. Thanks for calling it out and being so open about it though!

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",
KnVerey marked this conversation as resolved.
Show resolved Hide resolved
cloneSpec: "file://a/b/c/someRepo.git",
absPath: notCloned.Join("somepath"),
repoSpec: RepoSpec{
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",
},
},
{
name: "t21",
input: "file:///a/b/c/someRepo",
cloneSpec: "file:///a/b/c/someRepo",
absPath: notCloned.String(),
repoSpec: RepoSpec{
Host: "file://",
OrgRepo: "/a/b/c/someRepo",
},
},
{
name: "t22",
input: "file:///",
cloneSpec: "file:///",
absPath: notCloned.String(),
repoSpec: RepoSpec{
Host: "file://",
OrgRepo: "/",
},
},
{
name: "t23",
skip: "the `//` repo separator does not work",
input: "https://fake-git-hosting.org/path/to/repo//examples/multibases/dev",
cloneSpec: "https://fake-git-hosting.org/path/to.git",
absPath: notCloned.Join("/examples/multibases/dev"),
repoSpec: RepoSpec{
Host: "https://fake-git-hosting.org/",
OrgRepo: "path/to/repo",
Path: "/examples/multibases/dev",
GitSuffix: ".git",
},
},
{
name: "t24",
skip: "the `//` repo separator does not work",
input: "ssh://alice@acme.co/path/to/repo//examples/multibases/dev",
cloneSpec: "ssh://alice@acme.co/path/to/repo.git",
absPath: notCloned.Join("/examples/multibases/dev"),
repoSpec: RepoSpec{
Host: "ssh://alice@acme.co",
OrgRepo: "path/to/repo",
Path: "/examples/multibases/dev",
GitSuffix: ".git",
},
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
if tc.skip != "" {
t.Skip(tc.skip)
}

rs, err := NewRepoSpecFromURL(tc.input)
assert.NoError(t, err)
require.NoError(t, err)
assert.Equal(t, tc.cloneSpec, rs.CloneSpec(), "cloneSpec mismatch")
assert.Equal(t, tc.absPath, rs.AbsPath(), "absPath mismatch")
// some values have defaults. Clear them here so test cases remain compact.
Expand Down
2 changes: 1 addition & 1 deletion api/krusty/originannotation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ buildMetadata: [originAnnotations]
metadata:
annotations:
config.kubernetes.io/origin: |
repo: https://github.com/kubernetes-sigs/kustomize
repo: https://github.com/kubernetes-sigs/kustomize.git
ref: v1.0.6
configuredIn: examples/ldap/base/kustomization.yaml
configuredBy:
Expand Down
Loading