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 9 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/server",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OrgRepo: "path/to/server",
OrgRepo: "path/to/repo",

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops!

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
11 changes: 8 additions & 3 deletions api/internal/target/kusttarget.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,11 +412,16 @@ func (kt *KustTarget) accumulateResources(
}
ldr, err := kt.ldr.New(path)
if err != nil {
if kusterr.IsMalformedYAMLError(errF) { // Some error occurred while tyring to decode YAML file
return nil, errF
if strings.Contains(err.Error(), load.ErrRtNotDir.Error()) { // Was neither a remote resource nor a local directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed by this PR? Unfortunately we've caused regressions in the past by misunderstanding the possible error conditions/sources here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was incredibly difficult to debug remote load errors without this, since it swallows the actual failure of the remote load and instead prints out symlink eval failure. I think this is a significant improvement and I'd like to include it, though I could be convinced to separate it into a separate PR.

I was very careful to look through the code paths. I think you can see here that, an error will still always be returned. The content and type of the error may change. From what I can see from the execution path, this means that if there is a regression, at worst it means the wrong error message shows up, which may not be terrible.

Also, I originally made a much smaller change here that failed 2 regression tests that checked the error message. The result of fixing those is what you see here. I think it's fairly safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked through the code paths again with fresh eyes, and although I agree that the regression here would be with the error message, I think there are several cases where it would happen. Essentially, we know at this point that the target was not a valid individual file (local or remote), and that it is also not a valid base (local or remote), but it may still be a valid directory or an invalid file. Examples of cases where this PR currently changes the error message and should not:

  • Path is blank (will incorrectly say it was accumulating a remote resource)
  • Path is absolute (will incorrectly say it was accumulating a remote resource)
  • Path is a remote file containing invalid YAML (the yaml error will now be swallowed) <-- this is the worst one, try https://github.com/kubernetes-sigs/kustomize/blob/master/ARCHITECTURE.md?submodules=false to see it for example
  • Path creates a reference cycle (will say, sometimes incorrectly, it was accumulating a remote resource)

This code is complicated, also under-covered, and generates bad error messages for a lot of cases, including the one you pointed out here. I would love to see it improve, but in light of the above I would like to remove changes to it from this PR so we can get this merged.

if kusterr.IsMalformedYAMLError(errF) {
// Some error occurred while tyring to decode YAML file
return nil, errF
}
return nil, errors.Wrapf(
err, "accumulation err='%s'", errF.Error())
}
return nil, errors.Wrapf(
err, "accumulation err='%s'", errF.Error())
err, "accumulating remote resource: %s", path)
}
// store the origin, we'll need it later
origin := kt.origin.Copy()
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