From 91bd8266d3d188b0aa6ab7575697937beb59e4d4 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 10 May 2023 16:57:54 +0200 Subject: [PATCH 1/6] Added ability for deferred mutator execution --- bundle/deferred.go | 34 ++++++++++++ bundle/deferred_test.go | 108 +++++++++++++++++++++++++++++++++++++++ bundle/phases/deploy.go | 25 +++++---- bundle/phases/destroy.go | 21 ++++---- 4 files changed, 168 insertions(+), 20 deletions(-) create mode 100644 bundle/deferred.go create mode 100644 bundle/deferred_test.go diff --git a/bundle/deferred.go b/bundle/deferred.go new file mode 100644 index 0000000000..b41afc7a9e --- /dev/null +++ b/bundle/deferred.go @@ -0,0 +1,34 @@ +package bundle + +import ( + "context" + "errors" +) + +type DeferredMutator struct { + mutators []Mutator + onFinishOrError []Mutator +} + +func (d *DeferredMutator) Name() string { + return "deferred" +} + +func Defer(mutators []Mutator, onFinishOrError []Mutator) []Mutator { + return []Mutator{ + &DeferredMutator{ + mutators: mutators, + onFinishOrError: onFinishOrError, + }, + } +} + +func (d *DeferredMutator) Apply(ctx context.Context, b *Bundle) ([]Mutator, error) { + err := Apply(ctx, b, d.mutators) + errOnFinish := Apply(ctx, b, d.onFinishOrError) + + if err != nil || errOnFinish != nil { + return nil, errors.Join(err, errOnFinish) + } + return nil, nil +} diff --git a/bundle/deferred_test.go b/bundle/deferred_test.go new file mode 100644 index 0000000000..336b8aaf86 --- /dev/null +++ b/bundle/deferred_test.go @@ -0,0 +1,108 @@ +package bundle + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +type mutatorWithError struct { + applyCalled int + errorMsg string +} + +func (t *mutatorWithError) Name() string { + return "mutatorWithError" +} + +func (t *mutatorWithError) Apply(_ context.Context, b *Bundle) ([]Mutator, error) { + t.applyCalled++ + return nil, fmt.Errorf(t.errorMsg) +} + +func TestDeferredMutatorWhenAllMutatorsSucceed(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + m3 := &testMutator{} + cleanup := &testMutator{} + deferred_mutator := Defer([]Mutator{m1, m2, m3}, []Mutator{cleanup}) + + bundle := &Bundle{} + err := Apply(context.Background(), bundle, deferred_mutator) + assert.NoError(t, err) + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, m2.applyCalled) + assert.Equal(t, 1, m3.applyCalled) + assert.Equal(t, 1, cleanup.applyCalled) +} + +func TestDeferredMutatorWhenFirstFails(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + mErr := &mutatorWithError{errorMsg: "mutator error occurred"} + cleanup := &testMutator{} + deferred_mutator := Defer([]Mutator{mErr, m1, m2}, []Mutator{cleanup}) + + bundle := &Bundle{} + err := Apply(context.Background(), bundle, deferred_mutator) + assert.ErrorContains(t, err, "mutator error occurred") + + assert.Equal(t, 1, mErr.applyCalled) + assert.Equal(t, 0, m1.applyCalled) + assert.Equal(t, 0, m2.applyCalled) + assert.Equal(t, 1, cleanup.applyCalled) +} + +func TestDeferredMutatorWhenMiddleOneFails(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + mErr := &mutatorWithError{errorMsg: "mutator error occurred"} + cleanup := &testMutator{} + deferred_mutator := Defer([]Mutator{m1, mErr, m2}, []Mutator{cleanup}) + + bundle := &Bundle{} + err := Apply(context.Background(), bundle, deferred_mutator) + assert.ErrorContains(t, err, "mutator error occurred") + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, mErr.applyCalled) + assert.Equal(t, 0, m2.applyCalled) + assert.Equal(t, 1, cleanup.applyCalled) +} + +func TestDeferredMutatorWhenLastOneFails(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + mErr := &mutatorWithError{errorMsg: "mutator error occurred"} + cleanup := &testMutator{} + deferred_mutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanup}) + + bundle := &Bundle{} + err := Apply(context.Background(), bundle, deferred_mutator) + assert.ErrorContains(t, err, "mutator error occurred") + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, m2.applyCalled) + assert.Equal(t, 1, mErr.applyCalled) + assert.Equal(t, 1, cleanup.applyCalled) +} + +func TestDeferredMutatorCombinesErrorMessages(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + mErr := &mutatorWithError{errorMsg: "mutator error occurred"} + cleanupErr := &mutatorWithError{errorMsg: "cleanup error occurred"} + deferred_mutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanupErr}) + + bundle := &Bundle{} + err := Apply(context.Background(), bundle, deferred_mutator) + assert.ErrorContains(t, err, "mutator error occurred\ncleanup error occurred") + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, m2.applyCalled) + assert.Equal(t, 1, mErr.applyCalled) + assert.Equal(t, 1, cleanupErr.applyCalled) +} diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 2b656c8d4d..85e9aeb0e9 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -10,18 +10,21 @@ import ( // The deploy phase deploys artifacts and resources. func Deploy() bundle.Mutator { + deployPhase := bundle.Defer([]bundle.Mutator{ + lock.Acquire(), + files.Upload(), + artifacts.UploadAll(), + terraform.Interpolate(), + terraform.Write(), + terraform.StatePull(), + terraform.Apply(), + terraform.StatePush(), + }, []bundle.Mutator{ + lock.Release(), + }) + return newPhase( "deploy", - []bundle.Mutator{ - lock.Acquire(), - files.Upload(), - artifacts.UploadAll(), - terraform.Interpolate(), - terraform.Write(), - terraform.StatePull(), - terraform.Apply(), - terraform.StatePush(), - lock.Release(), - }, + deployPhase, ) } diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index baec5c4fcb..4df5ce3201 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -9,16 +9,19 @@ import ( // The destroy phase deletes artifacts and resources. func Destroy() bundle.Mutator { + destroyPhase := bundle.Defer([]bundle.Mutator{ + lock.Acquire(), + terraform.StatePull(), + terraform.Plan(terraform.PlanGoal("destroy")), + terraform.Destroy(), + terraform.StatePush(), + }, []bundle.Mutator{ + lock.Release(), + files.Delete(), + }) + return newPhase( "destroy", - []bundle.Mutator{ - lock.Acquire(), - terraform.StatePull(), - terraform.Plan(terraform.PlanGoal("destroy")), - terraform.Destroy(), - terraform.StatePush(), - lock.Release(), - files.Delete(), - }, + destroyPhase, ) } From b1c03924d4cb847cf16cd7122b9e5187b439b6ca Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 10 May 2023 17:24:48 +0200 Subject: [PATCH 2/6] Do not use errors.Join --- bundle/deferred.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/bundle/deferred.go b/bundle/deferred.go index b41afc7a9e..54f4b24853 100644 --- a/bundle/deferred.go +++ b/bundle/deferred.go @@ -2,7 +2,7 @@ package bundle import ( "context" - "errors" + "fmt" ) type DeferredMutator struct { @@ -24,11 +24,19 @@ func Defer(mutators []Mutator, onFinishOrError []Mutator) []Mutator { } func (d *DeferredMutator) Apply(ctx context.Context, b *Bundle) ([]Mutator, error) { - err := Apply(ctx, b, d.mutators) + mainErr := Apply(ctx, b, d.mutators) errOnFinish := Apply(ctx, b, d.onFinishOrError) + var err error = nil - if err != nil || errOnFinish != nil { - return nil, errors.Join(err, errOnFinish) + if mainErr != nil { + err = mainErr } - return nil, nil + if errOnFinish != nil { + if err == nil { + err = errOnFinish + } else { + err = fmt.Errorf("%w\n%w", err, errOnFinish) + } + } + return nil, err } From cc40e5637338f1153f344a208e71e644bfb5e3fc Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 10 May 2023 17:39:56 +0200 Subject: [PATCH 3/6] Correctly format errors --- bundle/deferred.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/bundle/deferred.go b/bundle/deferred.go index 54f4b24853..495c2512e1 100644 --- a/bundle/deferred.go +++ b/bundle/deferred.go @@ -26,17 +26,16 @@ func Defer(mutators []Mutator, onFinishOrError []Mutator) []Mutator { func (d *DeferredMutator) Apply(ctx context.Context, b *Bundle) ([]Mutator, error) { mainErr := Apply(ctx, b, d.mutators) errOnFinish := Apply(ctx, b, d.onFinishOrError) - var err error = nil + if mainErr == nil && errOnFinish == nil { + return nil, nil + } + err := fmt.Errorf("Error") if mainErr != nil { - err = mainErr + err = fmt.Errorf("%w\n%v", err, mainErr) } if errOnFinish != nil { - if err == nil { - err = errOnFinish - } else { - err = fmt.Errorf("%w\n%w", err, errOnFinish) - } + err = fmt.Errorf("%w\n%v", err, errOnFinish) } return nil, err } From ed2683be2d157960f5ff73cdbdc89beb9ba45168 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Fri, 12 May 2023 13:57:46 +0200 Subject: [PATCH 4/6] Fixed error wrapping --- bundle/deferred.go | 26 ++++++++++-------------- bundle/deferred_test.go | 20 +++++++++---------- bundle/phases/destroy.go | 2 +- libs/errors/errors.go | 41 ++++++++++++++++++++++++++++++++++++++ libs/errors/errors_test.go | 37 ++++++++++++++++++++++++++++++++++ 5 files changed, 100 insertions(+), 26 deletions(-) create mode 100644 libs/errors/errors.go create mode 100644 libs/errors/errors_test.go diff --git a/bundle/deferred.go b/bundle/deferred.go index 495c2512e1..967e8bd06d 100644 --- a/bundle/deferred.go +++ b/bundle/deferred.go @@ -2,40 +2,36 @@ package bundle import ( "context" - "fmt" + + "github.com/databricks/bricks/libs/errors" ) + + type DeferredMutator struct { mutators []Mutator - onFinishOrError []Mutator + finally []Mutator } func (d *DeferredMutator) Name() string { return "deferred" } -func Defer(mutators []Mutator, onFinishOrError []Mutator) []Mutator { +func Defer(mutators []Mutator, finally []Mutator) []Mutator { return []Mutator{ &DeferredMutator{ mutators: mutators, - onFinishOrError: onFinishOrError, + finally: finally, }, } } func (d *DeferredMutator) Apply(ctx context.Context, b *Bundle) ([]Mutator, error) { mainErr := Apply(ctx, b, d.mutators) - errOnFinish := Apply(ctx, b, d.onFinishOrError) - if mainErr == nil && errOnFinish == nil { - return nil, nil + errOnFinish := Apply(ctx, b, d.finally) + if mainErr != nil || errOnFinish != nil { + return nil, errors.FromMany(mainErr, errOnFinish) } - err := fmt.Errorf("Error") - if mainErr != nil { - err = fmt.Errorf("%w\n%v", err, mainErr) - } - if errOnFinish != nil { - err = fmt.Errorf("%w\n%v", err, errOnFinish) - } - return nil, err + return nil, nil } diff --git a/bundle/deferred_test.go b/bundle/deferred_test.go index 336b8aaf86..9d7f5d4026 100644 --- a/bundle/deferred_test.go +++ b/bundle/deferred_test.go @@ -27,10 +27,10 @@ func TestDeferredMutatorWhenAllMutatorsSucceed(t *testing.T) { m2 := &testMutator{} m3 := &testMutator{} cleanup := &testMutator{} - deferred_mutator := Defer([]Mutator{m1, m2, m3}, []Mutator{cleanup}) + deferredMutator := Defer([]Mutator{m1, m2, m3}, []Mutator{cleanup}) bundle := &Bundle{} - err := Apply(context.Background(), bundle, deferred_mutator) + err := Apply(context.Background(), bundle, deferredMutator) assert.NoError(t, err) assert.Equal(t, 1, m1.applyCalled) @@ -44,10 +44,10 @@ func TestDeferredMutatorWhenFirstFails(t *testing.T) { m2 := &testMutator{} mErr := &mutatorWithError{errorMsg: "mutator error occurred"} cleanup := &testMutator{} - deferred_mutator := Defer([]Mutator{mErr, m1, m2}, []Mutator{cleanup}) + deferredMutator := Defer([]Mutator{mErr, m1, m2}, []Mutator{cleanup}) bundle := &Bundle{} - err := Apply(context.Background(), bundle, deferred_mutator) + err := Apply(context.Background(), bundle, deferredMutator) assert.ErrorContains(t, err, "mutator error occurred") assert.Equal(t, 1, mErr.applyCalled) @@ -61,10 +61,10 @@ func TestDeferredMutatorWhenMiddleOneFails(t *testing.T) { m2 := &testMutator{} mErr := &mutatorWithError{errorMsg: "mutator error occurred"} cleanup := &testMutator{} - deferred_mutator := Defer([]Mutator{m1, mErr, m2}, []Mutator{cleanup}) + deferredMutator := Defer([]Mutator{m1, mErr, m2}, []Mutator{cleanup}) bundle := &Bundle{} - err := Apply(context.Background(), bundle, deferred_mutator) + err := Apply(context.Background(), bundle, deferredMutator) assert.ErrorContains(t, err, "mutator error occurred") assert.Equal(t, 1, m1.applyCalled) @@ -78,10 +78,10 @@ func TestDeferredMutatorWhenLastOneFails(t *testing.T) { m2 := &testMutator{} mErr := &mutatorWithError{errorMsg: "mutator error occurred"} cleanup := &testMutator{} - deferred_mutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanup}) + deferredMutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanup}) bundle := &Bundle{} - err := Apply(context.Background(), bundle, deferred_mutator) + err := Apply(context.Background(), bundle, deferredMutator) assert.ErrorContains(t, err, "mutator error occurred") assert.Equal(t, 1, m1.applyCalled) @@ -95,10 +95,10 @@ func TestDeferredMutatorCombinesErrorMessages(t *testing.T) { m2 := &testMutator{} mErr := &mutatorWithError{errorMsg: "mutator error occurred"} cleanupErr := &mutatorWithError{errorMsg: "cleanup error occurred"} - deferred_mutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanupErr}) + deferredMutator := Defer([]Mutator{m1, m2, mErr}, []Mutator{cleanupErr}) bundle := &Bundle{} - err := Apply(context.Background(), bundle, deferred_mutator) + err := Apply(context.Background(), bundle, deferredMutator) assert.ErrorContains(t, err, "mutator error occurred\ncleanup error occurred") assert.Equal(t, 1, m1.applyCalled) diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index 4df5ce3201..f069abbc9d 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -15,9 +15,9 @@ func Destroy() bundle.Mutator { terraform.Plan(terraform.PlanGoal("destroy")), terraform.Destroy(), terraform.StatePush(), + files.Delete(), }, []bundle.Mutator{ lock.Release(), - files.Delete(), }) return newPhase( diff --git a/libs/errors/errors.go b/libs/errors/errors.go new file mode 100644 index 0000000000..6ea056c669 --- /dev/null +++ b/libs/errors/errors.go @@ -0,0 +1,41 @@ +package errors + +type combinedError struct { + errors []error +} + +func FromMany(errors ...error) error { + n := 0 + for _, err := range errors { + if err != nil { + n++ + } + } + if n == 0 { + return nil + } + combinedErr := &combinedError{ + errors: make([]error, 0, n), + } + for _, err := range errors { + if err != nil { + combinedErr.errors = append(combinedErr.errors, err) + } + } + return combinedErr +} + +func (ce *combinedError) Error() string { + var b []byte + for i, err := range ce.errors { + if i > 0 { + b = append(b, '\n') + } + b = append(b, err.Error()...) + } + return string(b) +} + +func (ce *combinedError) Unwrap() []error { + return ce.errors +} diff --git a/libs/errors/errors_test.go b/libs/errors/errors_test.go new file mode 100644 index 0000000000..fde34b510e --- /dev/null +++ b/libs/errors/errors_test.go @@ -0,0 +1,37 @@ +package errors + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFromManyErrors(t *testing.T) { + e1 := fmt.Errorf("Error 1") + e2 := fmt.Errorf("Error 2") + e3 := fmt.Errorf("Error 3") + err := FromMany(e1, e2, e3) + + assert.True(t, errors.Is(err, e1)) + assert.True(t, errors.Is(err, e2)) + assert.True(t, errors.Is(err, e3)) + + assert.Equal(t, err.Error(), `Error 1 +Error 2 +Error 3`) +} + +func TestFromManyErrorsWihtNil(t *testing.T) { + e1 := fmt.Errorf("Error 1") + var e2 error = nil + e3 := fmt.Errorf("Error 3") + err := FromMany(e1, e2, e3) + + assert.True(t, errors.Is(err, e1)) + assert.True(t, errors.Is(err, e3)) + + assert.Equal(t, err.Error(), `Error 1 +Error 3`) +} From b845736e2005ab280cfdbdcb0af9d9d7b73f8fdd Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 15 May 2023 10:30:02 +0200 Subject: [PATCH 5/6] fixed formatting --- bundle/deferred.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/bundle/deferred.go b/bundle/deferred.go index 967e8bd06d..475f07dc2d 100644 --- a/bundle/deferred.go +++ b/bundle/deferred.go @@ -6,11 +6,9 @@ import ( "github.com/databricks/bricks/libs/errors" ) - - type DeferredMutator struct { - mutators []Mutator - finally []Mutator + mutators []Mutator + finally []Mutator } func (d *DeferredMutator) Name() string { @@ -20,8 +18,8 @@ func (d *DeferredMutator) Name() string { func Defer(mutators []Mutator, finally []Mutator) []Mutator { return []Mutator{ &DeferredMutator{ - mutators: mutators, - finally: finally, + mutators: mutators, + finally: finally, }, } } From 56514bcde7bf23df9f2d8ec5bdc69e54a6a92071 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Mon, 15 May 2023 11:50:57 +0200 Subject: [PATCH 6/6] Refactored code for aggregate error handling --- bundle/deferred.go | 4 +- libs/errors/errors.go | 41 ----------- libs/errs/aggregate.go | 68 +++++++++++++++++++ .../errors_test.go => errs/aggregate_test.go} | 2 +- 4 files changed, 71 insertions(+), 44 deletions(-) delete mode 100644 libs/errors/errors.go create mode 100644 libs/errs/aggregate.go rename libs/{errors/errors_test.go => errs/aggregate_test.go} (97%) diff --git a/bundle/deferred.go b/bundle/deferred.go index 475f07dc2d..e48aa3a0eb 100644 --- a/bundle/deferred.go +++ b/bundle/deferred.go @@ -3,7 +3,7 @@ package bundle import ( "context" - "github.com/databricks/bricks/libs/errors" + "github.com/databricks/bricks/libs/errs" ) type DeferredMutator struct { @@ -28,7 +28,7 @@ func (d *DeferredMutator) Apply(ctx context.Context, b *Bundle) ([]Mutator, erro mainErr := Apply(ctx, b, d.mutators) errOnFinish := Apply(ctx, b, d.finally) if mainErr != nil || errOnFinish != nil { - return nil, errors.FromMany(mainErr, errOnFinish) + return nil, errs.FromMany(mainErr, errOnFinish) } return nil, nil diff --git a/libs/errors/errors.go b/libs/errors/errors.go deleted file mode 100644 index 6ea056c669..0000000000 --- a/libs/errors/errors.go +++ /dev/null @@ -1,41 +0,0 @@ -package errors - -type combinedError struct { - errors []error -} - -func FromMany(errors ...error) error { - n := 0 - for _, err := range errors { - if err != nil { - n++ - } - } - if n == 0 { - return nil - } - combinedErr := &combinedError{ - errors: make([]error, 0, n), - } - for _, err := range errors { - if err != nil { - combinedErr.errors = append(combinedErr.errors, err) - } - } - return combinedErr -} - -func (ce *combinedError) Error() string { - var b []byte - for i, err := range ce.errors { - if i > 0 { - b = append(b, '\n') - } - b = append(b, err.Error()...) - } - return string(b) -} - -func (ce *combinedError) Unwrap() []error { - return ce.errors -} diff --git a/libs/errs/aggregate.go b/libs/errs/aggregate.go new file mode 100644 index 0000000000..b6bab0ef77 --- /dev/null +++ b/libs/errs/aggregate.go @@ -0,0 +1,68 @@ +package errs + +import "errors" + +type aggregateError struct { + errors []error +} + +func FromMany(errors ...error) error { + n := 0 + for _, err := range errors { + if err != nil { + n++ + } + } + if n == 0 { + return nil + } + aggregateErr := &aggregateError{ + errors: make([]error, 0, n), + } + for _, err := range errors { + if err != nil { + aggregateErr.errors = append(aggregateErr.errors, err) + } + } + return aggregateErr +} + +func (ce *aggregateError) Error() string { + var b []byte + for i, err := range ce.errors { + if i > 0 { + b = append(b, '\n') + } + b = append(b, err.Error()...) + } + return string(b) +} + +func (ce *aggregateError) Unwrap() error { + return errorChain(ce.errors) +} + +// Represents chained list of errors. +// Implements Error interface so that chain of errors +// can correctly work with errors.Is/As method +type errorChain []error + +func (ec errorChain) Error() string { + return ec[0].Error() +} + +func (ec errorChain) Unwrap() error { + if len(ec) == 1 { + return nil + } + + return ec[1:] +} + +func (ec errorChain) As(target interface{}) bool { + return errors.As(ec[0], target) +} + +func (ec errorChain) Is(target error) bool { + return errors.Is(ec[0], target) +} diff --git a/libs/errors/errors_test.go b/libs/errs/aggregate_test.go similarity index 97% rename from libs/errors/errors_test.go rename to libs/errs/aggregate_test.go index fde34b510e..a307e956f5 100644 --- a/libs/errors/errors_test.go +++ b/libs/errs/aggregate_test.go @@ -1,4 +1,4 @@ -package errors +package errs import ( "errors"