From c40c81b8a742057e0711dbf340ac0ad6d7ca2760 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 8 Jul 2024 19:39:59 +0200 Subject: [PATCH 01/10] Return early in bundle destroy if no deployment exists --- bundle/destroy/assert_root_path_exists.go | 38 +++++++++++++++++++++++ bundle/phases/destroy.go | 3 +- bundle/seq.go | 17 +++++++++- libs/diag/diagnostic.go | 21 +++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 bundle/destroy/assert_root_path_exists.go diff --git a/bundle/destroy/assert_root_path_exists.go b/bundle/destroy/assert_root_path_exists.go new file mode 100644 index 00000000000..60307a2e560 --- /dev/null +++ b/bundle/destroy/assert_root_path_exists.go @@ -0,0 +1,38 @@ +package destroy + +import ( + "context" + "errors" + "net/http" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/log" + "github.com/databricks/databricks-sdk-go/apierr" +) + +type assertRootPathExists struct{} + +func AssertRootPathExists() bundle.Mutator { + return &assertRootPathExists{} +} + +func (m *assertRootPathExists) Name() string { + return "destroy:assert_root_path_exists" +} + +func (m *assertRootPathExists) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { + w := b.WorkspaceClient() + _, err := w.Workspace.GetStatusByPath(ctx, b.Config.Workspace.RootPath) + + if err != nil { + var aerr *apierr.APIError + if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { + log.Debugf(ctx, "No active deployment found. %s does not exist. Skipping destroy.", b.Config.Workspace.RootPath) + cmdio.LogString(ctx, "No active deployment found to destroy!") + return bundle.DiagnosticBreakSequence + } + } + return nil +} diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index f974a056599..772c94ec94f 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -5,12 +5,13 @@ import ( "github.com/databricks/cli/bundle/deploy/files" "github.com/databricks/cli/bundle/deploy/lock" "github.com/databricks/cli/bundle/deploy/terraform" + "github.com/databricks/cli/bundle/destroy" ) // The destroy phase deletes artifacts and resources. func Destroy() bundle.Mutator { - destroyMutator := bundle.Seq( + destroy.AssertRootPathExists(), lock.Acquire(), bundle.Defer( bundle.Seq( diff --git a/bundle/seq.go b/bundle/seq.go index c1260a3f08a..968bca60759 100644 --- a/bundle/seq.go +++ b/bundle/seq.go @@ -2,10 +2,15 @@ package bundle import ( "context" + "errors" "github.com/databricks/cli/libs/diag" ) +// Control signal error that can be used to break out of a sequence of mutators. +var ErrorBreakSequence = errors.New("break sequence") +var DiagnosticBreakSequence = diag.FromErr(ErrorBreakSequence) + type seqMutator struct { mutators []Mutator } @@ -17,8 +22,17 @@ func (s *seqMutator) Name() string { func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { var diags diag.Diagnostics for _, m := range s.mutators { - diags = diags.Extend(Apply(ctx, b, m)) + nd := Apply(ctx, b, m) + + // Break out of the sequence. Filter the ErrorBreakSequence error so that + // it does not show up to the user. + if nd.ContainsError(ErrorBreakSequence) { + diags.Extend(nd.FilterError(ErrorBreakSequence)) + break + } + if diags.HasError() { + diags.Extend(nd) break } } @@ -28,3 +42,4 @@ func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { func Seq(ms ...Mutator) Mutator { return &seqMutator{mutators: ms} } + diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index 6215275512a..27a4b677234 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -112,3 +112,24 @@ func (ds Diagnostics) Filter(severity Severity) Diagnostics { } return out } + +// Returns true if the diagnostics contain the specified error +func (ds Diagnostics) ContainsError(err error) bool { + for _, d := range ds { + if d.Severity == Error && d.Summary == err.Error() { + return true + } + } + return false +} + +// Filter returns a new list of diagnostics that do not contain the specified error +func (ds Diagnostics) FilterError(err error) Diagnostics { + var out Diagnostics + for _, d := range ds { + if d.Severity != Error || d.Summary != err.Error() { + out = append(out, d) + } + } + return out +} From d95315708f1428b9fbaeee74192158a5eca37e28 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 8 Jul 2024 19:52:23 +0200 Subject: [PATCH 02/10] - --- bundle/destroy/assert_root_path_exists.go | 2 +- bundle/seq.go | 3 +- libs/diag/diagnostic.go | 2 +- libs/diag/diagnostic_test.go | 89 +++++++++++++++++++++++ 4 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 libs/diag/diagnostic_test.go diff --git a/bundle/destroy/assert_root_path_exists.go b/bundle/destroy/assert_root_path_exists.go index 60307a2e560..63b51469ae2 100644 --- a/bundle/destroy/assert_root_path_exists.go +++ b/bundle/destroy/assert_root_path_exists.go @@ -29,7 +29,7 @@ func (m *assertRootPathExists) Apply(ctx context.Context, b *bundle.Bundle) diag if err != nil { var aerr *apierr.APIError if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { - log.Debugf(ctx, "No active deployment found. %s does not exist. Skipping destroy.", b.Config.Workspace.RootPath) + log.Infof(ctx, "No active deployment found. %s does not exist. Skipping destroy.", b.Config.Workspace.RootPath) cmdio.LogString(ctx, "No active deployment found to destroy!") return bundle.DiagnosticBreakSequence } diff --git a/bundle/seq.go b/bundle/seq.go index 968bca60759..d2c67f9ecfb 100644 --- a/bundle/seq.go +++ b/bundle/seq.go @@ -27,7 +27,7 @@ func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { // Break out of the sequence. Filter the ErrorBreakSequence error so that // it does not show up to the user. if nd.ContainsError(ErrorBreakSequence) { - diags.Extend(nd.FilterError(ErrorBreakSequence)) + diags.Extend(nd.RemoveError(ErrorBreakSequence)) break } @@ -42,4 +42,3 @@ func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { func Seq(ms ...Mutator) Mutator { return &seqMutator{mutators: ms} } - diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index 27a4b677234..e1a46d20b21 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -124,7 +124,7 @@ func (ds Diagnostics) ContainsError(err error) bool { } // Filter returns a new list of diagnostics that do not contain the specified error -func (ds Diagnostics) FilterError(err error) Diagnostics { +func (ds Diagnostics) RemoveError(err error) Diagnostics { var out Diagnostics for _, d := range ds { if d.Severity != Error || d.Summary != err.Error() { diff --git a/libs/diag/diagnostic_test.go b/libs/diag/diagnostic_test.go new file mode 100644 index 00000000000..6c79c479316 --- /dev/null +++ b/libs/diag/diagnostic_test.go @@ -0,0 +1,89 @@ +package diag + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDiagnosticContainsError(t *testing.T) { + diags := Diagnostics{ + { + Severity: Error, + Summary: "error 1", + }, + { + Severity: Error, + Summary: "error 2", + }, + { + Severity: Warning, + Summary: "warning 1", + }, + } + + assert.True(t, diags.ContainsError(errors.New("error 1"))) + assert.True(t, diags.ContainsError(errors.New("error 2"))) + assert.False(t, diags.ContainsError(errors.New("error 3"))) +} + +func TestDiagnosticRemoveError(t *testing.T) { + diags := Diagnostics{ + { + Severity: Error, + Summary: "error 1", + }, + { + Severity: Error, + Summary: "error 2", + }, + { + Severity: Warning, + Summary: "warning 1", + }, + } + + filtered := diags.RemoveError(errors.New("error 1")) + assert.Len(t, filtered, 2) + assert.Equal(t, Diagnostics{ + { + Severity: Error, + Summary: "error 2", + }, + { + Severity: Warning, + Summary: "warning 1", + }, + }, filtered) + + filtered = diags.RemoveError(errors.New("error 2")) + assert.Len(t, filtered, 2) + assert.Equal(t, Diagnostics{ + { + Severity: Error, + Summary: "error 1", + }, + { + Severity: Warning, + Summary: "warning 1", + }, + }, filtered) + + filtered = diags.RemoveError(errors.New("warning 1")) + assert.Len(t, filtered, 3) + assert.Equal(t, Diagnostics{ + { + Severity: Error, + Summary: "error 1", + }, + { + Severity: Error, + Summary: "error 2", + }, + { + Severity: Warning, + Summary: "warning 1", + }, + }, filtered) +} From 34c4d975e7729f016dd249f6e368764ebe85c5bb Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 8 Jul 2024 19:58:00 +0200 Subject: [PATCH 03/10] add test for the mutator --- .../destroy/assert_root_path_exists_test.go | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 bundle/destroy/assert_root_path_exists_test.go diff --git a/bundle/destroy/assert_root_path_exists_test.go b/bundle/destroy/assert_root_path_exists_test.go new file mode 100644 index 00000000000..ee2dec5572f --- /dev/null +++ b/bundle/destroy/assert_root_path_exists_test.go @@ -0,0 +1,34 @@ +package destroy + +import ( + "context" + "testing" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config" + "github.com/databricks/databricks-sdk-go/apierr" + "github.com/databricks/databricks-sdk-go/experimental/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +func TestAssertRootPathExists(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/path/to/root", + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + workspaceApi := m.GetMockWorkspaceAPI() + workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/path/to/root").Return(nil, &apierr.APIError{ + StatusCode: 404, + ErrorCode: "RESOURCE_DOES_NOT_EXIST", + }) + + diags := bundle.Apply(context.Background(), b, AssertRootPathExists()) + assert.Equal(t, bundle.DiagnosticBreakSequence, diags) +} From 32f120cfd8bf0b9593b2f8b793bda806e6abc89f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 11:01:09 +0200 Subject: [PATCH 04/10] fix seq --- bundle/seq.go | 16 ++++++++++------ libs/diag/diagnostic.go | 1 + 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/bundle/seq.go b/bundle/seq.go index d2c67f9ecfb..b217deb2cf5 100644 --- a/bundle/seq.go +++ b/bundle/seq.go @@ -8,6 +8,7 @@ import ( ) // Control signal error that can be used to break out of a sequence of mutators. +// TODO: Are better names possible? var ErrorBreakSequence = errors.New("break sequence") var DiagnosticBreakSequence = diag.FromErr(ErrorBreakSequence) @@ -23,16 +24,19 @@ func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { var diags diag.Diagnostics for _, m := range s.mutators { nd := Apply(ctx, b, m) + hasError := nd.HasError() - // Break out of the sequence. Filter the ErrorBreakSequence error so that - // it does not show up to the user. + // Remove the ErrorBreakSequence error from the diagnostics. It's a control + // signal and should not be shown to the user. if nd.ContainsError(ErrorBreakSequence) { - diags.Extend(nd.RemoveError(ErrorBreakSequence)) - break + nd.RemoveError(ErrorBreakSequence) } - if diags.HasError() { - diags.Extend(nd) + // Extend the diagnostics with the diagnostics from the current mutator. + diags = diags.Extend(nd) + + // Break out of the sequence if there is an error. + if hasError { break } } diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index e1a46d20b21..5f38a546ec1 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -124,6 +124,7 @@ func (ds Diagnostics) ContainsError(err error) bool { } // Filter returns a new list of diagnostics that do not contain the specified error +// Rename this to filter back again? func (ds Diagnostics) RemoveError(err error) Diagnostics { var out Diagnostics for _, d := range ds { From df24350724e2012ed027a06d66806ce656dc171f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 11:07:11 +0200 Subject: [PATCH 05/10] some cleanup --- bundle/destroy/assert_root_path_exists.go | 2 +- bundle/destroy/assert_root_path_exists_test.go | 2 +- bundle/seq.go | 15 ++++++++------- libs/diag/diagnostic.go | 1 - 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/bundle/destroy/assert_root_path_exists.go b/bundle/destroy/assert_root_path_exists.go index 63b51469ae2..1eb44019ff9 100644 --- a/bundle/destroy/assert_root_path_exists.go +++ b/bundle/destroy/assert_root_path_exists.go @@ -31,7 +31,7 @@ func (m *assertRootPathExists) Apply(ctx context.Context, b *bundle.Bundle) diag if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { log.Infof(ctx, "No active deployment found. %s does not exist. Skipping destroy.", b.Config.Workspace.RootPath) cmdio.LogString(ctx, "No active deployment found to destroy!") - return bundle.DiagnosticBreakSequence + return bundle.DiagnosticSequenceBreak } } return nil diff --git a/bundle/destroy/assert_root_path_exists_test.go b/bundle/destroy/assert_root_path_exists_test.go index ee2dec5572f..ae258dda10b 100644 --- a/bundle/destroy/assert_root_path_exists_test.go +++ b/bundle/destroy/assert_root_path_exists_test.go @@ -30,5 +30,5 @@ func TestAssertRootPathExists(t *testing.T) { }) diags := bundle.Apply(context.Background(), b, AssertRootPathExists()) - assert.Equal(t, bundle.DiagnosticBreakSequence, diags) + assert.Equal(t, bundle.DiagnosticSequenceBreak, diags) } diff --git a/bundle/seq.go b/bundle/seq.go index b217deb2cf5..e86f1d85053 100644 --- a/bundle/seq.go +++ b/bundle/seq.go @@ -7,10 +7,11 @@ import ( "github.com/databricks/cli/libs/diag" ) -// Control signal error that can be used to break out of a sequence of mutators. -// TODO: Are better names possible? -var ErrorBreakSequence = errors.New("break sequence") -var DiagnosticBreakSequence = diag.FromErr(ErrorBreakSequence) +// Control signal error that can be returned by a mutator to break out of a sequence. +var ErrorSequenceBreak = errors.New("break sequence") + +// Convenient diagnostic that wraps ErrorSequenceBreak. +var DiagnosticSequenceBreak = diag.FromErr(ErrorSequenceBreak) type seqMutator struct { mutators []Mutator @@ -26,10 +27,10 @@ func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { nd := Apply(ctx, b, m) hasError := nd.HasError() - // Remove the ErrorBreakSequence error from the diagnostics. It's a control + // Remove the ErrorSequenceBreak error from the diagnostics. It's a control // signal and should not be shown to the user. - if nd.ContainsError(ErrorBreakSequence) { - nd.RemoveError(ErrorBreakSequence) + if nd.ContainsError(ErrorSequenceBreak) { + nd.RemoveError(ErrorSequenceBreak) } // Extend the diagnostics with the diagnostics from the current mutator. diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index 5f38a546ec1..e1a46d20b21 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -124,7 +124,6 @@ func (ds Diagnostics) ContainsError(err error) bool { } // Filter returns a new list of diagnostics that do not contain the specified error -// Rename this to filter back again? func (ds Diagnostics) RemoveError(err error) Diagnostics { var out Diagnostics for _, d := range ds { From bf49b5b6a30d039ace67f8be0a533d722a8df573 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 11:28:12 +0200 Subject: [PATCH 06/10] add tests for the break signal --- bundle/seq.go | 2 +- bundle/seq_test.go | 81 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/bundle/seq.go b/bundle/seq.go index e86f1d85053..adbca63d89c 100644 --- a/bundle/seq.go +++ b/bundle/seq.go @@ -30,7 +30,7 @@ func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { // Remove the ErrorSequenceBreak error from the diagnostics. It's a control // signal and should not be shown to the user. if nd.ContainsError(ErrorSequenceBreak) { - nd.RemoveError(ErrorSequenceBreak) + nd = nd.RemoveError(ErrorSequenceBreak) } // Extend the diagnostics with the diagnostics from the current mutator. diff --git a/bundle/seq_test.go b/bundle/seq_test.go index 74f975ed8fb..9eb5e8f64b2 100644 --- a/bundle/seq_test.go +++ b/bundle/seq_test.go @@ -89,3 +89,84 @@ func TestSeqWithErrorInsideFinallyStage(t *testing.T) { assert.Equal(t, 1, errorMut.applyCalled) assert.Equal(t, 0, m3.applyCalled) } + +func TestSeqWithErrorSequenceBreak(t *testing.T) { + errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} + m1 := &testMutator{} + m2 := &testMutator{} + m3 := &testMutator{} + seqMutator := Seq(m1, m2, errorMut, m3) + + b := &Bundle{} + diags := Apply(context.Background(), b, seqMutator) + assert.NoError(t, diags.Error()) + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, m2.applyCalled) + assert.Equal(t, 1, errorMut.applyCalled) + + // m3 is not called because the error mutator returns a break control signal. + assert.Equal(t, 0, m3.applyCalled) +} + +func TestSeqWithErrorSequenceBreakInsideDeferFirst(t *testing.T) { + errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} + m1 := &testMutator{} + m2 := &testMutator{} + m3 := &testMutator{} + seqMutator := Seq(m1, Defer(errorMut, m2), m3) + + b := &Bundle{} + diags := Apply(context.Background(), b, seqMutator) + assert.NoError(t, diags.Error()) + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, errorMut.applyCalled) + + // m2 should still be called because it's inside a Defer + assert.Equal(t, 1, m2.applyCalled) + assert.Equal(t, 0, m3.applyCalled) +} + +func TestSeqWithErrorSequenceBreakInsideDeferSecond(t *testing.T) { + errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} + m1 := &testMutator{} + m2 := &testMutator{} + m3 := &testMutator{} + seqMutator := Seq(m1, Defer(m2, errorMut), m3) + + b := &Bundle{} + diags := Apply(context.Background(), b, seqMutator) + assert.NoError(t, diags.Error()) + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, m2.applyCalled) + assert.Equal(t, 1, errorMut.applyCalled) + + // m3 is not called because the defer mutator returns a break control signal. + assert.Equal(t, 0, m3.applyCalled) +} + +func TestSeqErrorSequenceBreakDoesNotBreakMultipleSequences(t *testing.T) { + errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} + m1 := &testMutator{} + m2 := &testMutator{} + m3 := &testMutator{} + m4 := &testMutator{} + seqMutator := Seq(Seq(m1, errorMut, m2), Seq(m3, m4)) + + b := &Bundle{} + diags := Apply(context.Background(), b, seqMutator) + assert.NoError(t, diags.Error()) + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 1, errorMut.applyCalled) + + // m2 is not applied because the error mutator returns a break control signal. + assert.Equal(t, 0, m2.applyCalled) + + // m3 and m4 are still applied because the break control signal error should + // only break the current sequence, not the top level one. + assert.Equal(t, 1, m3.applyCalled) + assert.Equal(t, 1, m4.applyCalled) +} From abbb891992f1e4941dfa0d5c78dbe022745716c6 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 11:42:06 +0200 Subject: [PATCH 07/10] more coverage for assert root path --- bundle/destroy/assert_root_path_exists.go | 15 +++++++------ .../destroy/assert_root_path_exists_test.go | 21 ++++++++++++++++++- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/bundle/destroy/assert_root_path_exists.go b/bundle/destroy/assert_root_path_exists.go index 1eb44019ff9..3a81458c9b1 100644 --- a/bundle/destroy/assert_root_path_exists.go +++ b/bundle/destroy/assert_root_path_exists.go @@ -3,6 +3,7 @@ package destroy import ( "context" "errors" + "fmt" "net/http" "github.com/databricks/cli/bundle" @@ -26,13 +27,11 @@ func (m *assertRootPathExists) Apply(ctx context.Context, b *bundle.Bundle) diag w := b.WorkspaceClient() _, err := w.Workspace.GetStatusByPath(ctx, b.Config.Workspace.RootPath) - if err != nil { - var aerr *apierr.APIError - if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { - log.Infof(ctx, "No active deployment found. %s does not exist. Skipping destroy.", b.Config.Workspace.RootPath) - cmdio.LogString(ctx, "No active deployment found to destroy!") - return bundle.DiagnosticSequenceBreak - } + var aerr *apierr.APIError + if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { + log.Infof(ctx, "No active deployment found. %s does not exist. Skipping destroy.", b.Config.Workspace.RootPath) + cmdio.LogString(ctx, "No active deployment found to destroy!") + return bundle.DiagnosticSequenceBreak } - return nil + return diag.FromErr(fmt.Errorf("cannot assert root path exists: %w", err)) } diff --git a/bundle/destroy/assert_root_path_exists_test.go b/bundle/destroy/assert_root_path_exists_test.go index ae258dda10b..87013a2bc85 100644 --- a/bundle/destroy/assert_root_path_exists_test.go +++ b/bundle/destroy/assert_root_path_exists_test.go @@ -2,6 +2,7 @@ package destroy import ( "context" + "errors" "testing" "github.com/databricks/cli/bundle" @@ -30,5 +31,23 @@ func TestAssertRootPathExists(t *testing.T) { }) diags := bundle.Apply(context.Background(), b, AssertRootPathExists()) - assert.Equal(t, bundle.DiagnosticSequenceBreak, diags) + assert.Equal(t, bundle.ErrorSequenceBreak, diags.Error()) +} + +func TestAssertRootPathExistsIgnoresNon404Errors(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Workspace: config.Workspace{ + RootPath: "/path/to/root", + }, + }, + } + + m := mocks.NewMockWorkspaceClient(t) + b.SetWorkpaceClient(m.WorkspaceClient) + workspaceApi := m.GetMockWorkspaceAPI() + workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/path/to/root").Return(nil, errors.New("wsfs API failed")) + + diags := bundle.Apply(context.Background(), b, AssertRootPathExists()) + assert.EqualError(t, diags.Error(), "cannot assert root path exists: wsfs API failed") } From 27b09ceb2733ecf33bd5302bacd15974d413a24f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 15:36:53 +0200 Subject: [PATCH 08/10] use if mutator instead --- bundle/config/mutator/if.go | 11 ++- bundle/destroy/assert_root_path_exists.go | 37 -------- .../destroy/assert_root_path_exists_test.go | 53 ----------- bundle/phases/destroy.go | 31 ++++++- bundle/python/transform.go | 6 +- bundle/seq.go | 23 +---- bundle/seq_test.go | 81 ----------------- libs/diag/diagnostic.go | 21 ----- libs/diag/diagnostic_test.go | 89 ------------------- 9 files changed, 42 insertions(+), 310 deletions(-) delete mode 100644 bundle/destroy/assert_root_path_exists.go delete mode 100644 bundle/destroy/assert_root_path_exists_test.go delete mode 100644 libs/diag/diagnostic_test.go diff --git a/bundle/config/mutator/if.go b/bundle/config/mutator/if.go index 1b7856b3c3c..f4469efc1e1 100644 --- a/bundle/config/mutator/if.go +++ b/bundle/config/mutator/if.go @@ -8,13 +8,13 @@ import ( ) type ifMutator struct { - condition func(*bundle.Bundle) bool + condition func(context.Context, *bundle.Bundle) (bool, error) onTrueMutator bundle.Mutator onFalseMutator bundle.Mutator } func If( - condition func(*bundle.Bundle) bool, + condition func(context.Context, *bundle.Bundle) (bool, error), onTrueMutator bundle.Mutator, onFalseMutator bundle.Mutator, ) bundle.Mutator { @@ -24,7 +24,12 @@ func If( } func (m *ifMutator) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - if m.condition(b) { + v, err := m.condition(ctx, b) + if err != nil { + return diag.FromErr(err) + } + + if v { return bundle.Apply(ctx, b, m.onTrueMutator) } else { return bundle.Apply(ctx, b, m.onFalseMutator) diff --git a/bundle/destroy/assert_root_path_exists.go b/bundle/destroy/assert_root_path_exists.go deleted file mode 100644 index 3a81458c9b1..00000000000 --- a/bundle/destroy/assert_root_path_exists.go +++ /dev/null @@ -1,37 +0,0 @@ -package destroy - -import ( - "context" - "errors" - "fmt" - "net/http" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/libs/cmdio" - "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/log" - "github.com/databricks/databricks-sdk-go/apierr" -) - -type assertRootPathExists struct{} - -func AssertRootPathExists() bundle.Mutator { - return &assertRootPathExists{} -} - -func (m *assertRootPathExists) Name() string { - return "destroy:assert_root_path_exists" -} - -func (m *assertRootPathExists) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - w := b.WorkspaceClient() - _, err := w.Workspace.GetStatusByPath(ctx, b.Config.Workspace.RootPath) - - var aerr *apierr.APIError - if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { - log.Infof(ctx, "No active deployment found. %s does not exist. Skipping destroy.", b.Config.Workspace.RootPath) - cmdio.LogString(ctx, "No active deployment found to destroy!") - return bundle.DiagnosticSequenceBreak - } - return diag.FromErr(fmt.Errorf("cannot assert root path exists: %w", err)) -} diff --git a/bundle/destroy/assert_root_path_exists_test.go b/bundle/destroy/assert_root_path_exists_test.go deleted file mode 100644 index 87013a2bc85..00000000000 --- a/bundle/destroy/assert_root_path_exists_test.go +++ /dev/null @@ -1,53 +0,0 @@ -package destroy - -import ( - "context" - "errors" - "testing" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config" - "github.com/databricks/databricks-sdk-go/apierr" - "github.com/databricks/databricks-sdk-go/experimental/mocks" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/mock" -) - -func TestAssertRootPathExists(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - RootPath: "/path/to/root", - }, - }, - } - - m := mocks.NewMockWorkspaceClient(t) - b.SetWorkpaceClient(m.WorkspaceClient) - workspaceApi := m.GetMockWorkspaceAPI() - workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/path/to/root").Return(nil, &apierr.APIError{ - StatusCode: 404, - ErrorCode: "RESOURCE_DOES_NOT_EXIST", - }) - - diags := bundle.Apply(context.Background(), b, AssertRootPathExists()) - assert.Equal(t, bundle.ErrorSequenceBreak, diags.Error()) -} - -func TestAssertRootPathExistsIgnoresNon404Errors(t *testing.T) { - b := &bundle.Bundle{ - Config: config.Root{ - Workspace: config.Workspace{ - RootPath: "/path/to/root", - }, - }, - } - - m := mocks.NewMockWorkspaceClient(t) - b.SetWorkpaceClient(m.WorkspaceClient) - workspaceApi := m.GetMockWorkspaceAPI() - workspaceApi.EXPECT().GetStatusByPath(mock.Anything, "/path/to/root").Return(nil, errors.New("wsfs API failed")) - - diags := bundle.Apply(context.Background(), b, AssertRootPathExists()) - assert.EqualError(t, diags.Error(), "cannot assert root path exists: wsfs API failed") -} diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index 772c94ec94f..6754c2c5f85 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -1,17 +1,35 @@ package phases import ( + "context" + "errors" + "net/http" + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/deploy/files" "github.com/databricks/cli/bundle/deploy/lock" "github.com/databricks/cli/bundle/deploy/terraform" - "github.com/databricks/cli/bundle/destroy" + "github.com/databricks/cli/libs/log" + "github.com/databricks/databricks-sdk-go/apierr" ) +func assertRootPathExists(ctx context.Context, b *bundle.Bundle) (bool, error) { + w := b.WorkspaceClient() + _, err := w.Workspace.GetStatusByPath(ctx, b.Config.Workspace.RootPath) + + var aerr *apierr.APIError + if errors.As(err, &aerr) && aerr.StatusCode == http.StatusNotFound { + log.Infof(ctx, "Root path does not exist: %s", b.Config.Workspace.RootPath) + return false, nil + } + + return true, err +} + // The destroy phase deletes artifacts and resources. func Destroy() bundle.Mutator { destroyMutator := bundle.Seq( - destroy.AssertRootPathExists(), lock.Acquire(), bundle.Defer( bundle.Seq( @@ -30,6 +48,13 @@ func Destroy() bundle.Mutator { return newPhase( "destroy", - []bundle.Mutator{destroyMutator}, + []bundle.Mutator{ + // Only run deploy mutator if root path exists. + mutator.If( + assertRootPathExists, + destroyMutator, + bundle.LogString("No active deployment found to destroy!"), + ), + }, ) } diff --git a/bundle/python/transform.go b/bundle/python/transform.go index 457b45f7841..690e838787d 100644 --- a/bundle/python/transform.go +++ b/bundle/python/transform.go @@ -1,6 +1,7 @@ package python import ( + "context" "fmt" "strconv" "strings" @@ -64,8 +65,9 @@ dbutils.notebook.exit(s) // entry point. func TransformWheelTask() bundle.Mutator { return mutator.If( - func(b *bundle.Bundle) bool { - return b.Config.Experimental != nil && b.Config.Experimental.PythonWheelWrapper + func(_ context.Context, b *bundle.Bundle) (bool, error) { + res := b.Config.Experimental != nil && b.Config.Experimental.PythonWheelWrapper + return res, nil }, mutator.NewTrampoline( "python_wheel", diff --git a/bundle/seq.go b/bundle/seq.go index adbca63d89c..c1260a3f08a 100644 --- a/bundle/seq.go +++ b/bundle/seq.go @@ -2,17 +2,10 @@ package bundle import ( "context" - "errors" "github.com/databricks/cli/libs/diag" ) -// Control signal error that can be returned by a mutator to break out of a sequence. -var ErrorSequenceBreak = errors.New("break sequence") - -// Convenient diagnostic that wraps ErrorSequenceBreak. -var DiagnosticSequenceBreak = diag.FromErr(ErrorSequenceBreak) - type seqMutator struct { mutators []Mutator } @@ -24,20 +17,8 @@ func (s *seqMutator) Name() string { func (s *seqMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { var diags diag.Diagnostics for _, m := range s.mutators { - nd := Apply(ctx, b, m) - hasError := nd.HasError() - - // Remove the ErrorSequenceBreak error from the diagnostics. It's a control - // signal and should not be shown to the user. - if nd.ContainsError(ErrorSequenceBreak) { - nd = nd.RemoveError(ErrorSequenceBreak) - } - - // Extend the diagnostics with the diagnostics from the current mutator. - diags = diags.Extend(nd) - - // Break out of the sequence if there is an error. - if hasError { + diags = diags.Extend(Apply(ctx, b, m)) + if diags.HasError() { break } } diff --git a/bundle/seq_test.go b/bundle/seq_test.go index 9eb5e8f64b2..74f975ed8fb 100644 --- a/bundle/seq_test.go +++ b/bundle/seq_test.go @@ -89,84 +89,3 @@ func TestSeqWithErrorInsideFinallyStage(t *testing.T) { assert.Equal(t, 1, errorMut.applyCalled) assert.Equal(t, 0, m3.applyCalled) } - -func TestSeqWithErrorSequenceBreak(t *testing.T) { - errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} - m1 := &testMutator{} - m2 := &testMutator{} - m3 := &testMutator{} - seqMutator := Seq(m1, m2, errorMut, m3) - - b := &Bundle{} - diags := Apply(context.Background(), b, seqMutator) - assert.NoError(t, diags.Error()) - - assert.Equal(t, 1, m1.applyCalled) - assert.Equal(t, 1, m2.applyCalled) - assert.Equal(t, 1, errorMut.applyCalled) - - // m3 is not called because the error mutator returns a break control signal. - assert.Equal(t, 0, m3.applyCalled) -} - -func TestSeqWithErrorSequenceBreakInsideDeferFirst(t *testing.T) { - errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} - m1 := &testMutator{} - m2 := &testMutator{} - m3 := &testMutator{} - seqMutator := Seq(m1, Defer(errorMut, m2), m3) - - b := &Bundle{} - diags := Apply(context.Background(), b, seqMutator) - assert.NoError(t, diags.Error()) - - assert.Equal(t, 1, m1.applyCalled) - assert.Equal(t, 1, errorMut.applyCalled) - - // m2 should still be called because it's inside a Defer - assert.Equal(t, 1, m2.applyCalled) - assert.Equal(t, 0, m3.applyCalled) -} - -func TestSeqWithErrorSequenceBreakInsideDeferSecond(t *testing.T) { - errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} - m1 := &testMutator{} - m2 := &testMutator{} - m3 := &testMutator{} - seqMutator := Seq(m1, Defer(m2, errorMut), m3) - - b := &Bundle{} - diags := Apply(context.Background(), b, seqMutator) - assert.NoError(t, diags.Error()) - - assert.Equal(t, 1, m1.applyCalled) - assert.Equal(t, 1, m2.applyCalled) - assert.Equal(t, 1, errorMut.applyCalled) - - // m3 is not called because the defer mutator returns a break control signal. - assert.Equal(t, 0, m3.applyCalled) -} - -func TestSeqErrorSequenceBreakDoesNotBreakMultipleSequences(t *testing.T) { - errorMut := &mutatorWithError{errorMsg: ErrorSequenceBreak.Error()} - m1 := &testMutator{} - m2 := &testMutator{} - m3 := &testMutator{} - m4 := &testMutator{} - seqMutator := Seq(Seq(m1, errorMut, m2), Seq(m3, m4)) - - b := &Bundle{} - diags := Apply(context.Background(), b, seqMutator) - assert.NoError(t, diags.Error()) - - assert.Equal(t, 1, m1.applyCalled) - assert.Equal(t, 1, errorMut.applyCalled) - - // m2 is not applied because the error mutator returns a break control signal. - assert.Equal(t, 0, m2.applyCalled) - - // m3 and m4 are still applied because the break control signal error should - // only break the current sequence, not the top level one. - assert.Equal(t, 1, m3.applyCalled) - assert.Equal(t, 1, m4.applyCalled) -} diff --git a/libs/diag/diagnostic.go b/libs/diag/diagnostic.go index e1a46d20b21..6215275512a 100644 --- a/libs/diag/diagnostic.go +++ b/libs/diag/diagnostic.go @@ -112,24 +112,3 @@ func (ds Diagnostics) Filter(severity Severity) Diagnostics { } return out } - -// Returns true if the diagnostics contain the specified error -func (ds Diagnostics) ContainsError(err error) bool { - for _, d := range ds { - if d.Severity == Error && d.Summary == err.Error() { - return true - } - } - return false -} - -// Filter returns a new list of diagnostics that do not contain the specified error -func (ds Diagnostics) RemoveError(err error) Diagnostics { - var out Diagnostics - for _, d := range ds { - if d.Severity != Error || d.Summary != err.Error() { - out = append(out, d) - } - } - return out -} diff --git a/libs/diag/diagnostic_test.go b/libs/diag/diagnostic_test.go deleted file mode 100644 index 6c79c479316..00000000000 --- a/libs/diag/diagnostic_test.go +++ /dev/null @@ -1,89 +0,0 @@ -package diag - -import ( - "errors" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestDiagnosticContainsError(t *testing.T) { - diags := Diagnostics{ - { - Severity: Error, - Summary: "error 1", - }, - { - Severity: Error, - Summary: "error 2", - }, - { - Severity: Warning, - Summary: "warning 1", - }, - } - - assert.True(t, diags.ContainsError(errors.New("error 1"))) - assert.True(t, diags.ContainsError(errors.New("error 2"))) - assert.False(t, diags.ContainsError(errors.New("error 3"))) -} - -func TestDiagnosticRemoveError(t *testing.T) { - diags := Diagnostics{ - { - Severity: Error, - Summary: "error 1", - }, - { - Severity: Error, - Summary: "error 2", - }, - { - Severity: Warning, - Summary: "warning 1", - }, - } - - filtered := diags.RemoveError(errors.New("error 1")) - assert.Len(t, filtered, 2) - assert.Equal(t, Diagnostics{ - { - Severity: Error, - Summary: "error 2", - }, - { - Severity: Warning, - Summary: "warning 1", - }, - }, filtered) - - filtered = diags.RemoveError(errors.New("error 2")) - assert.Len(t, filtered, 2) - assert.Equal(t, Diagnostics{ - { - Severity: Error, - Summary: "error 1", - }, - { - Severity: Warning, - Summary: "warning 1", - }, - }, filtered) - - filtered = diags.RemoveError(errors.New("warning 1")) - assert.Len(t, filtered, 3) - assert.Equal(t, Diagnostics{ - { - Severity: Error, - Summary: "error 1", - }, - { - Severity: Error, - Summary: "error 2", - }, - { - Severity: Warning, - Summary: "warning 1", - }, - }, filtered) -} From bc42096233d38adde3ad5d0343a6f58e6729c475 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 15:41:56 +0200 Subject: [PATCH 09/10] move if to bundle package --- bundle/config/mutator/if.go | 41 ------------------------------------- bundle/if.go | 40 ++++++++++++++++++++++++++++++++++++ bundle/phases/destroy.go | 3 +-- bundle/python/transform.go | 2 +- 4 files changed, 42 insertions(+), 44 deletions(-) delete mode 100644 bundle/config/mutator/if.go create mode 100644 bundle/if.go diff --git a/bundle/config/mutator/if.go b/bundle/config/mutator/if.go deleted file mode 100644 index f4469efc1e1..00000000000 --- a/bundle/config/mutator/if.go +++ /dev/null @@ -1,41 +0,0 @@ -package mutator - -import ( - "context" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/libs/diag" -) - -type ifMutator struct { - condition func(context.Context, *bundle.Bundle) (bool, error) - onTrueMutator bundle.Mutator - onFalseMutator bundle.Mutator -} - -func If( - condition func(context.Context, *bundle.Bundle) (bool, error), - onTrueMutator bundle.Mutator, - onFalseMutator bundle.Mutator, -) bundle.Mutator { - return &ifMutator{ - condition, onTrueMutator, onFalseMutator, - } -} - -func (m *ifMutator) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - v, err := m.condition(ctx, b) - if err != nil { - return diag.FromErr(err) - } - - if v { - return bundle.Apply(ctx, b, m.onTrueMutator) - } else { - return bundle.Apply(ctx, b, m.onFalseMutator) - } -} - -func (m *ifMutator) Name() string { - return "If" -} diff --git a/bundle/if.go b/bundle/if.go new file mode 100644 index 00000000000..bad1d72d2f2 --- /dev/null +++ b/bundle/if.go @@ -0,0 +1,40 @@ +package bundle + +import ( + "context" + + "github.com/databricks/cli/libs/diag" +) + +type ifMutator struct { + condition func(context.Context, *Bundle) (bool, error) + onTrueMutator Mutator + onFalseMutator Mutator +} + +func If( + condition func(context.Context, *Bundle) (bool, error), + onTrueMutator Mutator, + onFalseMutator Mutator, +) Mutator { + return &ifMutator{ + condition, onTrueMutator, onFalseMutator, + } +} + +func (m *ifMutator) Apply(ctx context.Context, b *Bundle) diag.Diagnostics { + v, err := m.condition(ctx, b) + if err != nil { + return diag.FromErr(err) + } + + if v { + return Apply(ctx, b, m.onTrueMutator) + } else { + return Apply(ctx, b, m.onFalseMutator) + } +} + +func (m *ifMutator) Name() string { + return "If" +} diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index 6754c2c5f85..f1beace848a 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -6,7 +6,6 @@ import ( "net/http" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/config/mutator" "github.com/databricks/cli/bundle/deploy/files" "github.com/databricks/cli/bundle/deploy/lock" "github.com/databricks/cli/bundle/deploy/terraform" @@ -50,7 +49,7 @@ func Destroy() bundle.Mutator { "destroy", []bundle.Mutator{ // Only run deploy mutator if root path exists. - mutator.If( + bundle.If( assertRootPathExists, destroyMutator, bundle.LogString("No active deployment found to destroy!"), diff --git a/bundle/python/transform.go b/bundle/python/transform.go index 690e838787d..9d3b1ab6a53 100644 --- a/bundle/python/transform.go +++ b/bundle/python/transform.go @@ -64,7 +64,7 @@ dbutils.notebook.exit(s) // which installs uploaded wheels using %pip and then calling corresponding // entry point. func TransformWheelTask() bundle.Mutator { - return mutator.If( + return bundle.If( func(_ context.Context, b *bundle.Bundle) (bool, error) { res := b.Config.Experimental != nil && b.Config.Experimental.PythonWheelWrapper return res, nil From bedfcdada6d25adfb13dd95bf00962331d688159 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 15:47:03 +0200 Subject: [PATCH 10/10] add tests for the if mutator --- bundle/if_test.go | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 bundle/if_test.go diff --git a/bundle/if_test.go b/bundle/if_test.go new file mode 100644 index 00000000000..b3fc0b9d908 --- /dev/null +++ b/bundle/if_test.go @@ -0,0 +1,53 @@ +package bundle + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIfMutatorTrue(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + ifMutator := If(func(context.Context, *Bundle) (bool, error) { + return true, nil + }, m1, m2) + + b := &Bundle{} + diags := Apply(context.Background(), b, ifMutator) + assert.NoError(t, diags.Error()) + + assert.Equal(t, 1, m1.applyCalled) + assert.Equal(t, 0, m2.applyCalled) +} + +func TestIfMutatorFalse(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + ifMutator := If(func(context.Context, *Bundle) (bool, error) { + return false, nil + }, m1, m2) + + b := &Bundle{} + diags := Apply(context.Background(), b, ifMutator) + assert.NoError(t, diags.Error()) + + assert.Equal(t, 0, m1.applyCalled) + assert.Equal(t, 1, m2.applyCalled) +} + +func TestIfMutatorError(t *testing.T) { + m1 := &testMutator{} + m2 := &testMutator{} + ifMutator := If(func(context.Context, *Bundle) (bool, error) { + return true, assert.AnError + }, m1, m2) + + b := &Bundle{} + diags := Apply(context.Background(), b, ifMutator) + assert.Error(t, diags.Error()) + + assert.Equal(t, 0, m1.applyCalled) + assert.Equal(t, 0, m2.applyCalled) +}