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

be error when no path matching #5030

Merged
merged 6 commits into from
Jun 7, 2023
Merged

be error when no path matching #5030

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dfd2d76
be error when no path match
koba1t Feb 3, 2023
53a4134
fix error handling and test
koba1t Feb 8, 2023
cd49194
fix tests
koba1t Feb 8, 2023
694b3c9
Add the test cases for check the file when edit add command
koba1t Feb 28, 2023
8383b28
fix test case handling and update a function comment
koba1t May 24, 2023
565cff2
update test add KustomizationFile
koba1t May 24, 2023
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
9 changes: 9 additions & 0 deletions kustomize/commands/create/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"reflect"
"testing"

"github.com/stretchr/testify/assert"
"sigs.k8s.io/kustomize/api/provider"
"sigs.k8s.io/kustomize/api/types"
"sigs.k8s.io/kustomize/kustomize/v5/commands/internal/kustfile"
Expand Down Expand Up @@ -53,6 +54,14 @@ func TestCreateWithResources(t *testing.T) {
}
}

func TestCreateWithResourcesWithFileNotFound(t *testing.T) {
fSys := filesys.MakeEmptyDirInMemory()
assert.NoError(t, fSys.WriteFile("foo.yaml", []byte("")))
opts := createFlags{resources: "foo.yaml,bar.yaml"}
err := runCreate(opts, fSys, factory)
assert.EqualError(t, err, "bar.yaml has no match: must build at directory: not a valid directory: 'bar.yaml' doesn't exist")
}

func TestCreateWithNamespace(t *testing.T) {
fSys := filesys.MakeFsInMemory()
want := "foo"
Expand Down
16 changes: 13 additions & 3 deletions kustomize/commands/edit/add/addcomponent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestAddComponentHappyPath(t *testing.T) {
}

func TestAddComponentAlreadyThere(t *testing.T) {
fSys := filesys.MakeFsInMemory()
fSys := filesys.MakeEmptyDirInMemory()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

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))
}

err := fSys.WriteFile(componentFileName, []byte(componentFileContent))
require.NoError(t, err)
testutils_test.WriteTestKustomization(fSys)
Expand All @@ -52,7 +52,7 @@ func TestAddComponentAlreadyThere(t *testing.T) {
}

func TestAddKustomizationFileAsComponent(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it at 565cff2.

fSys := filesys.MakeFsInMemory()
fSys := filesys.MakeEmptyDirInMemory()
err := fSys.WriteFile(componentFileName, []byte(componentFileContent))
require.NoError(t, err)
testutils_test.WriteTestKustomization(fSys)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this test previously broken?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so.

assert.Contains(t, string(content), componentFileName)
}

func TestAddComponentNoArgs(t *testing.T) {
Expand All @@ -73,3 +73,13 @@ func TestAddComponentNoArgs(t *testing.T) {
err := cmd.Execute()
assert.EqualError(t, err, "must specify a component file")
}

func TestAddComponentFileNotFound(t *testing.T) {
fSys := filesys.MakeEmptyDirInMemory()

cmd := newCmdAddComponent(fSys)
args := []string{componentFileName}

err := cmd.RunE(cmd, args)
assert.EqualError(t, err, componentFileName+" has no match: must build at directory: not a valid directory: '"+componentFileName+"' doesn't exist")
}
16 changes: 13 additions & 3 deletions kustomize/commands/edit/add/addresource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ replacements:
}

func TestAddResourceAlreadyThere(t *testing.T) {
fSys := filesys.MakeFsInMemory()
fSys := filesys.MakeEmptyDirInMemory()
err := fSys.WriteFile(resourceFileName, []byte(resourceFileContent))
require.NoError(t, err)
testutils_test.WriteTestKustomization(fSys)
Expand All @@ -71,7 +71,7 @@ func TestAddResourceAlreadyThere(t *testing.T) {
}

func TestAddKustomizationFileAsResource(t *testing.T) {
fSys := filesys.MakeFsInMemory()
fSys := filesys.MakeEmptyDirInMemory()
err := fSys.WriteFile(resourceFileName, []byte(resourceFileContent))
require.NoError(t, err)
testutils_test.WriteTestKustomization(fSys)
Expand All @@ -83,7 +83,7 @@ func TestAddKustomizationFileAsResource(t *testing.T) {
content, err := testutils_test.ReadTestKustomization(fSys)
assert.NoError(t, err)

assert.NotContains(t, string(content), resourceFileName)
assert.Contains(t, string(content), resourceFileName)
}

func TestAddResourceNoArgs(t *testing.T) {
Expand All @@ -94,3 +94,13 @@ func TestAddResourceNoArgs(t *testing.T) {
assert.Error(t, err)
assert.Equal(t, "must specify a resource file", err.Error())
}

func TestAddResourceFileNotFound(t *testing.T) {
fSys := filesys.MakeEmptyDirInMemory()

cmd := newCmdAddResource(fSys)
args := []string{resourceFileName}

err := cmd.RunE(cmd, args)
assert.EqualError(t, err, resourceFileName+" has no match: must build at directory: not a valid directory: '"+resourceFileName+"' doesn't exist")
}
2 changes: 1 addition & 1 deletion kustomize/commands/internal/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@koba1t koba1t Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@koba1t koba1t May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I updated this function comment at 8383b28.

} else {
result = append(result, pattern)
if loader != nil {
Expand Down
5 changes: 3 additions & 2 deletions kustomize/commands/internal/util/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ func TestGlobPatternsWithLoaderRemoteFile(t *testing.T) {
}

// test load invalid file
resources, err = GlobPatternsWithLoader(fSys, ldr, []string{"http://invalid"})
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" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a check here for

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed it at 8383b28.

t.Fatalf("unexpected load error: %v", err)
}
if len(resources) > 0 {
Expand Down