Fix AppendingErrHandler issue causing errors not to be stored in pass…

…ed down handlers (#516)

* use a pointer to allow referencing the original AppendingErrHandler when appending errors

* add changelog

* Add AppendingErrorHandler unit tests

* Add simple pull request template

* Update contrib/pkg/output/errhandlers/err_handlers.go

Co-authored-by: Marco Schmidt <[email protected]>

* ExpectWithOffset err_handlers_test

* Update error handler list message

* noop

---------

Co-authored-by: Marco Schmidt <[email protected]>

.github/pull_request_template.md

@@ -0,0 +1,11 @@
+# Description
+
+Please include a thorough summary of the changes.
+
+This fixes ... \ This new feature can be used to ...
+
+# Context
+
+Summary of issue
+
+Description and justification of any interesting decisions made

changelog/v0.35.0/fix-AppendErrHandler-not-appending-errors-to-original-object.yaml

@@ -0,0 +1,7 @@
+changelog:
+  - type: FIX
+    issueLink: https://github.com/solo-io/skv2/issues/517
+    description: Fix issue where the AppendErrHandler was not appending errors to the original object.
+  - type: BREAKING_CHANGE
+    issueLink: https://github.com/solo-io/skv2/issues/517
+    description: Usages of AppendErrHandler will now need to pass it by reference (&).

contrib/pkg/output/errhandlers/err_handlers.go

@@ -9,23 +9,33 @@ import (
 
 // this file contains ErrorHandlers for handling errors created when writing output snapshots
 
+func ResourceWriteError(resource ezkube.Object, err error) error {
+	return eris.Wrapf(err, "writing resource %v failed", sets.Key(resource))
+}
+func ResourceDeleteError(resource ezkube.Object, err error) error {
+	return eris.Wrapf(err, "deleting resource %v failed", sets.Key(resource))
+}
+func ListError(err error) error {
+	return eris.Wrapf(err, "listing resources failed")
+}
+
 type AppendingErrHandler struct {
 	errs error
 }
 
-func (a AppendingErrHandler) HandleWriteError(resource ezkube.Object, err error) {
-	a.errs = multierror.Append(a.errs, eris.Wrapf(err, "writing resource %v failed", sets.Key(resource)))
+func (a *AppendingErrHandler) HandleWriteError(resource ezkube.Object, err error) {
+	a.errs = multierror.Append(a.errs, ResourceWriteError(resource, err))
 }
 
-func (a AppendingErrHandler) HandleDeleteError(resource ezkube.Object, err error) {
-	a.errs = multierror.Append(a.errs, eris.Wrapf(err, "deleting resource %v failed", sets.Key(resource)))
+func (a *AppendingErrHandler) HandleDeleteError(resource ezkube.Object, err error) {
+	a.errs = multierror.Append(a.errs, ResourceDeleteError(resource, err))
 }
 
-func (a AppendingErrHandler) HandleListError(err error) {
-	a.errs = multierror.Append(a.errs, eris.Wrapf(err, "listing failed"))
+func (a *AppendingErrHandler) HandleListError(err error) {
+	a.errs = multierror.Append(a.errs, ListError(err))
 }
 
-// returns the errors collected by the handler
-func (a AppendingErrHandler) Errors() error {
+// Errors returns the errors collected by the handler
+func (a *AppendingErrHandler) Errors() error {
 	return a.errs
 }

contrib/pkg/output/errhandlers/err_handlers_suite_test.go

@@ -0,0 +1,13 @@
+package errhandlers_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestCodegen(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Error Handlers Suite")
+}

contrib/pkg/output/errhandlers/err_handlers_test.go

@@ -0,0 +1,94 @@
+package errhandlers
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
+	v1 "github.com/solo-io/skv2/codegen/test/api/things.test.io/v1"
+	"github.com/solo-io/skv2/contrib/pkg/output"
+	"github.com/solo-io/skv2/pkg/ezkube"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+var _ = Describe("Error Handlers", func() {
+	var (
+		errorHandler output.ErrorHandler
+		fakeResource = &v1.Paint{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "fake-name",
+				Namespace: "fake-ns",
+			},
+		}
+	)
+
+	Context("AppendingErrHandler", func() {
+		BeforeEach(func() {
+			errorHandler = &AppendingErrHandler{}
+		})
+
+		DescribeTable("should append errors to the error handler object", func(mutation errorMutation, expects errorExpectation) {
+			mutation(errorHandler, fakeResource)
+			expects(errorHandler.(*AppendingErrHandler).Errors(), fakeResource)
+		},
+			Entry("when HandleListError is called", addListError, expectListError),
+			Entry("when HandleWriteError is called", addWriteError, expectWriteError),
+			Entry("when HandleDeleteError is called", addDeleteError, expectDeleteError),
+			Entry("when multiple error handler functions are invoked", func(handler output.ErrorHandler, resource ezkube.Object) {
+				addListError(handler, resource)
+				addWriteError(handler, resource)
+				addDeleteError(handler, resource)
+			}, func(err error, resource ezkube.Object) {
+				Expect(err).To(And(
+					MatchError(ListError(err)),
+					MatchError(ResourceWriteError(resource, err)),
+					MatchError(ResourceDeleteError(resource, err)),
+				))
+			}),
+		)
+
+		It("should not have an error when no errors occur", func() {
+			errs := errorHandler.(*AppendingErrHandler).Errors()
+			Expect(errs).NotTo(HaveOccurred())
+		})
+	})
+})
+
+type errorMutation func(handler output.ErrorHandler, resource ezkube.Object)
+
+func addListError(handler output.ErrorHandler, _ ezkube.Object) {
+	handler.HandleListError(errors.New("listing resources failed"))
+}
+func addWriteError(handler output.ErrorHandler, resource ezkube.Object) {
+	handler.HandleWriteError(resource, errors.New("writing resource failed"))
+}
+func addDeleteError(handler output.ErrorHandler, resource ezkube.Object) {
+	handler.HandleDeleteError(resource, errors.New("deleting resource failed"))
+}
+
+// errorExpectation is a function that expects a specific error to occur
+type errorExpectation func(err error, resource ezkube.Object)
+
+func expectListError(err error, resource ezkube.Object) {
+	ExpectWithOffset(1, err).To(HaveOccurred())
+	ExpectWithOffset(1, err).To(And(
+		MatchError(ListError(err)),
+		Not(MatchError(ResourceWriteError(resource, err))),
+		Not(MatchError(ResourceDeleteError(resource, err))),
+	))
+}
+func expectWriteError(err error, resource ezkube.Object) {
+	ExpectWithOffset(1, err).To(HaveOccurred())
+	ExpectWithOffset(1, err).To(And(
+		MatchError(ResourceWriteError(resource, err)),
+		Not(MatchError(ListError(err))),
+		Not(MatchError(ResourceDeleteError(resource, err))),
+	))
+}
+func expectDeleteError(err error, resource ezkube.Object) {
+	ExpectWithOffset(1, err).To(HaveOccurred())
+	ExpectWithOffset(1, err).To(And(
+		MatchError(ResourceDeleteError(resource, err)),
+		Not(MatchError(ListError(err))),
+		Not(MatchError(ResourceWriteError(resource, err))),
+	))
+}