Rewrite remoteload_test integration tests #4783

mightyguava · 2022-08-31T16:43:59Z

This PR rewrites remoteload_test.go based on discussion in #4640.

There are now a suite of tests exercising supported URL params using the file:// protocol, and a smaller set of tests with long timeouts for exercising actual remote fetches.

Behavior Change

The file:// protocol was not previously supported. I made a few small changes to repospec to support it. It's a bit hacked but to do it better would require a lot larger change that I do not want to do in this PR.
It adds better error messaging when remote loading fails. Instead of the previous error:

accumulating resources: accumulation err='accumulating resources from 'git@github.com:kubernetes-sigs/kustomiz//examples/multibases/dev': evalsymlink failure on '/private/var/folders/9k/72l1zfg94fl9p5c3jdrv0p_40000gn/T/kustomize-868128946/git@github.com:kubernetes-sigs/kustomiz/examples/multibases/dev' : lstat /private/var/folders/9k/72l1zfg94fl9p5c3jdrv0p_40000gn/T/kustomize-868128946/git@github.com:kubernetes-sigs: no such file or directory': git cmd = '/opt/homebrew/bin/git fetch --depth=1 origin HEAD': exit status 128

it will now show the output of the git command that failed as well, something like the below.

accumulating resources: accumulating remote resource: git@github.com:kubernetes-sigs/kustomiz//examples/multibases/dev?submodules=0: git cmd = '/opt/homebrew/bin/git fetch --depth=1 git@github.com:kubernetes-sigs/kustomiz.git HEAD' failed:
ERROR: Repository not found.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
: exit status 128

k8s-ci-robot · 2022-08-31T16:44:00Z

@mightyguava: This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-08-31T16:44:07Z

Hi @mightyguava. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mightyguava · 2022-08-31T16:47:01Z

api/krusty/remoteloader_test.go

