be error when no path matching #5030

koba1t · 2023-02-03T16:29:43Z

What happened?

kustomize edit command does not return an error when adding no matching path.

$ kustomize init
$ ls
kustomization.yaml
$ kustomize edit add resource do-not-exist
2023/02/04 01:08:18 do-not-exist has no match
$ echo $?
0

What doing?

It was caused by this line only show the warning message.
I think better return the error on this line.

Maybe this fix is not the best, please write your opinions if you have any.

cailyn-codes · 2023-02-06T15:12:18Z

kustomize/commands/internal/util/util_test.go

+	if err == nil {
 		t.Fatalf("unexpected load error: %v", err)


question: If err is nil isn't there no error?

I fix this line.

cailyn-codes · 2023-02-06T15:12:38Z

kustomize/commands/internal/util/util.go

@@ -42,7 +42,7 @@ func GlobPatternsWithLoader(fSys filesys.FileSystem, ldr ifc.Loader, patterns []
 		if len(files) == 0 {
 			loader, err := ldr.New(pattern)
 			if err != nil {
-				log.Printf("%s has no match", pattern)
+				return nil, fmt.Errorf("%s has no match", pattern)


suggestion: Maybe worth including the original error?

That is a good suggestion. Thanks!
I add it.

k8s-ci-robot · 2023-02-08T19:01:47Z

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.

cailyn-codes · 2023-02-09T21:16:37Z

/lgtm

KnVerey · 2023-02-27T17:56:39Z

kustomize/commands/edit/add/addcomponent_test.go

@@ -63,7 +63,7 @@ func TestAddKustomizationFileAsComponent(t *testing.T) {

 	content, err := testutils_test.ReadTestKustomization(fSys)
 	require.NoError(t, err)
-	assert.NotContains(t, string(content), componentFileName)


Was this test previously broken?

I think so.

KnVerey · 2023-02-27T17:57:22Z

kustomize/commands/edit/add/addcomponent_test.go

@@ -38,7 +38,7 @@ func TestAddComponentHappyPath(t *testing.T) {
 }

 func TestAddComponentAlreadyThere(t *testing.T) {
-	fSys := filesys.MakeFsInMemory()
+	fSys := filesys.MakeEmptyDirInMemory()


Why these changes?

Because I think these test cases were broken.
It looks like MakeEmptyDirInMemory is required for the parent directory position for the file path parameter.
But, these test cases were passed despite using the wrong parameter.
So, I fixed to use this function that is not required for the parent directory position.

If you want, I can rewrite for using filesys.MakeEmptyDirInMemory() like the below case.

func TestAddComponentAlreadyThere(t *testing.T) { fSys := filesys.MakeFsInMemory() err := fSys.WriteFile(componentFileName, []byte(componentFileContent)) require.NoError(t, err) testutils_test.WriteTestKustomization(fSys) cmd := newCmdAddComponent(fSys) args := []string{"/" + componentFileName} // add '/' dir for parameter. assert.NoError(t, cmd.RunE(cmd, args)) // adding an existing component doesn't return an error assert.NoError(t, cmd.RunE(cmd, args)) }

KnVerey · 2023-02-27T17:57:58Z

kustomize/commands/internal/util/util.go

@@ -42,7 +42,7 @@ func GlobPatternsWithLoader(fSys filesys.FileSystem, ldr ifc.Loader, patterns []
 		if len(files) == 0 {
 			loader, err := ldr.New(pattern)
 			if err != nil {
-				log.Printf("%s has no match", pattern)
+				return nil, fmt.Errorf("%s has no match: %w", pattern, err)


can we add a new "edit add" test covering this error case please?

I add test cases at 694b3c9.

I think this function is called by only below 3 points.
https://github.com/search?q=repo%3Akubernetes-sigs%2Fkustomize+util.GlobPatternsWithLoader&type=code

Can you update the comment description of this function to specify that it returns an error if there are no matching files and it can't load from remote?

I updated this function comment at 8383b28.

koba1t · 2023-02-28T19:14:46Z

@KnVerey

I fixed and responded to your comments.
Could you recheck there?

natasha41575 · 2023-05-19T19:23:40Z

kustomize/commands/internal/util/util_test.go

-	if err != nil {
+	invalidURL := "http://invalid"
+	resources, err = GlobPatternsWithLoader(fSys, ldr, []string{invalidURL})
+	if err != nil && err.Error() != invalidURL+" has no match: "+invalidURL+" not exist" {


Can we also add a check here for

if err == nil { t.Fatalf("expected error but did not receive one") }

I fixed it at 8383b28.

natasha41575 · 2023-05-19T19:28:47Z

kustomize/commands/internal/util/util.go

@@ -42,7 +42,7 @@ func GlobPatternsWithLoader(fSys filesys.FileSystem, ldr ifc.Loader, patterns []
 		if len(files) == 0 {
 			loader, err := ldr.New(pattern)
 			if err != nil {
-				log.Printf("%s has no match", pattern)
+				return nil, fmt.Errorf("%s has no match: %w", pattern, err)


Can you update the comment description of this function to specify that it returns an error if there are no matching files and it can't load from remote?

natasha41575 · 2023-05-19T19:37:47Z

kustomize/commands/edit/add/addcomponent_test.go

@@ -52,7 +52,7 @@ func TestAddComponentAlreadyThere(t *testing.T) {
 }

 func TestAddKustomizationFileAsComponent(t *testing.T) {


I think this test was originally supposed to check that if you try to add the kustomization.yaml file itself to components, it won't get added.

@annasong20 attempted to fix this in https://github.com/kubernetes-sigs/kustomize/pull/4702/files#diff-6f49ffbefe3aa6f9f90b63fa94b7019f1004257536c2264e6445f7b311d10b99, but it fell off our radar and it never merged. Do you mind picking up her fixes from TestAddKustomizationFileAsComponent and TestAddKustomizationFileAsResource from that PR?

I added it at 565cff2.

koba1t · 2023-05-24T20:32:47Z

Hi @natasha41575
Could you recheck it?

natasha41575 · 2023-06-07T14:32:12Z

/lgtm
/approve

k8s-ci-robot · 2023-06-07T14:32:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: koba1t, natasha41575

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~~ [natasha41575]

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

be error when no path match

dfd2d76

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 3, 2023

k8s-ci-robot requested review from KnVerey and yuwenma February 3, 2023 16:29

cailyn-codes reviewed Feb 6, 2023

View reviewed changes

fix error handling and test

53a4134

fix tests

cd49194

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 8, 2023

k8s-ci-robot assigned cailyn-codes Feb 9, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2023

KnVerey reviewed Feb 27, 2023

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 28, 2023

koba1t force-pushed the fix/error_when_no_path_match branch from 85a4c4b to 6508cf6 Compare February 28, 2023 19:16

Add the test cases for check the file when edit add command

694b3c9

koba1t force-pushed the fix/error_when_no_path_match branch from 6508cf6 to 694b3c9 Compare February 28, 2023 19:21

natasha41575 reviewed May 19, 2023

View reviewed changes

natasha41575 assigned natasha41575 and koba1t and unassigned cailyn-codes May 24, 2023

koba1t added 2 commits May 25, 2023 04:25

fix test case handling and update a function comment

8383b28

update test add KustomizationFile

565cff2

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2023

k8s-ci-robot merged commit dce9426 into kubernetes-sigs:master Jun 7, 2023

koba1t deleted the fix/error_when_no_path_match branch June 7, 2023 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

be error when no path matching #5030

be error when no path matching #5030

koba1t commented Feb 3, 2023

cailyn-codes Feb 6, 2023

koba1t Feb 8, 2023 •

edited

Loading

cailyn-codes Feb 6, 2023

koba1t Feb 8, 2023

k8s-ci-robot commented Feb 8, 2023

cailyn-codes commented Feb 9, 2023

KnVerey Feb 27, 2023

koba1t Feb 28, 2023

KnVerey Feb 27, 2023

koba1t Feb 28, 2023

KnVerey Feb 27, 2023

koba1t Feb 28, 2023 •

edited

Loading

natasha41575 May 19, 2023

koba1t May 24, 2023 •

edited

Loading

koba1t commented Feb 28, 2023

natasha41575 May 19, 2023

koba1t May 24, 2023

natasha41575 May 19, 2023

natasha41575 May 19, 2023

koba1t May 24, 2023

koba1t commented May 24, 2023

natasha41575 commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

		@@ -52,7 +52,7 @@ func TestAddComponentAlreadyThere(t *testing.T) {
		}

		func TestAddKustomizationFileAsComponent(t *testing.T) {

be error when no path matching #5030

be error when no path matching #5030

Conversation

koba1t commented Feb 3, 2023

What happened?

What doing?

Choose a reason for hiding this comment

koba1t Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 8, 2023

cailyn-codes commented Feb 9, 2023

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

koba1t Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koba1t May 24, 2023 • edited Loading

Choose a reason for hiding this comment

koba1t commented Feb 28, 2023

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

koba1t commented May 24, 2023

natasha41575 commented Jun 7, 2023

k8s-ci-robot commented Jun 7, 2023

koba1t Feb 8, 2023 •

edited

Loading

koba1t Feb 28, 2023 •

edited

Loading

koba1t May 24, 2023 •

edited

Loading