From ff6cd3ca553dcec79bd328a2fea95e4eef01becf Mon Sep 17 00:00:00 2001 From: Jeffrey Regan Date: Tue, 26 Feb 2019 17:19:24 -0800 Subject: [PATCH] Report unused variables. --- pkg/expansion/expand.go | 5 +- pkg/expansion/expand_test.go | 80 +++++++++++++++++++++++++------ pkg/target/resaccumulator.go | 17 ++++++- pkg/target/resaccumulator_test.go | 47 ++++++++++++++++++ pkg/transformers/refvars.go | 44 +++++++++++------ pkg/transformers/refvars_test.go | 10 ++-- 6 files changed, 167 insertions(+), 36 deletions(-) diff --git a/pkg/expansion/expand.go b/pkg/expansion/expand.go index c4f0e0ba35..de55e46148 100644 --- a/pkg/expansion/expand.go +++ b/pkg/expansion/expand.go @@ -36,11 +36,14 @@ func syntaxWrap(input string) string { // implements the expansion semantics defined in the expansion spec; it // returns the input string wrapped in the expansion syntax if no mapping // for the input is found. -func MappingFuncFor(context ...map[string]string) func(string) string { +func MappingFuncFor( + counts map[string]int, + context ...map[string]string) func(string) string { return func(input string) string { for _, vars := range context { val, ok := vars[input] if ok { + counts[input]++ return val } } diff --git a/pkg/expansion/expand_test.go b/pkg/expansion/expand_test.go index 4b6a982117..2a8a131253 100644 --- a/pkg/expansion/expand_test.go +++ b/pkg/expansion/expand_test.go @@ -14,12 +14,19 @@ See the License for the specific language governing permissions and limitations under the License. */ -package expansion +package expansion_test import ( "testing" + + . "sigs.k8s.io/kustomize/pkg/expansion" ) +type expected struct { + count int + edited string +} + func TestMapReference(t *testing.T) { type env struct { Name string @@ -46,21 +53,23 @@ func TestMapReference(t *testing.T) { "BLU": "$(ZOO)-2", } - mapping := MappingFuncFor(declaredEnv) + counts := make(map[string]int) + mapping := MappingFuncFor(counts, declaredEnv) for _, env := range envs { declaredEnv[env.Name] = Expand(env.Value, mapping) } - expectedEnv := map[string]string{ - "FOO": "bar", - "ZOO": "bar-1", - "BLU": "bar-1-2", + expectedEnv := map[string]expected{ + "FOO": {count: 1, edited: "bar"}, + "ZOO": {count: 1, edited: "bar-1"}, + "BLU": {count: 0, edited: "bar-1-2"}, } for k, v := range expectedEnv { - if e, a := v, declaredEnv[k]; e != a { - t.Errorf("Expected %v, got %v", e, a) + if e, a := v, declaredEnv[k]; e.edited != a || e.count != counts[k] { + t.Errorf("Expected %v count=%d, got %v count=%d", + e.edited, e.count, a, counts[k]) } else { delete(declaredEnv, k) } @@ -79,9 +88,7 @@ func TestMapping(t *testing.T) { "VAR_REF": "$(VAR_A)", "VAR_EMPTY": "", } - mapping := MappingFuncFor(context) - - doExpansionTest(t, mapping) + doExpansionTest(t, context) } func TestMappingDual(t *testing.T) { @@ -94,51 +101,64 @@ func TestMappingDual(t *testing.T) { "VAR_C": "C", "VAR_REF": "$(VAR_A)", } - mapping := MappingFuncFor(context, context2) - doExpansionTest(t, mapping) + doExpansionTest(t, context, context2) } -func doExpansionTest(t *testing.T, mapping func(string) string) { +func doExpansionTest(t *testing.T, context ...map[string]string) { cases := []struct { name string input string expected string + counts map[string]int }{ { name: "whole string", input: "$(VAR_A)", expected: "A", + counts: map[string]int{"VAR_A": 1}, }, { name: "repeat", input: "$(VAR_A)-$(VAR_A)", expected: "A-A", + counts: map[string]int{"VAR_A": 2}, + }, + { + name: "multiple repeats", + input: "$(VAR_A)-$(VAR_B)-$(VAR_B)-$(VAR_B)-$(VAR_A)", + expected: "A-B-B-B-A", + counts: map[string]int{"VAR_A": 2, "VAR_B": 3}, }, { name: "beginning", input: "$(VAR_A)-1", expected: "A-1", + counts: map[string]int{"VAR_A": 1}, }, { name: "middle", input: "___$(VAR_B)___", expected: "___B___", + counts: map[string]int{"VAR_B": 1}, }, { name: "end", input: "___$(VAR_C)", expected: "___C", + counts: map[string]int{"VAR_C": 1}, }, { name: "compound", input: "$(VAR_A)_$(VAR_B)_$(VAR_C)", expected: "A_B_C", + counts: map[string]int{"VAR_A": 1, "VAR_B": 1, "VAR_C": 1}, }, { name: "escape & expand", input: "$$(VAR_B)_$(VAR_A)", expected: "$(VAR_B)_A", + counts: map[string]int{"VAR_A": 1}, }, { name: "compound escape", @@ -154,16 +174,19 @@ func doExpansionTest(t *testing.T, mapping func(string) string) { name: "backslash escape ignored", input: "foo\\$(VAR_C)bar", expected: "foo\\Cbar", + counts: map[string]int{"VAR_C": 1}, }, { name: "backslash escape ignored", input: "foo\\\\$(VAR_C)bar", expected: "foo\\\\Cbar", + counts: map[string]int{"VAR_C": 1}, }, { name: "lots of backslashes", input: "foo\\\\\\\\$(VAR_A)bar", expected: "foo\\\\\\\\Abar", + counts: map[string]int{"VAR_A": 1}, }, { name: "nested var references", @@ -179,16 +202,19 @@ func doExpansionTest(t *testing.T, mapping func(string) string) { name: "value is a reference", input: "$(VAR_REF)", expected: "$(VAR_A)", + counts: map[string]int{"VAR_REF": 1}, }, { name: "value is a reference x 2", input: "%%$(VAR_REF)--$(VAR_REF)%%", expected: "%%$(VAR_A)--$(VAR_A)%%", + counts: map[string]int{"VAR_REF": 2}, }, { name: "empty var", input: "foo$(VAR_EMPTY)bar", expected: "foobar", + counts: map[string]int{"VAR_EMPTY": 1}, }, { name: "unterminated expression", @@ -234,6 +260,7 @@ func doExpansionTest(t *testing.T, mapping func(string) string) { name: "multiple (odd) operators, var defined", input: "$$$$$$$(VAR_A)", expected: "$$$A", + counts: map[string]int{"VAR_A": 1}, }, { name: "missing open expression", @@ -249,16 +276,19 @@ func doExpansionTest(t *testing.T, mapping func(string) string) { name: "trailing incomplete expression not consumed", input: "$(VAR_B)_______$(A", expected: "B_______$(A", + counts: map[string]int{"VAR_B": 1}, }, { name: "trailing incomplete expression, no content, is not consumed", input: "$(VAR_C)_______$(", expected: "C_______$(", + counts: map[string]int{"VAR_C": 1}, }, { name: "operator at end of input string is preserved", input: "$(VAR_A)foobarzab$", expected: "Afoobarzab$", + counts: map[string]int{"VAR_A": 1}, }, { name: "shell escaped incomplete expr", @@ -293,9 +323,31 @@ func doExpansionTest(t *testing.T, mapping func(string) string) { } for _, tc := range cases { + counts := make(map[string]int) + mapping := MappingFuncFor(counts, context...) expanded := Expand(tc.input, mapping) if e, a := tc.expected, expanded; e != a { t.Errorf("%v: expected %q, got %q", tc.name, e, a) } + if len(counts) != len(tc.counts) { + t.Errorf("%v: len(counts)=%d != len(tc.counts)=%d", + tc.name, len(counts), len(tc.counts)) + } + if len(tc.counts) > 0 { + for k, expectedCount := range tc.counts { + c, ok := counts[k] + if ok { + if c != expectedCount { + t.Errorf( + "%v: k=%s, expected count %d, got %d", + tc.name, k, expectedCount, c) + } + } else { + t.Errorf( + "%v: k=%s, expected count %d, got zero", + tc.name, k, expectedCount) + } + } + } } } diff --git a/pkg/target/resaccumulator.go b/pkg/target/resaccumulator.go index 7894a8fb29..b8c45015a5 100644 --- a/pkg/target/resaccumulator.go +++ b/pkg/target/resaccumulator.go @@ -18,6 +18,9 @@ package target import ( "fmt" + "log" + "strings" + "sigs.k8s.io/kustomize/pkg/resid" "sigs.k8s.io/kustomize/pkg/resmap" "sigs.k8s.io/kustomize/pkg/transformers" @@ -135,8 +138,18 @@ func (ra *ResAccumulator) ResolveVars() error { if err != nil { return err } - return ra.Transform(transformers.NewRefVarTransformer( - replacementMap, ra.tConfig.VarReference)) + if len(replacementMap) == 0 { + return nil + } + t := transformers.NewRefVarTransformer( + replacementMap, ra.tConfig.VarReference) + err = ra.Transform(t) + if len(t.UnusedVars()) > 0 { + log.Printf( + "well-defined vars that were never replaced: %s\n", + strings.Join(t.UnusedVars(), ",")) + } + return err } func (ra *ResAccumulator) FixBackReferences() (err error) { diff --git a/pkg/target/resaccumulator_test.go b/pkg/target/resaccumulator_test.go index 55dab77f6c..42b61cc654 100644 --- a/pkg/target/resaccumulator_test.go +++ b/pkg/target/resaccumulator_test.go @@ -17,6 +17,9 @@ limitations under the License. package target_test import ( + "bytes" + "log" + "os" "strings" "testing" @@ -120,6 +123,50 @@ func TestResolveVarsHappy(t *testing.T) { } } +func TestResolveVarsOneUnused(t *testing.T) { + ra, _, err := makeResAccumulator() + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + err = ra.MergeVars([]types.Var{ + { + Name: "SERVICE_ONE", + ObjRef: types.Target{ + Gvk: gvk.Gvk{Version: "v1", Kind: "Service"}, + Name: "backendOne"}, + }, + { + Name: "SERVICE_UNUSED", + ObjRef: types.Target{ + Gvk: gvk.Gvk{Version: "v1", Kind: "Service"}, + Name: "backendTwo"}, + }, + }) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + err = ra.ResolveVars() + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + expectLog(t, buf, "well-defined vars that were never replaced: SERVICE_UNUSED") + c := getCommand(find("deploy1", ra.ResMap())) + if c != "myserver --somebackendService backendOne --yetAnother $(SERVICE_TWO)" { + t.Fatalf("unexpected command: %s", c) + } +} + +func expectLog(t *testing.T, log bytes.Buffer, expect string) { + if !strings.Contains(log.String(), expect) { + t.Fatalf("expected log containing '%s', got '%s'", expect, log.String()) + } +} + func TestResolveVarsVarNeedsDisambiguation(t *testing.T) { ra, rf, err := makeResAccumulator() if err != nil { diff --git a/pkg/transformers/refvars.go b/pkg/transformers/refvars.go index cace71f166..b31ec6e7a2 100644 --- a/pkg/transformers/refvars.go +++ b/pkg/transformers/refvars.go @@ -2,28 +2,26 @@ package transformers import ( "fmt" - "sigs.k8s.io/kustomize/pkg/expansion" "sigs.k8s.io/kustomize/pkg/resmap" "sigs.k8s.io/kustomize/pkg/transformers/config" ) -type refvarTransformer struct { - fieldSpecs []config.FieldSpec - mappingFunc func(string) string +type RefVarTransformer struct { + varMap map[string]string + replacementCounts map[string]int + fieldSpecs []config.FieldSpec + mappingFunc func(string) string } -// NewRefVarTransformer returns a Transformer that replaces $(VAR) style -// variables with values. +// NewRefVarTransformer returns a new RefVarTransformer +// that replaces $(VAR) style variables with values. // The fieldSpecs are the places to look for occurrences of $(VAR). func NewRefVarTransformer( - varMap map[string]string, fs []config.FieldSpec) Transformer { - if len(varMap) == 0 { - return NewNoOpTransformer() - } - return &refvarTransformer{ - fieldSpecs: fs, - mappingFunc: expansion.MappingFuncFor(varMap), + varMap map[string]string, fs []config.FieldSpec) *RefVarTransformer { + return &RefVarTransformer{ + varMap: varMap, + fieldSpecs: fs, } } @@ -31,7 +29,7 @@ func NewRefVarTransformer( // embedded instances of $VAR style variables, e.g. a container command string. // The function returns the string with the variables expanded to their final // values. -func (rv *refvarTransformer) replaceVars(in interface{}) (interface{}, error) { +func (rv *RefVarTransformer) replaceVars(in interface{}) (interface{}, error) { switch vt := in.(type) { case []interface{}: var xs []string @@ -63,8 +61,24 @@ func (rv *refvarTransformer) replaceVars(in interface{}) (interface{}, error) { } } +// UnusedVars returns slice of Var names that were unused +// after a Transform run. +func (rv *RefVarTransformer) UnusedVars() []string { + var unused []string + for k := range rv.varMap { + _, ok := rv.replacementCounts[k] + if !ok { + unused = append(unused, k) + } + } + return unused +} + // Transform replaces $(VAR) style variables with values. -func (rv *refvarTransformer) Transform(m resmap.ResMap) error { +func (rv *RefVarTransformer) Transform(m resmap.ResMap) error { + rv.replacementCounts = make(map[string]int) + rv.mappingFunc = expansion.MappingFuncFor( + rv.replacementCounts, rv.varMap) for id, res := range m { for _, fieldSpec := range rv.fieldSpecs { if id.Gvk().IsSelected(&fieldSpec.Gvk) { diff --git a/pkg/transformers/refvars_test.go b/pkg/transformers/refvars_test.go index fd2e9695b2..6e6826fadd 100644 --- a/pkg/transformers/refvars_test.go +++ b/pkg/transformers/refvars_test.go @@ -5,7 +5,6 @@ import ( "testing" "sigs.k8s.io/kustomize/pkg/resid" - "sigs.k8s.io/kustomize/pkg/resmap" "sigs.k8s.io/kustomize/pkg/transformers/config" ) @@ -17,7 +16,8 @@ func TestVarRef(t *testing.T) { res resmap.ResMap } type expected struct { - res resmap.ResMap + res resmap.ResMap + unused []string } testCases := []struct { description string @@ -28,7 +28,8 @@ func TestVarRef(t *testing.T) { description: "var replacement in map[string]", given: given{ varMap: map[string]string{ - "FOO": "BAR", + "FOO": "replacementForFoo", + "BAR": "replacementForBar", }, fs: []config.FieldSpec{ {Gvk: cmap, Path: "data"}, @@ -58,11 +59,12 @@ func TestVarRef(t *testing.T) { "name": "cm1", }, "data": map[string]interface{}{ - "item1": "BAR", + "item1": "replacementForFoo", "item2": "bla", }, }), }, + unused: []string{"BAR"}, }, }, }