+		},
+		{
+			name: "has origin annotation",
+			skip: true, // The annotated path should be "pod.yaml" but is "notCloned/pod.yaml"


Calling out that there may be an existing bug.

Looked into this briefly, and I think it is due to the special handling of the file protocol specifically. The path given to Origin#Append is not parsing as a RepoSpec, I think because it does not have .git in it. The error is url lacks orgRepo: pod.yaml, and the fallback essentially assumes that the origin in question is not a clone. So I think the problem is new to this PR, and another reason to make the file protocol handling as similar as possible to the others.

Ack, you are right. This goes back to the problem where GitSuffix has some special treatment that doesn't work well for the file:// scheme. I made some changes that try to make it play a bit nicer.

Can you now enable this test? Since the problem is novel to this PR, I would expect it to be working after your changes.

It appears that this is not novel to this PR. I've created #4801 as a minimal repro case on the existing code.

natasha41575 · 2022-08-31T21:34:14Z

/ok-to-test

api/krusty/remoteloader_test.go

KnVerey

Please update https://github.com/kubernetes-sigs/kustomize/blob/master/examples/remoteBuild.md, which currently says file clones are not supported. Please give examples and mention the difference between this and regular paths, i.e. that it ensures a clean checkout vs using the path directly.

KnVerey · 2022-09-07T20:13:57Z

api/internal/git/repospec.go

@@ -118,6 +119,20 @@ func parseGitURL(n string) (
 		path, gitRef, gitTimeout, gitSubmodules = peelQuery(n[index+len(gitDelimiter)+len(orgRepo):])
 		return
 	}
+	if strings.HasPrefix(n, fileScheme) {


It seems to me that your change to accommodate file:// in parseHostSpec below is never reached, since if a url has that prefix, it will end up here and return before L136. I also noticed that this is very similar to the git suffix code below. Instead of having a separate conditional, can we modify L138-164 something like this:

if strings.Contains(n, gitSuffix) { index := strings.Index(n, gitSuffix) if strings.HasPrefix(host, fileScheme) { host += n[0:index] // the path to the repo is part of the "url" for local file schemes } else { orgRepo = n[0:index] } n = n[index+len(gitSuffix):] if len(n) > 0 && n[0] == '/' { n = n[1:] } path, gitRef, gitTimeout, gitSubmodules = peelQuery(n) return }

That would result in the host not including .git, and GitSuffix being populated. That looks more consistent to me, but is there a reason you did otherwise?

Since we're using the file protocol for the bulk of integration tests, it would be best if its code paths are as similar as possible. To that end: is there a reason beyond naming why we couldn't let the middle segment get extracted as "OrgRepo"?

I did not populate GitSuffix because of this line, that drops the GitSuffix from the Origin repo.

kustomize/api/resource/origin.go

Line 55 in 14947e4

originCopy.Repo = repoSpec.Host + repoSpec.OrgRepo

. Looking at it again I think it was an unintentional omission.

Ah, now I remember there was another more problematic reason. GitSuffix defaults to ".git". It would be automatically included in the CloneSpec(). This would make the clone fail for file:// URLs where .git is not in the directory name.

I think the author who added GitSuffix may have assumed that .git is ignored by git tooling. That is not true. It's the hosting providers like GitHub and BitBucket that alias the repo name for convenience.

api/internal/git/repospec_test.go

KnVerey · 2022-09-07T20:33:50Z

api/internal/target/kusttarget.go

@@ -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.


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

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.

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.

KnVerey · 2022-09-07T20:43:27Z

api/krusty/remoteloader_test.go

+- "file://$ROOT/simple.git?ref=change-image"
+`,
+
+			expected: strings.ReplaceAll(simpleBuild, "nginx:1.7.9", "nginx:2"),


nit: create a variable for this result, like you did for the dev build expectation

KnVerey · 2022-09-07T20:44:18Z

api/krusty/remoteloader_test.go

+resources:
+- file://$ROOT/with-submodule.git/submodule?submodules=0
+`,
+			err: "unable to find",


what is the full error message?

KnVerey · 2022-09-08T01:48:40Z

api/internal/git/repospec.go

+	if strings.HasPrefix(n, fileScheme) {
+		idx := strings.Index(n, gitSuffix)
+		if idx == -1 {
+			// invalid, must have .git somewhere


Did you find something stating this? It seems to me that if we accept the urls in the same format git itself does, it should not end in .git (unless the directory in question is named that way, as you've done in the tests). If I'm right about that, we need to accommodate the format where the subpath follows // for the file protocol, like we do for https and ssh (note that // seems to be accepted even if .git is present). If I'm wrong, then that format will need to be tested in the true-remote tests.

I was not aware of the // syntax. I didn't know that there were docs for remoteBuild in the examples directory. Most of my changes here were based on the assumption we didn't have a way to indicate where the repo starts in the URL. I will change to use the // syntax.

I do feel that // is unintuitive and strange. It's also not documented on the kustomize site as far as I can tell. That's where I go look for docs. In past experience, // in a URL is usually a mistake when a developer forgets to use something like url.Join() to join URL segments.

Ah, I'm working on this, I remember why I required the .git suffix. This part of the code hardcodes orgRepo to 2 path segments. I wasn't sure if that a 2-segment orgRepo was relied on in other parts of the code and didn't want to cause regressions modifying this assumption.

I did notice that if there is a .git suffix, the code short-circuits before here and will take a multi-segment orgRepo. Hence why I went down that path.

Ok, as I'm digging into this more. I remember that I did see the // syntax in tests and briefly looked into it. I'm fairly certain that Kustomize does not actually have code that supports the // syntax. If you look at the existing tests, all uses of // are either

directly after the .git, which short-circuts the orgRepo parsing logic

directly after a 2-segment org/repo, which works because the org/repo/ is always chopped off.

So, I thought at the time you were just using it for documentation in tests. Could you please how you'd like to proceed?

Btw, all this WEIRDNESS!! is why I have the following comment in the PR description

I made a few small changes to repospec to support it. It's a bit hacked but to do it better would require a lot larger change that I do not want to do in this PR.

I think to make this more clean in a way you and I would both like would actually require rewriting this file that I do not wish to do right now. There are a bunch of rarely used, documented features like _git, //, git:: that were added for very specific cases but implemented a bit more generically (or not) so that it makes Kustomize be a little more lenient or stricter than it should in certain cases not exercised in tests.

I didn't know that there were docs for remoteBuild in the examples directory.

Yeah, unfortunately our documentation is quite fragmented: #4338 . I'm eager to support contributors to work on that project, which has quite a low barrier to entry (with limited time, Natasha and I focus on high context work newer folks cannot do).

Kustomize does not actually have code that supports the // syntax

Implementation aside, support for it does seem deliberate. The test in RepoSpec was specifically added as regression coverage: #780.

So, I thought at the time you were just using it for documentation in tests. Could you please how you'd like to proceed?

We need to keep supporting // both because it is documented and just because it currently works. Whatever has worked has been working for a long time, and we don't want to cause end user pain without a very good reason. FWIW the preamble to one of the RepoSpec tests alludes to this:

func TestNewRepoSpecFromUrl_Permute(t *testing.T) { // Generate all many permutations of host, orgRepo, pathName, and ref. // Not all combinations make sense, but the parsing is very permissive and // we probably stil don't want to break backwards compatibility for things // that are unintentionally supported.

As you've discovered, the // convention actually comes in handy in situations where the .git suffix is not used. I tried to find the source of it, and my educated guess is that we inherited it from go-getter: https://github.com/hashicorp/go-getter#subdirectories. Before my time, Kustomize used to use go-getter directly, but it was later replaced with bespoke code because kubernetes/kubernetes (for kubectl kustomize) could not accept that dependency.

Btw, all this WEIRDNESS!! is why I have the following comment in the PR description

Thank you for sticking with us through this. We don't have all the historical context on this either, so perhaps that helps with understanding why we felt we needed the code freeze until our coverage of this area is reliable. 😄

I think to make this more clean in a way you and I would both like would actually require rewriting this file that I do not wish to do right now.

I totally agree with you that we should save refactoring for after we're finished with the test coverage improvements, and do it separately. If you have ideas for how that should look, we're definitely interested in the follow up PR!

We need to keep supporting // both because it is documented and just because it currently works.

Regarding that, I was saying that it doesn't currently work. It only works in cases where a single / would also work just fine. I added support for // only for the file:// protocol.

KnVerey · 2022-09-08T02:25:48Z

api/krusty/remoteloader_test.go

+`,
+		},
+		{
+			name: "has ref path and origin annotation",


please add timeout both as a dedicated test and to this combo test.

api/krusty/remoteloader_test.go

KnVerey · 2022-09-08T02:57:15Z

api/krusty/remoteloader_test.go

+		},
+		{
+			name: "has origin annotation",
+			skip: true, // The annotated path should be "pod.yaml" but is "notCloned/pod.yaml"


Looked into this briefly, and I think it is due to the special handling of the file protocol specifically. The path given to Origin#Append is not parsing as a RepoSpec, I think because it does not have .git in it. The error is url lacks orgRepo: pod.yaml, and the fallback essentially assumes that the origin in question is not a clone. So I think the problem is new to this PR, and another reason to make the file protocol handling as similar as possible to the others.

mightyguava · 2022-09-08T17:28:29Z

@KnVerey thank you for the very thorough review. I have addressed and/or replied to all of your comments.

KnVerey · 2022-09-08T19:09:32Z

examples/remoteBuild.md

+Kustomize will clone the repo to a temporary directory and do a clean checkout
+of the `ref`. This behavior is differs from a direct path reference
+like `/path/to/repo/someSubdir`, in which case Kustomize will not use Git at
+all, and process the files at the path directly.


Very nice explanation, thank you. 👌

KnVerey · 2022-09-08T19:33:01Z

api/internal/git/repospec.go

+	if strings.HasPrefix(n, fileScheme) {
+		idx := strings.Index(n, gitSuffix)
+		if idx == -1 {
+			// invalid, must have .git somewhere


I didn't know that there were docs for remoteBuild in the examples directory.

Yeah, unfortunately our documentation is quite fragmented: #4338 . I'm eager to support contributors to work on that project, which has quite a low barrier to entry (with limited time, Natasha and I focus on high context work newer folks cannot do).

Kustomize does not actually have code that supports the // syntax

Implementation aside, support for it does seem deliberate. The test in RepoSpec was specifically added as regression coverage: #780.

So, I thought at the time you were just using it for documentation in tests. Could you please how you'd like to proceed?

We need to keep supporting // both because it is documented and just because it currently works. Whatever has worked has been working for a long time, and we don't want to cause end user pain without a very good reason. FWIW the preamble to one of the RepoSpec tests alludes to this:

func TestNewRepoSpecFromUrl_Permute(t *testing.T) { // Generate all many permutations of host, orgRepo, pathName, and ref. // Not all combinations make sense, but the parsing is very permissive and // we probably stil don't want to break backwards compatibility for things // that are unintentionally supported.

As you've discovered, the // convention actually comes in handy in situations where the .git suffix is not used. I tried to find the source of it, and my educated guess is that we inherited it from go-getter: https://github.com/hashicorp/go-getter#subdirectories. Before my time, Kustomize used to use go-getter directly, but it was later replaced with bespoke code because kubernetes/kubernetes (for kubectl kustomize) could not accept that dependency.

Btw, all this WEIRDNESS!! is why I have the following comment in the PR description

Thank you for sticking with us through this. We don't have all the historical context on this either, so perhaps that helps with understanding why we felt we needed the code freeze until our coverage of this area is reliable. 😄

I think to make this more clean in a way you and I would both like would actually require rewriting this file that I do not wish to do right now.

I totally agree with you that we should save refactoring for after we're finished with the test coverage improvements, and do it separately. If you have ideas for how that should look, we're definitely interested in the follow up PR!

natasha41575

This PR looks good to me (I just have a few minor nits). I'll defer to @KnVerey to make sure her comments are addressed for approval.

/lgtm

natasha41575 · 2022-09-15T22:12:22Z

api/internal/git/gitrunner.go

 			if err != nil {
-				return errors.Wrapf(err, "git cmd = '%s'", cmd.String())
+				return errors.Wrapf(err, "git cmd = '%s' failed:\n%s", cmd.String(), string(out))


minor wording nit

Suggested change

return errors.Wrapf(err, "git cmd = '%s' failed:\n%s", cmd.String(), string(out))

return errors.Wrapf(err, "failed to run '%s': %s", cmd.String(), string(out))

natasha41575 · 2022-09-15T22:18:56Z

api/internal/git/repospec_test.go

+			cloneSpec: "https://github.com/kubernetes-sigs/kustomize.git",
+			absPath:   notCloned.Join("/examples/multibases/dev"),
+			repoSpec: RepoSpec{
+				Host:      "https://github.com/",


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.

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!

natasha41575 · 2022-09-15T22:23:22Z

api/internal/git/repospec_test.go

@@ -285,11 +287,104 @@ 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's a comment in L117 above that says

// No tests for submodules and timeout as the expectations are hard-coded
// to the defaults for compactness.

Does this test change that? I'm wondering if we should remove that comment now.

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.

api/krusty/remoteloader_test.go

mightyguava · 2022-09-16T12:14:42Z

Thank you for the review @natasha41575! It seems that with this change I broke a couple origin tests. I ended up updating the tests in my last commit. I think the end result is potentially more technically correct than the previous behavior so I'm inclined to keep it if you think the behavior change is OK?

#4783 (comment) is the motivation and I don't currently have an alternative in mind.

natasha41575 · 2022-09-16T16:26:56Z

api/krusty/transformerannotation_test.go

@@ -288,7 +288,7 @@ kind: Pod
 metadata:
  annotations:
    alpha.config.kubernetes.io/transformations: |
-      - repo: https://github.com/kubernetes-sigs/kustomize
+      - repo: https://github.com/kubernetes-sigs/kustomize.git


For these origin annotations, it looks like the only behavior change introduced by this PR is that we now preserve the .git in the repo url. If that's the case, then yes I agree that this is more correct.

KnVerey

Please fix the linter error 🙏

KnVerey · 2022-09-16T23:33:25Z

api/internal/git/repospec_test.go

+			absPath:   notCloned.Join("/examples/multibases/dev"),
+			repoSpec: RepoSpec{
+				Host:      "https://fake-git-hosting.org/",
+				OrgRepo:   "path/to/server",


Suggested change

OrgRepo: "path/to/server",

OrgRepo: "path/to/repo",

Missed this one?

KnVerey · 2022-09-17T00:12:30Z

api/internal/target/kusttarget.go

@@ -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.


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.

KnVerey · 2022-09-17T00:18:04Z

api/krusty/remoteloader_test.go

+		},
+		{
+			name: "has origin annotation",
+			skip: true, // The annotated path should be "pod.yaml" but is "notCloned/pod.yaml"


Can you now enable this test? Since the problem is novel to this PR, I would expect it to be working after your changes.

api/krusty/remoteloader_test.go

mightyguava · 2022-09-17T14:13:04Z

@KnVerey done! And replied to your comments.

Re: lint, not directly related, but I've never been able to get the full lint + test suite to pass(even on master) on my laptop. Random errors show up. I run tests on what I know the code impacts, but have been relying on CI to some extent.

KnVerey · 2022-09-17T15:52:18Z

/ok-to-test

Re: lint, not directly related, but I've never been able to get the full lint + test suite to pass(even on master) on my laptop. Random errors show up. I run tests on what I know the code impacts, but have been relying on CI to some extent.

Can you confirm you're using the same version of golangci-lint that our makefile installs? (you can do make uninstall-out-of-tree-tools then make install-out-of-tree-tools to force install it). That has usually been the problem with linter inconsistencies folks have seen in the past.

KnVerey · 2022-09-17T15:59:40Z

api/internal/git/cloner.go

 	ref := "HEAD"
 	if repoSpec.Ref != "" {
 		ref = repoSpec.Ref
 	}
-	if err = r.run("fetch", "--depth=1", repoSpec.CloneSpec(), ref); err != nil {
+	if err = r.run("fetch", "--depth=1", "origin", ref); err != nil {


I'm fine with these changes staying if you want; it's the api/internal/target/kusttarget.go changes specifically that I think need dedicated attention in a new PR. Sorry if that was unclear.

Great! Added these 2 back and they fixed the one failing test

KnVerey · 2022-09-17T16:00:26Z

api/internal/git/gitrunner.go

 			if err != nil {
-				return errors.Wrapf(err, "failed to run '%s': %s", cmd.String(), string(out))
+				return errors.Wrapf(err, "git cmd = '%s'", cmd.String())


Same here: I'm fine with these changes staying if you want; it's the api/internal/target/kusttarget.go changes specifically that I think need dedicated attention in a new PR. Sorry if that was unclear.

KnVerey · 2022-09-17T16:03:56Z

Thanks for opening the PR with the repro case showing the origin annotation problem is not new! There's a test failure related to error messaging, but otherwise this is good to go.

/approve

k8s-ci-robot · 2022-09-17T16:04:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, mightyguava

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KnVerey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mightyguava · 2022-09-17T16:53:34Z

Can you confirm you're using the same version of golangci-lint that our makefile installs? (you can do make uninstall-out-of-tree-tools then make install-out-of-tree-tools to force install it). That has usually been the problem with linter inconsistencies folks have seen in the past.

Yeah, it is. I think I'll figure that out later :)

❯ golangci-lint version
golangci-lint has version v1.46.2 built from (unknown, mod sum: "h1:o90t/Xa6dhJbvy8Bz2RpzUXqrkigp19DLStMolTZbyo=") on (unknown)

KnVerey · 2022-09-19T16:08:01Z

/lgtm

mightyguava added 2 commits August 31, 2022 12:40

Better error message when git clone fails

a249b04

support file:// URLs

ddc6825

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 31, 2022

mightyguava changed the title ~~Rewrite integration tests~~ Rewrite remoteload_test integration tests Aug 31, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 31, 2022

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 31, 2022

k8s-ci-robot requested review from KnVerey and yuwenma August 31, 2022 16:44

mightyguava mentioned this pull request Aug 31, 2022

Attempt to fix remoteload_test flakes and add better errors #4778

Closed

rewrite remoteload_test

185ec88

mightyguava force-pushed the rewrite-integration-tests branch from 7dfe314 to 185ec88 Compare August 31, 2022 16:46

mightyguava commented Aug 31, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 31, 2022

natasha41575 reviewed Aug 31, 2022

View reviewed changes

api/krusty/remoteloader_test.go Show resolved Hide resolved

lint and test fix

1287caf

KnVerey reviewed Sep 8, 2022

View reviewed changes

fixes for kverey's comments

74cc1c0

mightyguava force-pushed the rewrite-integration-tests branch from 30ca1c7 to 74cc1c0 Compare September 8, 2022 17:27

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 8, 2022

mightyguava added 2 commits September 8, 2022 14:06

document file:// remote load

775b7a1

replace assert with require where appropriate

0963a2a

KnVerey reviewed Sep 8, 2022

View reviewed changes

mightyguava force-pushed the rewrite-integration-tests branch from 92d3031 to 3667068 Compare September 11, 2022 15:26

natasha41575 reviewed Sep 15, 2022

View reviewed changes

k8s-ci-robot assigned natasha41575 Sep 15, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2022

fixes plus pr review from natasha

020fec9

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2022

natasha41575 reviewed Sep 16, 2022

View reviewed changes

KnVerey reviewed Sep 17, 2022

View reviewed changes

fixes for review, lint

3217f9a

mightyguava mentioned this pull request Sep 17, 2022

DO NOT MERGE: minimal repro for origin annotation bug #4801

Closed

KnVerey reviewed Sep 17, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2022

mightyguava added 2 commits September 17, 2022 12:48

revert changes to error handling

4c0bf39

fix skipped test

215455c

mightyguava force-pushed the rewrite-integration-tests branch from 57947c4 to 215455c Compare September 17, 2022 16:53

KnVerey added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 19, 2022

k8s-ci-robot assigned KnVerey Sep 19, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2022

k8s-ci-robot merged commit e62480d into kubernetes-sigs:master Sep 19, 2022

mightyguava mentioned this pull request Sep 20, 2022

Flaky integration tests #4640

Closed

annasong20 mentioned this pull request Sep 21, 2022

Implement kustomize localize localization layer #4797

Closed

annasong20 mentioned this pull request May 19, 2023

Git submodule needs origin #5131

Closed

	return errors.Wrapf(err, "git cmd = '%s' failed:\n%s", cmd.String(), string(out))
	return errors.Wrapf(err, "failed to run '%s': %s", cmd.String(), string(out))

Rewrite remoteload_test integration tests #4783

Rewrite remoteload_test integration tests #4783

Conversation

mightyguava commented Aug 31, 2022 • edited Loading

k8s-ci-robot commented Aug 31, 2022

k8s-ci-robot commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 commented Aug 31, 2022

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyguava Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

mightyguava Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyguava Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

mightyguava Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

mightyguava Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyguava Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyguava commented Sep 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyguava Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

mightyguava commented Sep 16, 2022 • edited Loading

natasha41575 Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mightyguava commented Sep 17, 2022

KnVerey commented Sep 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Sep 17, 2022

k8s-ci-robot commented Sep 17, 2022

mightyguava commented Sep 17, 2022

KnVerey commented Sep 19, 2022

mightyguava commented Aug 31, 2022 •

edited

Loading

mightyguava Sep 8, 2022 •

edited

Loading

mightyguava Sep 8, 2022 •

edited

Loading

mightyguava Sep 8, 2022 •

edited

Loading

mightyguava Sep 8, 2022 •

edited

Loading

mightyguava Sep 8, 2022 •

edited

Loading

mightyguava Sep 8, 2022 •

edited

Loading

mightyguava Sep 16, 2022 •

edited

Loading

mightyguava commented Sep 16, 2022 •

edited

Loading

natasha41575 Sep 16, 2022 •

edited

Loading