From 9a85071085269d0499d103ee31f5948207c79f69 Mon Sep 17 00:00:00 2001 From: jregan Date: Sun, 21 Apr 2019 13:33:14 -0700 Subject: [PATCH] Delete kustomizationerror. Do a longstanding TODO to remove kustomizationerror. It wasn't used much, and it wasn't used consistently, because it's complicated to decided when it's worth proceeding to accumulate errors when one already knows that one has a fatal error in the kustomization. Its use was blocking refactoring for simplicity and making tests harder to write. Removing it lets us reinstate the cyclomatic complexity check in KustTarget. Also added more info to the affected error messages. --- go.mod | 2 +- go.sum | 4 +- pkg/internal/error/kustomizationerror.go | 61 ------ pkg/internal/error/kustomizationerror_test.go | 89 --------- pkg/resmap/factory.go | 27 ++- pkg/resmap/factory_test.go | 6 +- pkg/target/kusttarget.go | 174 ++++++++++-------- 7 files changed, 110 insertions(+), 253 deletions(-) delete mode 100644 pkg/internal/error/kustomizationerror.go delete mode 100644 pkg/internal/error/kustomizationerror_test.go diff --git a/go.mod b/go.mod index da97c1e5b..c83b51083 100644 --- a/go.mod +++ b/go.mod @@ -23,7 +23,7 @@ require ( github.com/modern-go/reflect2 v0.0.0-20180228065516-1df9eeb2bb81 // indirect github.com/onsi/ginkgo v1.8.0 // indirect github.com/onsi/gomega v1.5.0 // indirect - github.com/pkg/errors v0.8.0 + github.com/pkg/errors v0.8.1 github.com/spf13/cobra v0.0.2 github.com/spf13/pflag v1.0.1 github.com/stretchr/testify v1.3.0 // indirect diff --git a/go.sum b/go.sum index 241ce5825..a12faa59e 100644 --- a/go.sum +++ b/go.sum @@ -47,8 +47,8 @@ github.com/onsi/ginkgo v1.8.0 h1:VkHVNpR4iVnU8XQR6DBm8BqYjN7CRzw+xKUbVVbbW9w= github.com/onsi/ginkgo v1.8.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= github.com/onsi/gomega v1.5.0 h1:izbySO9zDPmjJ8rDjLvkA2zJHIo+HkYXHnf7eN7SSyo= github.com/onsi/gomega v1.5.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= -github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw= -github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I= +github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/spf13/cobra v0.0.2 h1:NfkwRbgViGoyjBKsLI0QMDcuMnhM+SBg3T0cGfpvKDE= diff --git a/pkg/internal/error/kustomizationerror.go b/pkg/internal/error/kustomizationerror.go deleted file mode 100644 index 0d53ca9b8..000000000 --- a/pkg/internal/error/kustomizationerror.go +++ /dev/null @@ -1,61 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package error - -import ( - "fmt" -) - -// KustomizationError represents an error with a kustomization. -type KustomizationError struct { - KustomizationPath string - ErrorMsg string -} - -func (ke KustomizationError) Error() string { - return fmt.Sprintf("Kustomization File [%s]: %s\n", ke.KustomizationPath, ke.ErrorMsg) -} - -// KustomizationErrors collects all errors. -type KustomizationErrors struct { - kErrors []error -} - -func (ke *KustomizationErrors) Error() string { - errormsg := "" - for _, e := range ke.kErrors { - errormsg += e.Error() + "\n" - } - return errormsg -} - -// Append adds error to a collection of errors. -func (ke *KustomizationErrors) Append(e error) { - ke.kErrors = append(ke.kErrors, e) -} - -// Get returns all collected errors. -func (ke *KustomizationErrors) Get() []error { - return ke.kErrors -} - -// BatchAppend adds all errors from another KustomizationErrors -func (ke *KustomizationErrors) BatchAppend(e KustomizationErrors) { - for _, err := range e.Get() { - ke.kErrors = append(ke.kErrors, err) - } -} diff --git a/pkg/internal/error/kustomizationerror_test.go b/pkg/internal/error/kustomizationerror_test.go deleted file mode 100644 index bb6ed0dba..000000000 --- a/pkg/internal/error/kustomizationerror_test.go +++ /dev/null @@ -1,89 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package error - -import ( - "fmt" - "strings" - "testing" -) - -func TestKustomizationError_Error(t *testing.T) { - errorMsg := "Kustomization not found" - - me := KustomizationError{KustomizationPath: filepath, ErrorMsg: errorMsg} - - if !strings.Contains(me.Error(), filepath) { - t.Errorf("Incorrect KustomizationError.Error() message \n") - t.Errorf("Expected filepath %s, but unfound\n", filepath) - } - - if !strings.Contains(me.Error(), errorMsg) { - t.Errorf("Incorrect KustomizationError.Error() message \n") - t.Errorf("Expected errorMsg %s, but unfound\n", errorMsg) - } -} - -func TestKustomizationErrors_Error(t *testing.T) { - me := KustomizationError{KustomizationPath: filepath, ErrorMsg: "Kustomization not found"} - ce := ConfigmapError{Path: filepath, ErrorMsg: "can't find configmap name"} - pe := PatchError{KustomizationPath: filepath, PatchFilepath: filepath, ErrorMsg: "can't find patch file"} - re := ResourceError{KustomizationPath: filepath, ResourceFilepath: filepath, ErrorMsg: "can't find resource file"} - se := SecretError{KustomizationPath: filepath, ErrorMsg: "can't find secret name"} - mes := KustomizationErrors{kErrors: []error{me, ce, pe, re, se}} - expectedErrorMsg := fmt.Sprintf("%s\n%s\n%s\n%s\n%s\n", me.Error(), ce.Error(), pe.Error(), re.Error(), se.Error()) - if mes.Error() != expectedErrorMsg { - t.Errorf("Incorrect KustomizationErrors.Error() message\n") - t.Errorf(" Expected: %s\n", expectedErrorMsg) - t.Errorf(" Got: %s\n", mes.Error()) - } -} - -func TestKustomizationErrors_Get(t *testing.T) { - ce := ConfigmapError{Path: "kustomization/filepath", ErrorMsg: "can't find configmap name"} - mes := KustomizationErrors{kErrors: []error{ce}} - if len(mes.Get()) != 1 { - t.Errorf("Incorrect KustomizationErrors.Get()\n") - t.Errorf(" Expected: %v\n", []error{ce}) - t.Errorf(" Got: %s\n", mes.Get()) - } -} - -func TestKustomizationErrors_Append(t *testing.T) { - ce := ConfigmapError{Path: "kustomization/filepath", ErrorMsg: "can't find configmap name"} - pe := PatchError{KustomizationPath: "kustomization/filepath", PatchFilepath: "patch/path", ErrorMsg: "can't find patch file"} - mes := KustomizationErrors{kErrors: []error{ce}} - mes.Append(pe) - if len(mes.Get()) != 2 { - t.Errorf("Incorrect KustomizationErrors.Append()\n") - t.Errorf(" Expected: %d error\n%v/n", 2, []error{ce, pe}) - t.Errorf(" Got: %d error\n%v\n", len(mes.Get()), mes.Get()) - } -} - -func TestKustomizationErrors_BatchAppend(t *testing.T) { - ce := ConfigmapError{Path: "kustomization/filepath", ErrorMsg: "can't find configmap name"} - pe := PatchError{KustomizationPath: "kustomization/filepath", PatchFilepath: "patch/path", ErrorMsg: "can't find patch file"} - mes := KustomizationErrors{kErrors: []error{ce}} - me := KustomizationErrors{kErrors: []error{pe}} - mes.BatchAppend(me) - if len(mes.Get()) != 2 { - t.Errorf("Incorrect KustomizationErrors.Append()\n") - t.Errorf(" Expected: %d error\n%v/n", 2, []error{ce, pe}) - t.Errorf(" Got: %d error\n%v\n", len(mes.Get()), mes.Get()) - } -} diff --git a/pkg/resmap/factory.go b/pkg/resmap/factory.go index 7209d47b2..b178dbe53 100644 --- a/pkg/resmap/factory.go +++ b/pkg/resmap/factory.go @@ -41,22 +41,18 @@ func (rmF *Factory) RF() *resource.Factory { return rmF.resF } -// FromFiles returns a ResMap given a resource path slice. -func (rmF *Factory) FromFiles( - loader ifc.Loader, paths []string) (ResMap, error) { - var result []ResMap - for _, path := range paths { - content, err := loader.Load(path) - if err != nil { - return nil, errors.Wrap(err, "Load from path "+path+" failed") - } - res, err := rmF.NewResMapFromBytes(content) - if err != nil { - return nil, internal.Handler(err, path) - } - result = append(result, res) +// FromFile returns a ResMap given a resource path. +func (rmF *Factory) FromFile( + loader ifc.Loader, path string) (ResMap, error) { + content, err := loader.Load(path) + if err != nil { + return nil, err } - return MergeWithErrorOnIdCollision(result...) + res, err := rmF.NewResMapFromBytes(content) + if err != nil { + return nil, internal.Handler(err, path) + } + return res, nil } // newResMapFromBytes decodes a list of objects in byte array format. @@ -65,7 +61,6 @@ func (rmF *Factory) NewResMapFromBytes(b []byte) (ResMap, error) { if err != nil { return nil, err } - result := ResMap{} for _, res := range resources { id := res.Id() diff --git a/pkg/resmap/factory_test.go b/pkg/resmap/factory_test.go index 985179eed..30c9b6080 100644 --- a/pkg/resmap/factory_test.go +++ b/pkg/resmap/factory_test.go @@ -32,7 +32,7 @@ import ( "sigs.k8s.io/kustomize/pkg/types" ) -func TestFromFiles(t *testing.T) { +func TestFromFile(t *testing.T) { resourceStr := `apiVersion: apps/v1 kind: Deployment @@ -85,12 +85,10 @@ metadata: }), } - m, _ := rmF.FromFiles( - l, []string{"deployment.yaml"}) + m, _ := rmF.FromFile(l, "deployment.yaml") if len(m) != 3 { t.Fatalf("%#v should contain 3 appResource, but got %d", m, len(m)) } - if err := expected.ErrorIfNotEqual(m); err != nil { t.Fatalf("actual doesn't match expected: %v", err) } diff --git a/pkg/target/kusttarget.go b/pkg/target/kusttarget.go index a0641b550..f85ae7d9a 100644 --- a/pkg/target/kusttarget.go +++ b/pkg/target/kusttarget.go @@ -28,7 +28,6 @@ import ( "sigs.k8s.io/kustomize/pkg/accumulator" "sigs.k8s.io/kustomize/pkg/ifc" "sigs.k8s.io/kustomize/pkg/ifc/transformer" - interror "sigs.k8s.io/kustomize/pkg/internal/error" patchtransformer "sigs.k8s.io/kustomize/pkg/patch/transformer" "sigs.k8s.io/kustomize/pkg/pgmconfig" "sigs.k8s.io/kustomize/pkg/plugins" @@ -195,23 +194,16 @@ func (kt *KustTarget) shouldAddHashSuffixesToGeneratedResources() bool { // holding customized resources and the data/rules used // to do so. The name back references and vars are // not yet fixed. -func (kt *KustTarget) AccumulateTarget() ( // nolint: gocyclo +func (kt *KustTarget) AccumulateTarget() ( ra *accumulator.ResAccumulator, err error) { - // TODO(monopole): Get rid of the KustomizationErrors accumulator. - // It's not consistently used, and complicates tests. - errs := &interror.KustomizationErrors{} - ra, errs = kt.accumulateBases() - resources, err := kt.rFactory.FromFiles( - kt.ldr, kt.kustomization.Resources) + ra = accumulator.MakeEmptyAccumulator() + err = kt.accumulateResources(ra, kt.kustomization.Bases) if err != nil { - errs.Append(errors.Wrap(err, "rawResources failed to read Resources")) + return nil, errors.Wrap(err, "accumulating bases") } - if len(errs.Get()) > 0 { - return ra, errs - } - err = ra.MergeResourcesWithErrorOnIdCollision(resources) + err = kt.accumulateResources(ra, kt.kustomization.Resources) if err != nil { - errs.Append(errors.Wrap(err, "MergeResourcesWithErrorOnIdCollision")) + return nil, errors.Wrap(err, "accumulating resources") } tConfig, err := config.MakeTransformerConfig( kt.ldr, kt.kustomization.Configurations) @@ -220,41 +212,46 @@ func (kt *KustTarget) AccumulateTarget() ( // nolint: gocyclo } err = ra.MergeConfig(tConfig) if err != nil { - errs.Append(errors.Wrap(err, "MergeConfig")) + return nil, errors.Wrapf( + err, "merging config %v", tConfig) } err = ra.MergeVars(kt.kustomization.Vars) if err != nil { - errs.Append(errors.Wrap(err, "MergeVars")) + return nil, errors.Wrapf( + err, "merging vars %v", kt.kustomization.Vars) } crdTc, err := config.LoadConfigFromCRDs(kt.ldr, kt.kustomization.Crds) if err != nil { - errs.Append(errors.Wrap(err, "LoadCRDs")) + return nil, errors.Wrapf( + err, "loading CRDs %v", kt.kustomization.Crds) } err = ra.MergeConfig(crdTc) if err != nil { - errs.Append(errors.Wrap(err, "merge CRDs")) + return nil, errors.Wrapf( + err, "merging CRDs %v", crdTc) } - resMap, err := kt.generateConfigMapsAndSecrets(errs) + resMap, err := kt.generateConfigMapsAndSecrets() if err != nil { - errs.Append(errors.Wrap(err, "generateConfigMapsAndSecrets")) + return nil, errors.Wrap( + err, "generating legacy configMaps and secrets") } err = ra.MergeResourcesWithOverride(resMap) if err != nil { - return nil, err + return nil, errors.Wrap( + err, "merging legacy configMaps and secrets") } if kt.pluginConfig.GoEnabled { - kt.generateFromPlugins(ra, errs) - if len(errs.Get()) > 0 { - return ra, errs + err := kt.generateFromPlugins(ra) + if err != nil { + return nil, err } } patches, err := kt.rFactory.RF().SliceFromPatches( kt.ldr, kt.kustomization.PatchesStrategicMerge) if err != nil { - errs.Append(errors.Wrap(err, "SliceFromPatches")) - } - if len(errs.Get()) > 0 { - return nil, errs + return nil, errors.Wrapf( + err, "reading strategic merge patches %v", + kt.kustomization.PatchesStrategicMerge) } t, err := kt.newTransformer(patches, ra.GetTransformerConfig()) if err != nil { @@ -268,80 +265,97 @@ func (kt *KustTarget) AccumulateTarget() ( // nolint: gocyclo } func (kt *KustTarget) generateFromPlugins( - ra *accumulator.ResAccumulator, - errs *interror.KustomizationErrors) { + ra *accumulator.ResAccumulator) error { generators, err := kt.loadGeneratorPlugins() if err != nil { - errs.Append(err) - return + return errors.Wrap(err, "loading generator plugins") } for _, g := range generators { resMap, err := g.Generate() if err != nil { - errs.Append(err) - } else { - err = ra.MergeResourcesWithErrorOnIdCollision(resMap) - if err != nil { - errs.Append(errors.Wrap(err, "from plugin")) - } + return errors.Wrapf(err, "generating from %v", g) + } + err = ra.MergeResourcesWithErrorOnIdCollision(resMap) + if err != nil { + return errors.Wrapf(err, "merging from generator %v", g) } } + return nil } -func (kt *KustTarget) generateConfigMapsAndSecrets( - errs *interror.KustomizationErrors) (resmap.ResMap, error) { +func (kt *KustTarget) generateConfigMapsAndSecrets() (resmap.ResMap, error) { cms, err := kt.rFactory.NewResMapFromConfigMapArgs( kt.ldr, kt.kustomization.GeneratorOptions, kt.kustomization.ConfigMapGenerator) if err != nil { - errs.Append(errors.Wrap(err, "NewResMapFromConfigMapArgs")) + return nil, errors.Wrapf( + err, "configmapgenerator: %v", kt.kustomization.ConfigMapGenerator) } secrets, err := kt.rFactory.NewResMapFromSecretArgs( kt.ldr, kt.kustomization.GeneratorOptions, kt.kustomization.SecretGenerator) if err != nil { - errs.Append(errors.Wrap(err, "NewResMapFromSecretArgs")) + return nil, errors.Wrapf( + err, "secretgenerator: %v", kt.kustomization.SecretGenerator) } return resmap.MergeWithErrorOnIdCollision(cms, secrets) } -// accumulateBases returns a new ResAccumulator -// holding customized resources and the data/rules -// used to customized them from only the _bases_ -// of this KustTarget. -func (kt *KustTarget) accumulateBases() ( - ra *accumulator.ResAccumulator, errs *interror.KustomizationErrors) { - errs = &interror.KustomizationErrors{} - ra = accumulator.MakeEmptyAccumulator() - - for _, path := range kt.kustomization.Bases { +// accumulateResources fills the given resourceAccumulator +// with resources read from the given list of paths. +func (kt *KustTarget) accumulateResources( + ra *accumulator.ResAccumulator, paths []string) error { + for _, path := range paths { ldr, err := kt.ldr.New(path) - if err != nil { - errs.Append(errors.Wrap(err, "couldn't make loader for "+path)) - continue + if err == nil { + err = kt.accumulateDirectory(ra, ldr, path) + if err != nil { + return err + } + } else { + err = kt.accumulateFile(ra, path) + if err != nil { + return err + } } - subKt, err := NewKustTarget( - ldr, kt.rFactory, kt.tFactory, kt.pluginConfig) - if err != nil { - errs.Append(errors.Wrap(err, "couldn't make target for "+path)) - ldr.Cleanup() - continue - } - subRa, err := subKt.AccumulateTarget() - if err != nil { - errs.Append(errors.Wrap(err, "AccumulateTarget")) - ldr.Cleanup() - continue - } - err = ra.MergeAccumulator(subRa) - if err != nil { - errs.Append(errors.Wrap(err, path)) - } - ldr.Cleanup() } - return ra, errs + return nil +} + +func (kt *KustTarget) accumulateDirectory( + ra *accumulator.ResAccumulator, ldr ifc.Loader, path string) error { + defer ldr.Cleanup() + subKt, err := NewKustTarget( + ldr, kt.rFactory, kt.tFactory, kt.pluginConfig) + if err != nil { + return errors.Wrapf(err, "couldn't make target for path '%s'", path) + } + subRa, err := subKt.AccumulateTarget() + if err != nil { + return errors.Wrapf( + err, "recursed accumulation of path '%s'", path) + } + err = ra.MergeAccumulator(subRa) + if err != nil { + return errors.Wrapf( + err, "recursed merging from path '%s'", path) + } + return nil +} + +func (kt *KustTarget) accumulateFile( + ra *accumulator.ResAccumulator, path string) error { + resources, err := kt.rFactory.FromFile(kt.ldr, path) + if err != nil { + return errors.Wrapf(err, "accumulating resources from '%s'", path) + } + err = ra.MergeResourcesWithErrorOnIdCollision(resources) + if err != nil { + return errors.Wrapf(err, "merging resources from '%s'", path) + } + return nil } // newTransformer makes a Transformer that does a collection @@ -401,21 +415,21 @@ func (kt *KustTarget) newTransformer( } func (kt *KustTarget) loadTransformerPlugins() ([]transformers.Transformer, error) { - configs, err := kt.rFactory.FromFiles( - kt.ldr, kt.kustomization.Transformers) + ra := accumulator.MakeEmptyAccumulator() + err := kt.accumulateResources(ra, kt.kustomization.Transformers) if err != nil { return nil, err } return plugins.NewTransformerLoader( - kt.pluginConfig, kt.ldr, kt.rFactory).Load(configs) + kt.pluginConfig, kt.ldr, kt.rFactory).Load(ra.ResMap()) } func (kt *KustTarget) loadGeneratorPlugins() ([]transformers.Generator, error) { - configs, err := kt.rFactory.FromFiles( - kt.ldr, kt.kustomization.Generators) + ra := accumulator.MakeEmptyAccumulator() + err := kt.accumulateResources(ra, kt.kustomization.Generators) if err != nil { return nil, err } return plugins.NewGeneratorLoader( - kt.pluginConfig, kt.ldr, kt.rFactory).Load(configs) + kt.pluginConfig, kt.ldr, kt.rFactory).Load(ra.ResMap()) }