From e771902a076e3acf22d8adead5c176e14b3af5d1 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Mon, 4 May 2020 22:25:57 -0700 Subject: [PATCH 1/4] Avoid Setter and Substitution with same name --- cmd/config/go.mod | 1 + cmd/config/go.sum | 2 + .../internal/commands/cmdcreatesetter.go | 29 ++++++++++++-- .../internal/commands/cmdcreatesetter_test.go | 40 +++++++++++++++++-- .../commands/cmdcreatesubstitution.go | 19 +++++++++ .../commands/cmdcreatesubstitution_test.go | 26 ++++++++++++ 6 files changed, 110 insertions(+), 7 deletions(-) diff --git a/cmd/config/go.mod b/cmd/config/go.mod index 3743484ce..d803e8a0a 100644 --- a/cmd/config/go.mod +++ b/cmd/config/go.mod @@ -6,6 +6,7 @@ require ( github.com/coreos/go-etcd v2.0.0+incompatible // indirect github.com/cpuguy83/go-md2man v1.0.10 // indirect github.com/go-errors/errors v1.0.1 + github.com/go-openapi/spec v0.19.5 github.com/olekukonko/tablewriter v0.0.4 github.com/posener/complete/v2 v2.0.1-alpha.12 github.com/spf13/cobra v1.0.0 diff --git a/cmd/config/go.sum b/cmd/config/go.sum index 3dc08d7c2..23657cce4 100644 --- a/cmd/config/go.sum +++ b/cmd/config/go.sum @@ -173,6 +173,7 @@ github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B github.com/spf13/cast v1.3.0/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE= github.com/spf13/cobra v0.0.5 h1:f0B+LkLX6DtmRH1isoNA9VTtNUK9K8xYd28JNNfOv/s= github.com/spf13/cobra v0.0.5/go.mod h1:3K3wKZymM7VvHMDS9+Akkh4K60UwM26emMESw8tLCHU= +github.com/spf13/cobra v1.0.0 h1:6m/oheQuQ13N9ks4hubMG6BnvwOeaJrqSPLahSnczz8= github.com/spf13/cobra v1.0.0/go.mod h1:/6GTrnGXV9HjY+aR4k0oJ5tcvakLuG6EuKReYlHNrgE= github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo= github.com/spf13/pflag v0.0.0-20170130214245-9ff6c6923cff/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= @@ -225,6 +226,7 @@ golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLL golang.org/x/net v0.0.0-20190827160401-ba9fcec4b297/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20191004110552-13f9640d40b9 h1:rjwSpXsdiK0dV8/Naq3kAw9ymfAeJIyd0upUIElB+lI= golang.org/x/net v0.0.0-20191004110552-13f9640d40b9/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20200226121028-0de0cce0169b h1:0mm1VjtFUOIlE1SbDlwjYaDxZVDP2S5ou6y0gSgXHu8= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= diff --git a/cmd/config/internal/commands/cmdcreatesetter.go b/cmd/config/internal/commands/cmdcreatesetter.go index ec4297eb3..c74992a67 100644 --- a/cmd/config/internal/commands/cmdcreatesetter.go +++ b/cmd/config/internal/commands/cmdcreatesetter.go @@ -4,11 +4,17 @@ package commands import ( + "fmt" + + "github.com/go-openapi/spec" "github.com/spf13/cobra" "sigs.k8s.io/kustomize/cmd/config/ext" "sigs.k8s.io/kustomize/cmd/config/internal/generateddocs/commands" + "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/kio" + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/setters" + "sigs.k8s.io/kustomize/kyaml/setters2" "sigs.k8s.io/kustomize/kyaml/setters2/settersutil" ) @@ -86,12 +92,29 @@ func (r *CreateSetterRunner) preRunE(c *cobra.Command, args []string) error { if setterVersion == "v2" { var err error r.OpenAPIFile, err = ext.GetOpenAPIFile(args) - r.CreateSetter.Description = r.Set.SetPartialField.Description - r.CreateSetter.SetBy = r.Set.SetPartialField.SetBy - r.CreateSetter.Type = r.Set.SetPartialField.Type if err != nil { return err } + + if err := openapi.AddSchemaFromFile(r.OpenAPIFile); err != nil { + return err + } + + // check if substitution with same name exists and throw error + ref, err := spec.NewRef(setters2.DefinitionsPrefix + setters2.SubstitutionDefinitionPrefix + r.CreateSetter.Name) + if err != nil { + return err + } + + _, err = openapi.Resolve(&ref) + if err == nil { + return errors.Errorf(fmt.Sprintf("substitution with name %s already exists, "+ + "substitution and setter can't have same name", r.CreateSetter.Name)) + } + + r.CreateSetter.Description = r.Set.SetPartialField.Description + r.CreateSetter.SetBy = r.Set.SetPartialField.SetBy + r.CreateSetter.Type = r.Set.SetPartialField.Type } return nil } diff --git a/cmd/config/internal/commands/cmdcreatesetter_test.go b/cmd/config/internal/commands/cmdcreatesetter_test.go index ac4141cd0..9ec0e9451 100644 --- a/cmd/config/internal/commands/cmdcreatesetter_test.go +++ b/cmd/config/internal/commands/cmdcreatesetter_test.go @@ -22,8 +22,10 @@ func TestCreateSetterCommand(t *testing.T) { input string args []string out string + inputOpenAPI string expectedOpenAPI string expectedResources string + err string }{ { name: "add replicas", @@ -36,6 +38,10 @@ metadata: spec: replicas: 3 `, + inputOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +`, expectedOpenAPI: ` apiVersion: v1alpha1 kind: Example @@ -58,6 +64,27 @@ spec: replicas: 3 # {"$ref":"#/definitions/io.k8s.cli.setters.replicas"} `, }, + { + name: "error if substitution with same name exists", + args: []string{"my-image", "3", "--description", "hello world", "--set-by", "me"}, + inputOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +openAPI: + definitions: + io.k8s.cli.substitutions.my-image: + x-k8s-cli: + substitution: + name: my-image + pattern: something/${my-image-setter}::${my-tag-setter}/nginxotherthing + values: + - marker: ${my-image-setter} + ref: '#/definitions/io.k8s.cli.setters.my-image-setter' + - marker: ${my-tag-setter} + ref: '#/definitions/io.k8s.cli.setters.my-tag-setter' + `, + err: "substitution with name my-image already exists, substitution and setter can't have same name", + }, } for i := range tests { test := tests[i] @@ -71,10 +98,7 @@ spec: t.FailNow() } defer os.Remove(f.Name()) - err = ioutil.WriteFile(f.Name(), []byte(` -apiVersion: v1alpha1 -kind: Example -`), 0600) + err = ioutil.WriteFile(f.Name(), []byte(test.inputOpenAPI), 0600) if !assert.NoError(t, err) { t.FailNow() } @@ -99,6 +123,14 @@ kind: Example runner.Command.SetOut(out) runner.Command.SetArgs(append([]string{r.Name()}, test.args...)) err = runner.Command.Execute() + if test.err != "" { + if !assert.NotNil(t, err) { + t.FailNow() + } else { + assert.Equal(t, err.Error(), test.err) + return + } + } if !assert.NoError(t, err) { t.FailNow() } diff --git a/cmd/config/internal/commands/cmdcreatesubstitution.go b/cmd/config/internal/commands/cmdcreatesubstitution.go index d7a675a92..a51d7d9db 100644 --- a/cmd/config/internal/commands/cmdcreatesubstitution.go +++ b/cmd/config/internal/commands/cmdcreatesubstitution.go @@ -4,12 +4,15 @@ package commands import ( + "fmt" "regexp" "strings" + "github.com/go-openapi/spec" "github.com/spf13/cobra" "sigs.k8s.io/kustomize/cmd/config/ext" "sigs.k8s.io/kustomize/kyaml/errors" + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/setters2" "sigs.k8s.io/kustomize/kyaml/setters2/settersutil" ) @@ -63,6 +66,22 @@ func (r *CreateSubstitutionRunner) preRunE(c *cobra.Command, args []string) erro return err } + if err := openapi.AddSchemaFromFile(r.OpenAPIFile); err != nil { + return err + } + + // check if setter with same name exists and throw error + ref, err := spec.NewRef(setters2.DefinitionsPrefix + setters2.SetterDefinitionPrefix + r.CreateSubstitution.Name) + if err != nil { + return err + } + + _, err = openapi.Resolve(&ref) // resolve the setter to its openAPI def + if err == nil { + return errors.Errorf(fmt.Sprintf("setter with name %s already exists, "+ + "substitution and setter can't have same name", r.CreateSubstitution.Name)) + } + // extract setter name tokens from pattern enclosed in ${} re := regexp.MustCompile(`\$\{([^}]*)\}`) markers := re.FindAll([]byte(r.CreateSubstitution.Pattern), -1) diff --git a/cmd/config/internal/commands/cmdcreatesubstitution_test.go b/cmd/config/internal/commands/cmdcreatesubstitution_test.go index 09a60c4bd..90ffc854b 100644 --- a/cmd/config/internal/commands/cmdcreatesubstitution_test.go +++ b/cmd/config/internal/commands/cmdcreatesubstitution_test.go @@ -25,6 +25,7 @@ func TestCreateSubstitutionCommand(t *testing.T) { out string expectedOpenAPI string expectedResources string + err string }{ { name: "substitution replicas", @@ -103,6 +104,23 @@ spec: image: sidecar:1.7.9 `, }, + { + name: "error if setter with same name exists", + args: []string{ + "my-image", "--field-value", "nginx:1.7.9", "--pattern", "${my-image-setter}:${my-tag-setter}"}, + inputOpenAPI: ` +apiVersion: v1alpha1 +kind: Example +openAPI: + definitions: + io.k8s.cli.setters.my-image: + x-k8s-cli: + setter: + name: my-image + value: "nginx" + `, + err: "setter with name my-image already exists, substitution and setter can't have same name", + }, { name: "substitution and create setters 1", args: []string{ @@ -206,6 +224,14 @@ spec: runner.Command.SetOut(out) runner.Command.SetArgs(append([]string{r.Name()}, test.args...)) err = runner.Command.Execute() + + if test.err != "" { + if !assert.NotNil(t, err) { + t.FailNow() + } + assert.Equal(t, err.Error(), test.err) + return + } if !assert.NoError(t, err) { t.FailNow() } From 6d926088771931c05575d1f586f8e8f6428da5fc Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Tue, 5 May 2020 15:29:38 -0700 Subject: [PATCH 2/4] List substitutions --- cmd/config/go.mod | 4 +- cmd/config/go.sum | 2 + .../internal/commands/cmdlistsetters.go | 99 +++++++++++++------ .../internal/commands/cmdlistsetters_test.go | 6 ++ kyaml/setters2/list.go | 82 ++++++++++++++- kyaml/setters2/list_test.go | 2 +- 6 files changed, 159 insertions(+), 36 deletions(-) diff --git a/cmd/config/go.mod b/cmd/config/go.mod index 3743484ce..8b5dcb00f 100644 --- a/cmd/config/go.mod +++ b/cmd/config/go.mod @@ -13,7 +13,7 @@ require ( github.com/stretchr/testify v1.4.0 github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8 // indirect k8s.io/apimachinery v0.17.0 - sigs.k8s.io/kustomize/kyaml v0.0.0 + sigs.k8s.io/kustomize/kyaml v0.1.7 ) -replace sigs.k8s.io/kustomize/kyaml v0.0.0 => ../../kyaml +replace sigs.k8s.io/kustomize/kyaml v0.1.7 => ../../kyaml diff --git a/cmd/config/go.sum b/cmd/config/go.sum index 3dc08d7c2..23657cce4 100644 --- a/cmd/config/go.sum +++ b/cmd/config/go.sum @@ -173,6 +173,7 @@ github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B github.com/spf13/cast v1.3.0/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE= github.com/spf13/cobra v0.0.5 h1:f0B+LkLX6DtmRH1isoNA9VTtNUK9K8xYd28JNNfOv/s= github.com/spf13/cobra v0.0.5/go.mod h1:3K3wKZymM7VvHMDS9+Akkh4K60UwM26emMESw8tLCHU= +github.com/spf13/cobra v1.0.0 h1:6m/oheQuQ13N9ks4hubMG6BnvwOeaJrqSPLahSnczz8= github.com/spf13/cobra v1.0.0/go.mod h1:/6GTrnGXV9HjY+aR4k0oJ5tcvakLuG6EuKReYlHNrgE= github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo= github.com/spf13/pflag v0.0.0-20170130214245-9ff6c6923cff/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= @@ -225,6 +226,7 @@ golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLL golang.org/x/net v0.0.0-20190827160401-ba9fcec4b297/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20191004110552-13f9640d40b9 h1:rjwSpXsdiK0dV8/Naq3kAw9ymfAeJIyd0upUIElB+lI= golang.org/x/net v0.0.0-20191004110552-13f9640d40b9/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20200226121028-0de0cce0169b h1:0mm1VjtFUOIlE1SbDlwjYaDxZVDP2S5ou6y0gSgXHu8= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/sync v0.0.0-20180314180146-1d60e4601c6f/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= diff --git a/cmd/config/internal/commands/cmdlistsetters.go b/cmd/config/internal/commands/cmdlistsetters.go index 566bcbd5b..d88a4802d 100644 --- a/cmd/config/internal/commands/cmdlistsetters.go +++ b/cmd/config/internal/commands/cmdlistsetters.go @@ -59,42 +59,83 @@ func (r *ListSettersRunner) preRunE(c *cobra.Command, args []string) error { func (r *ListSettersRunner) runE(c *cobra.Command, args []string) error { if setterVersion == "v2" { - // use setters v2 - path, err := ext.GetOpenAPIFile(args) - if err != nil { + if err := r.ListSetters(c, args); err != nil { return err } - if err := r.List.List(path, args[0]); err != nil { - return err - } - table := newTable(c.OutOrStdout(), r.Markdown) - table.SetHeader([]string{"NAME", "VALUE", "SET BY", "DESCRIPTION", "COUNT"}) - for i := range r.List.Setters { - s := r.List.Setters[i] - v := s.Value - - // if the setter is for a list, populate the values - if len(s.ListValues) > 0 { - v = strings.Join(s.ListValues, ",") - v = fmt.Sprintf("[%s]", v) - } - table.Append([]string{ - s.Name, v, s.SetBy, s.Description, fmt.Sprintf("%d", s.Count)}) - } - table.Render() - - if len(r.List.Setters) == 0 { - // exit non-0 if no matching setters are found - if ExitOnError { - os.Exit(1) - } - } - return nil + return r.ListSubstitutions(c, args) } return handleError(c, lookup(r.Lookup, c, args)) } +func (r *ListSettersRunner) ListSetters(c *cobra.Command, args []string) error { + // use setters v2 + path, err := ext.GetOpenAPIFile(args) + if err != nil { + return err + } + if err := r.List.ListSetters(path, args[0]); err != nil { + return err + } + table := newTable(c.OutOrStdout(), r.Markdown) + table.SetHeader([]string{"NAME", "VALUE", "SET BY", "DESCRIPTION", "COUNT"}) + for i := range r.List.Setters { + s := r.List.Setters[i] + v := s.Value + + // if the setter is for a list, populate the values + if len(s.ListValues) > 0 { + v = strings.Join(s.ListValues, ",") + v = fmt.Sprintf("[%s]", v) + } + table.Append([]string{ + s.Name, v, s.SetBy, s.Description, fmt.Sprintf("%d", s.Count)}) + } + table.Render() + + if len(r.List.Setters) == 0 { + // exit non-0 if no matching setters are found + if ExitOnError { + os.Exit(1) + } + } + return nil +} + +func (r *ListSettersRunner) ListSubstitutions(c *cobra.Command, args []string) error { + // use setters v2 + path, err := ext.GetOpenAPIFile(args) + if err != nil { + return err + } + if err := r.List.ListSubst(path); err != nil { + return err + } + table := newTable(c.OutOrStdout(), r.Markdown) + table.SetHeader([]string{"SUBSTITUTION", "PATTERN", "SETTERS"}) + for i := range r.List.Substitutions { + s := r.List.Substitutions[i] + setters := "" + for _, value := range s.Values { + setter := strings.TrimPrefix(value.Ref, setters2.DefinitionsPrefix+setters2.SetterDefinitionPrefix) + setters = setters + "," + setter + } + setters = fmt.Sprintf("[%s]", strings.TrimPrefix(setters, ",")) + table.Append([]string{ + s.Name, s.Pattern, setters}) + } + if len(r.List.Substitutions) == 0 { + // exit non-0 if no matching substitutions are found + if ExitOnError { + os.Exit(1) + } + } else { + table.Render() + } + + return nil +} + func newTable(o io.Writer, m bool) *tablewriter.Table { table := tablewriter.NewWriter(o) table.SetRowLine(false) diff --git a/cmd/config/internal/commands/cmdlistsetters_test.go b/cmd/config/internal/commands/cmdlistsetters_test.go index dbd97a2b4..77d475a73 100644 --- a/cmd/config/internal/commands/cmdlistsetters_test.go +++ b/cmd/config/internal/commands/cmdlistsetters_test.go @@ -104,6 +104,8 @@ spec: image nginx me2 hello world 2 2 replicas 3 me1 hello world 1 1 tag 1.7.9 me3 hello world 3 1 + SUBSTITUTION PATTERN SETTERS + image IMAGE:TAG [image,tag] `, }, { @@ -176,6 +178,8 @@ spec: image nginx me2 hello world 2 3 replicas 3 me1 hello world 1 2 tag 1.7.9 me3 hello world 3 2 + SUBSTITUTION PATTERN SETTERS + image IMAGE:TAG [image,tag] `, }, { @@ -247,6 +251,8 @@ spec: `, expected: ` NAME VALUE SET BY DESCRIPTION COUNT image nginx me2 hello world 2 3 + SUBSTITUTION PATTERN SETTERS + image IMAGE:TAG [image,tag] `, }, } diff --git a/kyaml/setters2/list.go b/kyaml/setters2/list.go index a0436732f..ca00f944c 100644 --- a/kyaml/setters2/list.go +++ b/kyaml/setters2/list.go @@ -18,10 +18,12 @@ type List struct { Name string Setters []SetterDefinition + + Substitutions []SubstitutionDefinition } -// List initializes l.Setters with the setters from the OpenAPI definitions in the file -func (l *List) List(openAPIPath, resourcePath string) error { +// ListSetters initializes l.Setters with the setters from the OpenAPI definitions in the file +func (l *List) ListSetters(openAPIPath, resourcePath string) error { if err := openapi.AddSchemaFromFile(openAPIPath); err != nil { return err } @@ -29,10 +31,22 @@ func (l *List) List(openAPIPath, resourcePath string) error { if err != nil { return err } - return l.list(y, resourcePath) + return l.listSetters(y, resourcePath) } -func (l *List) list(object *yaml.RNode, resourcePath string) error { +// ListSubst initializes l.Substitutions with the substitutions from the OpenAPI definitions in the file +func (l *List) ListSubst(openAPIPath string) error { + if err := openapi.AddSchemaFromFile(openAPIPath); err != nil { + return err + } + y, err := yaml.ReadFile(openAPIPath) + if err != nil { + return err + } + return l.listSubst(y) +} + +func (l *List) listSetters(object *yaml.RNode, resourcePath string) error { // read the OpenAPI definitions def, err := object.Pipe(yaml.LookupCreate(yaml.MappingNode, "openAPI", "definitions")) if err != nil { @@ -105,6 +119,66 @@ func (l *List) list(object *yaml.RNode, resourcePath string) error { return nil } +func (l *List) listSubst(object *yaml.RNode) error { + // read the OpenAPI definitions + def, err := object.Pipe(yaml.LookupCreate(yaml.MappingNode, "openAPI", "definitions")) + if err != nil { + return err + } + if yaml.IsEmpty(def) { + return nil + } + + // iterate over definitions -- find those that are substitutions + err = def.VisitFields(func(node *yaml.MapNode) error { + subst := SubstitutionDefinition{} + + // the definition key -- contains the substitution name + key := node.Key.YNode().Value + + if !strings.HasPrefix(key, SubstitutionDefinitionPrefix) { + // not a substitution -- doesn't have the right prefix + return nil + } + + substNode, err := node.Value.Pipe(yaml.Lookup(K8sCliExtensionKey, "substitution")) + if err != nil { + return err + } + if yaml.IsEmpty(substNode) { + // has the substitution prefix, but missing the setter extension + return errors.Errorf("missing x-k8s-cli.substitution for %s", key) + } + + // unmarshal the yaml for the substitution extension into the definition struct + b, err := substNode.String() + if err != nil { + return err + } + if err := yaml.Unmarshal([]byte(b), &subst); err != nil { + return err + } + + if l.Name != "" && l.Name != subst.Name { + // not the substitution that was requested by list + return nil + } + + l.Substitutions = append(l.Substitutions, subst) + return nil + }) + if err != nil { + return err + } + + // sort the substitutions by their name + sort.Slice(l.Substitutions, func(i, j int) bool { + return l.Substitutions[i].Name < l.Substitutions[j].Name + }) + + return nil +} + // count returns the number of fields set by the setter with name func (l *List) count(path, name string) (int, error) { s := &Set{Name: name} diff --git a/kyaml/setters2/list_test.go b/kyaml/setters2/list_test.go index 4b1a24874..291992060 100644 --- a/kyaml/setters2/list_test.go +++ b/kyaml/setters2/list_test.go @@ -276,7 +276,7 @@ spec: // invoke the setter instance := &List{Name: test.setter} - err = instance.List(f.Name(), r.Name()) + err = instance.ListSetters(f.Name(), r.Name()) if !assert.NoError(t, err) { t.FailNow() } From 7309a5cb0696aced99fd9aab6b7f31ce148c8de7 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Wed, 6 May 2020 12:38:12 -0700 Subject: [PATCH 3/4] Suggested Changes --- cmd/config/internal/commands/cmdcreatesetter.go | 11 +++++------ cmd/config/internal/commands/cmdcreatesubstitution.go | 5 +++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/config/internal/commands/cmdcreatesetter.go b/cmd/config/internal/commands/cmdcreatesetter.go index c74992a67..4a9db6a73 100644 --- a/cmd/config/internal/commands/cmdcreatesetter.go +++ b/cmd/config/internal/commands/cmdcreatesetter.go @@ -4,8 +4,6 @@ package commands import ( - "fmt" - "github.com/go-openapi/spec" "github.com/spf13/cobra" "sigs.k8s.io/kustomize/cmd/config/ext" @@ -106,10 +104,11 @@ func (r *CreateSetterRunner) preRunE(c *cobra.Command, args []string) error { return err } - _, err = openapi.Resolve(&ref) - if err == nil { - return errors.Errorf(fmt.Sprintf("substitution with name %s already exists, "+ - "substitution and setter can't have same name", r.CreateSetter.Name)) + subst, _ := openapi.Resolve(&ref) + // if substitution already exists with the input setter name, throw error + if subst != nil { + return errors.Errorf("substitution with name %s already exists, "+ + "substitution and setter can't have same name", r.CreateSetter.Name) } r.CreateSetter.Description = r.Set.SetPartialField.Description diff --git a/cmd/config/internal/commands/cmdcreatesubstitution.go b/cmd/config/internal/commands/cmdcreatesubstitution.go index a51d7d9db..b4cb47988 100644 --- a/cmd/config/internal/commands/cmdcreatesubstitution.go +++ b/cmd/config/internal/commands/cmdcreatesubstitution.go @@ -76,8 +76,9 @@ func (r *CreateSubstitutionRunner) preRunE(c *cobra.Command, args []string) erro return err } - _, err = openapi.Resolve(&ref) // resolve the setter to its openAPI def - if err == nil { + setter, _ := openapi.Resolve(&ref) + // if setter already exists with input substitution name, throw error + if setter != nil { return errors.Errorf(fmt.Sprintf("setter with name %s already exists, "+ "substitution and setter can't have same name", r.CreateSubstitution.Name)) } From 5bae01fa68ba8dd436eb6b6609f17ba7e09ee837 Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Thu, 7 May 2020 20:22:17 -0700 Subject: [PATCH 4/4] UX improvements to kyaml/fn/framework --- kyaml/fn/framework/doc.go | 43 +++++++-------- kyaml/fn/framework/example/main.go | 12 +++-- kyaml/fn/framework/example_test.go | 80 ++++++++++++++++------------ kyaml/fn/framework/framework.go | 40 ++++---------- kyaml/fn/framework/framework_test.go | 7 ++- kyaml/fn/framework/types.go | 2 +- 6 files changed, 86 insertions(+), 98 deletions(-) diff --git a/kyaml/fn/framework/doc.go b/kyaml/fn/framework/doc.go index f5b82fd18..2ef5336d9 100644 --- a/kyaml/fn/framework/doc.go +++ b/kyaml/fn/framework/doc.go @@ -6,7 +6,20 @@ // // Examples // -// Example function implementation using framework.ResourceList with functionConfig +// Example function implementation using framework.Command with flag input +// +// var value string +// resourceList := &framework.ResourceList{} +// cmd := framework.Command(resourceList, func() error { +// for i := range resourceList.Items { +// // modify the items... +// } +// return nil +// }) +// cmd.Flags().StringVar(&value, "value", "", "annotation value") +// if err := cmd.Execute(); err != nil { return err } +// +// Example function implementation using framework.ResourceList with a struct input // // type Spec struct { // Value string `yaml:"value,omitempty"` @@ -24,23 +37,11 @@ // } // if err := rl.Write(); err != nil { return err } // -// Example function implementation using framework.Command with flags -// -// var value string -// cmd := framework.Command(nil, func(items []*yaml.RNode) ([]*yaml.RNode, error) { -// for i := range items { -// // modify the items... -// } -// return items, nil -// }) -// cmd.Flags().StringVar(&value, "value", "", "annotation value") -// if err := cmd.Execute(); err != nil { return err } -// // Architecture // -// Functions modify a slice of resources (ResourceList.items) which are read as input and written +// Functions modify a slice of resources (ResourceList.Items) which are read as input and written // as output. The function itself may be configured through a functionConfig -// (ResourceList.functionConfig). +// (ResourceList.FunctionConfig). // // Example Function Input: // @@ -67,7 +68,7 @@ // # run the function by creating this container and providing this // # Example as the functionConfig // config.kubernetes.io/function: | -// image: image/containing/fuction:impl +// image: image/containing/function:impl // spec: // value: foo // @@ -77,7 +78,7 @@ // // Functions may also be specified imperatively and run using: // -// config run DIR/ --image image/containing/fuction:impl -- value=foo +// config run DIR/ --image image/containing/function:impl -- value=foo // // When run imperatively, a ConfigMap is generated for the functionConfig, and the command // arguments are set as ConfigMap data entries. @@ -91,15 +92,11 @@ // // Mutator and Generator Functions // -// Functions may add, delete or modify resources by modifying the items slice. -// When using framework.Command this is done through returning the new items slice. -// When using framework.ResourceList this is done through modifying ResourceList.Items in place. +// Functions may add, delete or modify resources by modifying the ResourceList.Items slice. // // Validator Functions // -// A function may validate resources by providing a Result. -// When using framework.Command this is done through returning a framework.Result as an error. -// WHen using framework.ResourceList this is done through setting ResourceList.Result. +// A function may emit validation results by setting the ResourceList.Result // // Configuring Functions // diff --git a/kyaml/fn/framework/example/main.go b/kyaml/fn/framework/example/main.go index c84f49bbe..239b6b7c7 100644 --- a/kyaml/fn/framework/example/main.go +++ b/kyaml/fn/framework/example/main.go @@ -12,14 +12,16 @@ import ( func main() { var value string - cmd := framework.Command(nil, func(items []*yaml.RNode) ([]*yaml.RNode, error) { - for i := range items { + resourceList := framework.ResourceList{} + cmd := framework.Command(&resourceList, func() error { + for i := range resourceList.Items { // set the annotation on each resource item - if err := items[i].PipeE(yaml.SetAnnotation("value", value)); err != nil { - return nil, err + err := resourceList.Items[i].PipeE(yaml.SetAnnotation("value", value)) + if err != nil { + return err } } - return items, nil + return nil }) cmd.Flags().StringVar(&value, "value", "", "annotation value") diff --git a/kyaml/fn/framework/example_test.go b/kyaml/fn/framework/example_test.go index d791eae67..3d5b37393 100644 --- a/kyaml/fn/framework/example_test.go +++ b/kyaml/fn/framework/example_test.go @@ -89,15 +89,17 @@ functionConfig: func ExampleCommand_modify() { // configure the annotation value using a flag parsed from // ResourceList.functionConfig.data.value + resourceList := framework.ResourceList{} var value string - cmd := framework.Command(nil, func(items []*yaml.RNode) ([]*yaml.RNode, error) { - for i := range items { + cmd := framework.Command(&resourceList, func() error { + for i := range resourceList.Items { // set the annotation on each resource item - if err := items[i].PipeE(yaml.SetAnnotation("value", value)); err != nil { - return nil, err + err := resourceList.Items[i].PipeE(yaml.SetAnnotation("value", value)) + if err != nil { + return err } } - return items, nil + return nil }) cmd.Flags().StringVar(&value, "value", "", "annotation value") @@ -164,12 +166,13 @@ func ExampleCommand_generateReplace() { functionConfig := &ExampleServiceGenerator{} // function implementation -- generate a Service resource - cmd := framework.Command(functionConfig, func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { + resourceList := &framework.ResourceList{FunctionConfig: functionConfig} + cmd := framework.Command(resourceList, func() error { var newNodes []*yaml.RNode - for i := range nodes { - meta, err := nodes[i].GetMeta() + for i := range resourceList.Items { + meta, err := resourceList.Items[i].GetMeta() if err != nil { - return nil, err + return err } // something we already generated, remove it from the list so we regenerate it @@ -178,7 +181,7 @@ func ExampleCommand_generateReplace() { meta.APIVersion == "v1" { continue } - newNodes = append(newNodes, nodes[i]) + newNodes = append(newNodes, resourceList.Items[i]) } // generate the resource @@ -188,9 +191,11 @@ metadata: name: %s `, functionConfig.Spec.Name)) if err != nil { - return nil, err + return err } - return append(newNodes, n), nil + newNodes = append(newNodes, n) + resourceList.Items = newNodes + return nil }) // for testing purposes only -- normally read from stdin when Executing @@ -341,12 +346,13 @@ func ExampleCommand_generateUpdate() { functionConfig := &ExampleServiceGenerator{} // function implementation -- generate or update a Service resource - cmd := framework.Command(functionConfig, func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { + resourceList := &framework.ResourceList{FunctionConfig: functionConfig} + cmd := framework.Command(resourceList, func() error { var found bool - for i := range nodes { - meta, err := nodes[i].GetMeta() + for i := range resourceList.Items { + meta, err := resourceList.Items[i].GetMeta() if err != nil { - return nil, err + return err } // something we already generated, reconcile it to make sure it matches what @@ -356,9 +362,9 @@ func ExampleCommand_generateUpdate() { meta.APIVersion == "v1" { // set some values for k, v := range functionConfig.Spec.Annotations { - err := nodes[i].PipeE(yaml.SetAnnotation(k, v)) + err := resourceList.Items[i].PipeE(yaml.SetAnnotation(k, v)) if err != nil { - return nil, err + return err } } found = true @@ -366,7 +372,7 @@ func ExampleCommand_generateUpdate() { } } if found { - return nodes, nil + return nil } // generate the resource if not found @@ -375,18 +381,18 @@ kind: Service metadata: name: %s `, functionConfig.Spec.Name)) + if err != nil { + return err + } for k, v := range functionConfig.Spec.Annotations { err := n.PipeE(yaml.SetAnnotation(k, v)) if err != nil { - return nil, err + return err } } - nodes = append(nodes, n) - if err != nil { - return nil, err - } + resourceList.Items = append(resourceList.Items, n) - return nodes, nil + return nil }) // for testing purposes only -- normally read from stdin when Executing @@ -445,25 +451,26 @@ functionConfig: // If any Deployments do not contain spec.replicas, then the function will return results // which will be set on ResourceList.results func ExampleCommand_validate() { - cmd := framework.Command(nil, func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { + resourceList := &framework.ResourceList{} + cmd := framework.Command(resourceList, func() error { // validation results var validationResults []framework.Item // validate that each Deployment resource has spec.replicas set - for i := range nodes { + for i := range resourceList.Items { // only check Deployment resources - meta, err := nodes[i].GetMeta() + meta, err := resourceList.Items[i].GetMeta() if err != nil { - return nil, err + return err } if meta.Kind != "Deployment" { continue } // lookup replicas field - r, err := nodes[i].Pipe(yaml.Lookup("spec", "replicas")) + r, err := resourceList.Items[i].Pipe(yaml.Lookup("spec", "replicas")) if err != nil { - return nil, err + return err } // check replicas not specified @@ -481,11 +488,14 @@ func ExampleCommand_validate() { }) } - // framework will only consider results an error if it has at least 1 item - return nodes, framework.Result{ - Name: "replicas-validator", - Items: validationResults, + if len(validationResults) > 0 { + resourceList.Result = &framework.Result{ + Name: "replicas-validator", + Items: validationResults, + } } + + return resourceList.Result }) // for testing purposes only -- normally read from stdin when Executing diff --git a/kyaml/fn/framework/framework.go b/kyaml/fn/framework/framework.go index c89246cec..5f1dc65b8 100644 --- a/kyaml/fn/framework/framework.go +++ b/kyaml/fn/framework/framework.go @@ -163,20 +163,18 @@ func (r *ResourceList) Write() error { // Command returns a cobra.Command to run a function. // -// The cobra.Command will use a ResourceList to Read() the input, run the provided function, -// and Write() the output. -// -// If functionConfig is non-nil, the ResourceList.functionConfig will be unmarshalled into it. +// The cobra.Command will use the provided ResourceList to Read() the input, +// run the provided function, and then Write() the output. // // The returned cobra.Command will have a "gen" subcommand which can be used to generate // a Dockerfile to build the function into a container image // // go run main.go gen DIR/ -func Command(functionConfig interface{}, function Function) cobra.Command { +func Command(resourceList *ResourceList, function Function) cobra.Command { cmd := cobra.Command{} AddGenerateDockerfile(&cmd) cmd.RunE = func(cmd *cobra.Command, args []string) error { - err := execute(function, functionConfig, cmd) + err := execute(resourceList, function, cmd) if err != nil { fmt.Fprintf(cmd.ErrOrStderr(), "%v", err) } @@ -209,38 +207,20 @@ CMD ["function"] cmd.AddCommand(gen) } -func execute(function Function, functionConfig interface{}, cmd *cobra.Command) error { - rl := ResourceList{ - FunctionConfig: functionConfig, - Flags: cmd.Flags(), - Writer: cmd.OutOrStdout(), - Reader: cmd.InOrStdin(), - } +func execute(rl *ResourceList, function Function, cmd *cobra.Command) error { + rl.Reader = cmd.InOrStdin() + rl.Writer = cmd.OutOrStdout() + rl.Flags = cmd.Flags() if err := rl.Read(); err != nil { return err } - // run the function implementation - var err error - rl.Items, err = function(rl.Items) - - // set the ResourceList.results for validating functions - var result *Result - if err != nil { - if val, ok := err.(Result); ok { - rl.Result = &val - } else { - return errors.Wrap(err) - } - } + retErr := function() if err := rl.Write(); err != nil { return err } - if result != nil && result.ExitCode() != 0 { - return errors.Wrap(err) - } - return nil + return retErr } diff --git a/kyaml/fn/framework/framework_test.go b/kyaml/fn/framework/framework_test.go index 01bbd9ba3..fabb7a258 100644 --- a/kyaml/fn/framework/framework_test.go +++ b/kyaml/fn/framework/framework_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/assert" "sigs.k8s.io/kustomize/kyaml/fn/framework" - "sigs.k8s.io/kustomize/kyaml/yaml" ) func TestCommand_dockerfile(t *testing.T) { @@ -22,9 +21,9 @@ func TestCommand_dockerfile(t *testing.T) { defer os.RemoveAll(d) // create a function - cmd := framework.Command(nil, func(nodes []*yaml.RNode) ([]*yaml.RNode, error) { - return nil, nil - }) + + resourceList := &framework.ResourceList{} + cmd := framework.Command(resourceList, func() error { return nil }) // generate the Dockerfile cmd.SetArgs([]string{"gen", d}) diff --git a/kyaml/fn/framework/types.go b/kyaml/fn/framework/types.go index 041ea2725..f734e19ba 100644 --- a/kyaml/fn/framework/types.go +++ b/kyaml/fn/framework/types.go @@ -11,7 +11,7 @@ import ( // Function defines a function which mutates or validates a collection of configuration // To create a structured validation result, return a Result as the error. -type Function func(nodes []*yaml.RNode) ([]*yaml.RNode, error) +type Function func() error // Result defines a function result which will be set on the emitted ResourceList type Result struct {