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.

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

Add --no-verify flag to edit add resource command #5333

2 changes: 1 addition & 1 deletion kustomize/commands/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func runCreate(opts createFlags, fSys filesys.FileSystem, rf *resource.Factory)
var resources []string
var err error
if opts.resources != "" {
resources, err = util.GlobPatternsWithLoader(fSys, ldrhelper.NewFileLoaderAtCwd(fSys), strings.Split(opts.resources, ","))
resources, err = util.GlobPatternsWithLoader(fSys, ldrhelper.NewFileLoaderAtCwd(fSys), strings.Split(opts.resources, ","), false)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion kustomize/commands/edit/add/addcomponent.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (o *addComponentOptions) Validate(args []string) error {

// RunAddComponent runs addComponent command (do real work).
func (o *addComponentOptions) RunAddComponent(fSys filesys.FileSystem) error {
components, err := util.GlobPatternsWithLoader(fSys, loader.NewFileLoaderAtCwd(fSys), o.componentFilePaths)
components, err := util.GlobPatternsWithLoader(fSys, loader.NewFileLoaderAtCwd(fSys), o.componentFilePaths, false)
if err != nil {
return err
}
Expand Down
6 changes: 5 additions & 1 deletion kustomize/commands/edit/add/addresource.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

type addResourceOptions struct {
resourceFilePaths []string
skipValidation bool
}

// newCmdAddResource adds the name of a file containing a resource to the kustomization file.
Expand All @@ -35,6 +36,9 @@ func newCmdAddResource(fSys filesys.FileSystem) *cobra.Command {
return o.RunAddResource(fSys)
},
}
cmd.Flags().BoolVar(&o.skipValidation, "skip-validation", false,
Copy link
Contributor

@natasha41575 natasha41575 Oct 2, 2023

Choose a reason for hiding this comment

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

For parity with the flag we are adding to kustomize localize (#5276), can we likewise name this flag --no-verify?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be better. I've renamed the flag.

"skip validation for resources",
)
return cmd
}

Expand All @@ -49,7 +53,7 @@ func (o *addResourceOptions) Validate(args []string) error {

// RunAddResource runs addResource command (do real work).
func (o *addResourceOptions) RunAddResource(fSys filesys.FileSystem) error {
resources, err := util.GlobPatternsWithLoader(fSys, ldrhelper.NewFileLoaderAtCwd(fSys), o.resourceFilePaths)
resources, err := util.GlobPatternsWithLoader(fSys, ldrhelper.NewFileLoaderAtCwd(fSys), o.resourceFilePaths, o.skipValidation)
if err != nil {
return err
}
Expand Down
42 changes: 27 additions & 15 deletions kustomize/commands/internal/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,28 +30,40 @@ func GlobPatterns(fSys filesys.FileSystem, patterns []string) ([]string, error)
return result, nil
}

// GlobPatterns accepts a slice of glob strings and returns the set of matching file paths. If files are not found, will try load from remote.
// It returns an error if there are no matching files or it can't load from remote.
func GlobPatternsWithLoader(fSys filesys.FileSystem, ldr ifc.Loader, patterns []string) ([]string, error) {
// GlobPatterns accepts a slice of glob strings and returns the set of matching file paths.
// If validation is skipped, then it will return the patterns as provided.
// Otherwise, It will try to load the files from the filesystem.
// If files are not found in the filesystem, it will try to load from remote.
// It returns an error if validation is not skipped and there are no matching files or it can't load from remote.
func GlobPatternsWithLoader(fSys filesys.FileSystem, ldr ifc.Loader, patterns []string, skipValidation bool) ([]string, error) {
var result []string
for _, pattern := range patterns {
if skipValidation {
result = append(result, pattern)
continue
}

files, err := fSys.Glob(pattern)
if err != nil {
return nil, err
return nil, fmt.Errorf("error checking the filesystem: %w", err)
}
if len(files) == 0 {
loader, err := ldr.New(pattern)
if err != nil {
return nil, fmt.Errorf("%s has no match: %w", pattern, err)
} else {
result = append(result, pattern)
if loader != nil {
loader.Cleanup()
}
}

if len(files) != 0 {
result = append(result, files...)
continue
}
result = append(result, files...)

loader, err := ldr.New(pattern)
if err != nil {
return nil, fmt.Errorf("%s has no match: %w", pattern, err)
hailkomputer marked this conversation as resolved.
Show resolved Hide resolved
}

result = append(result, pattern)
if loader != nil {
if err = loader.Cleanup(); err != nil {
return nil, fmt.Errorf("error cleaning up loader: %w", err)
}
}
}
return result, nil
}
Expand Down
67 changes: 26 additions & 41 deletions kustomize/commands/internal/util/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"reflect"
"testing"

"github.com/stretchr/testify/require"
"sigs.k8s.io/kustomize/api/ifc"
"sigs.k8s.io/kustomize/kyaml/filesys"
)
Expand All @@ -21,26 +22,18 @@ func TestConvertToMap(t *testing.T) {
expected["g"] = "h:k"

result, err := ConvertToMap(args, "annotation")
if err != nil {
t.Errorf("unexpected error: %v", err.Error())
}
require.NoError(t, err, "unexpected error")

eq := reflect.DeepEqual(expected, result)
if !eq {
t.Errorf("Converted map does not match expected, expected: %v, result: %v\n", expected, result)
}
require.True(t, eq, "Converted map does not match expected")
}

func TestConvertToMapError(t *testing.T) {
args := "a:b,c:\"d\",:f:g"

_, err := ConvertToMap(args, "annotation")
if err == nil {
t.Errorf("expected an error")
}
if err.Error() != "invalid annotation: ':f:g' (need k:v pair where v may be quoted)" {
t.Errorf("incorrect error: %v", err.Error())
}
require.Error(t, err, "expected error but did not receive one")
require.Equal(t, "invalid annotation: ':f:g' (need k:v pair where v may be quoted)", err.Error(), "incorrect error")
}

func TestConvertSliceToMap(t *testing.T) {
Expand All @@ -52,14 +45,10 @@ func TestConvertSliceToMap(t *testing.T) {
expected["g"] = "h:k"

result, err := ConvertSliceToMap(args, "annotation")
if err != nil {
t.Errorf("unexpected error: %v", err.Error())
}
require.NoError(t, err, "unexpected error")

eq := reflect.DeepEqual(expected, result)
if !eq {
t.Errorf("Converted map does not match expected, expected: %v, result: %v\n", expected, result)
}
require.True(t, eq, "Converted map does not match expected")
}

func TestGlobPatternsWithLoaderRemoteFile(t *testing.T) {
Expand All @@ -71,34 +60,30 @@ func TestGlobPatternsWithLoaderRemoteFile(t *testing.T) {
}

// test load remote file
resources, err := GlobPatternsWithLoader(fSys, ldr, []string{httpPath})
if err != nil {
t.Fatalf("unexpected load error: %v", err)
}
if len(resources) != 1 || resources[0] != httpPath {
t.Fatalf("incorrect resources: %v", resources)
}
resources, err := GlobPatternsWithLoader(fSys, ldr, []string{httpPath}, false)
require.NoError(t, err, "unexpected load error")
require.Equal(t, 1, len(resources), "incorrect resources")
require.Equal(t, httpPath, resources[0], "incorrect resources")

// test load local and remote file
resources, err = GlobPatternsWithLoader(fSys, ldr, []string{httpPath, "/test.yml"})
if err != nil {
t.Fatalf("unexpected load error: %v", err)
}
if len(resources) != 2 || resources[0] != httpPath || resources[1] != "/test.yml" {
t.Fatalf("incorrect resources: %v", resources)
}
resources, err = GlobPatternsWithLoader(fSys, ldr, []string{httpPath, "/test.yml"}, false)
require.NoError(t, err, "unexpected load error")
require.Equal(t, 2, len(resources), "incorrect resources")
require.Equal(t, httpPath, resources[0], "incorrect resources")
require.Equal(t, "/test.yml", resources[1], "incorrect resources")

// test load invalid file
invalidURL := "http://invalid"
resources, err = GlobPatternsWithLoader(fSys, ldr, []string{invalidURL})
if err == nil {
t.Fatalf("expected error but did not receive one")
} else if err.Error() != invalidURL+" has no match: "+invalidURL+" not exist" {
t.Fatalf("unexpected load error: %v", err)
}
if len(resources) > 0 {
t.Fatalf("incorrect resources: %v", resources)
}
resources, err = GlobPatternsWithLoader(fSys, ldr, []string{invalidURL}, false)
require.Error(t, err, "expected error but did not receive one")
require.Equal(t, invalidURL+" has no match: "+invalidURL+" not exist", err.Error(), "unexpected load error")
require.Equal(t, 0, len(resources), "incorrect resources")

// test load unreachable remote file with skipped verification
resources, err = GlobPatternsWithLoader(fSys, ldr, []string{invalidURL}, true)
require.NoError(t, err, "unexpected load error")
require.Equal(t, 1, len(resources), "incorrect resources")
require.Equal(t, invalidURL, resources[0], "incorrect resources")
Copy link
Contributor

@natasha41575 natasha41575 Oct 2, 2023

Choose a reason for hiding this comment

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

Thanks for fixing up these tests and adding one for this feature.

I think it might be nice to rewrite these tests using test structs and subtests, here is an example of this for the remove tests, so that it is easier to read and distinguish each test. You don't need to do this as part of this PR, but it would be a welcome change if you'd like to submit a follow-up PR to do so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was also thinking about that when I refactored the tests. Let me tackle that in a separate PR.

}

type fakeLoader struct {
Expand Down
Loading