-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
Rewrite remoteload_test integration tests #4783
Changes from 7 commits
a249b04
ddc6825
185ec88
1287caf
74cc1c0
775b7a1
0963a2a
3667068
020fec9
3217f9a
4c0bf39
215455c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,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}) | ||
|
@@ -285,6 +286,79 @@ func TestNewRepoSpecFromUrl_Smoke(t *testing.T) { | |
GitSuffix: ".git", | ||
}, | ||
}, | ||
{ | ||
name: "t15", | ||
input: "https://github.com/kubernetes-sigs/kustomize//examples/multibases/dev/?submodules=0&ref=v1.0.6&timeout=300", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a comment in L117 above that says
Does this test change that? I'm wondering if we should remove that comment now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is true. I copied this test from elsewhere and left the query params, but it is true that they were "inactive" in the test. I will remove the query params here. |
||
cloneSpec: "https://github.com/kubernetes-sigs/kustomize.git", | ||
absPath: notCloned.Join("/examples/multibases/dev"), | ||
repoSpec: RepoSpec{ | ||
Host: "https://github.com/", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember you mentioning in another PR that we should probably rename There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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", | ||
}, | ||
}, | ||
} | ||
for _, tc := range testcases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor wording nit