From c40c81b8a742057e0711dbf340ac0ad6d7ca2760 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Mon, 8 Jul 2024 19:39:59 +0200 Subject: [PATCH 01/20] 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/20] - --- 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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) +} From 217a7cd300c4b16ff6dec067ee1bcb142f84ea18 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 19:36:57 +0200 Subject: [PATCH 11/20] Move to a single prompt during `bundle destroy` --- bundle/bundle.go | 5 ++ bundle/deploy/files/delete.go | 23 ++------- bundle/deploy/terraform/destroy.go | 83 ++---------------------------- bundle/deploy/terraform/plan.go | 11 ++-- bundle/phases/destroy.go | 71 +++++++++++++++++++++++-- libs/terraform/plan.go | 35 +++++++++++-- 6 files changed, 116 insertions(+), 112 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index 032d98abc85..d5f6ba8a566 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -69,6 +69,11 @@ type Bundle struct { // files AutoApprove bool + // If true, we require user approval to deploy. This is + // TODO: On both destroy and deploy, error with suggesting `--auto-approve` + // if operating from a non-tty. + RequireApprovalForDeploy bool + // Tagging is used to normalize tag keys and values. // The implementation depends on the cloud being targeted. Tagging tags.Cloud diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index 1339714495e..7ae407e4954 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -10,9 +10,9 @@ import ( "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/cli/libs/sync" "github.com/databricks/databricks-sdk-go/service/workspace" - "github.com/fatih/color" ) type delete struct{} @@ -22,24 +22,7 @@ func (m *delete) Name() string { } func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Do not delete files if terraform destroy was not consented - if !b.Plan.IsEmpty && !b.Plan.ConfirmApply { - return nil - } - - cmdio.LogString(ctx, "Starting deletion of remote bundle files") - cmdio.LogString(ctx, fmt.Sprintf("Bundle remote directory is %s", b.Config.Workspace.RootPath)) - - red := color.New(color.FgRed).SprintFunc() - if !b.AutoApprove { - proceed, err := cmdio.AskYesOrNo(ctx, fmt.Sprintf("\n%s and all files in it will be %s Proceed?", b.Config.Workspace.RootPath, red("deleted permanently!"))) - if err != nil { - return diag.FromErr(err) - } - if !proceed { - return nil - } - } + cmdio.LogString(ctx, "Deleting files...") err := b.WorkspaceClient().Workspace.Delete(ctx, workspace.Delete{ Path: b.Config.Workspace.RootPath, @@ -55,7 +38,7 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return diag.FromErr(err) } - cmdio.LogString(ctx, "Successfully deleted files!") + log.Debugf(ctx, "Successfully deleted files!") return nil } diff --git a/bundle/deploy/terraform/destroy.go b/bundle/deploy/terraform/destroy.go index 16f074a2224..cfab2a8add7 100644 --- a/bundle/deploy/terraform/destroy.go +++ b/bundle/deploy/terraform/destroy.go @@ -2,61 +2,14 @@ package terraform import ( "context" - "fmt" - "strings" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" - "github.com/fatih/color" + "github.com/databricks/cli/libs/log" "github.com/hashicorp/terraform-exec/tfexec" - tfjson "github.com/hashicorp/terraform-json" ) -type PlanResourceChange struct { - ResourceType string `json:"resource_type"` - Action string `json:"action"` - ResourceName string `json:"resource_name"` -} - -func (c *PlanResourceChange) String() string { - result := strings.Builder{} - switch c.Action { - case "delete": - result.WriteString(" delete ") - default: - result.WriteString(c.Action + " ") - } - switch c.ResourceType { - case "databricks_job": - result.WriteString("job ") - case "databricks_pipeline": - result.WriteString("pipeline ") - default: - result.WriteString(c.ResourceType + " ") - } - result.WriteString(c.ResourceName) - return result.String() -} - -func (c *PlanResourceChange) IsInplaceSupported() bool { - return false -} - -func logDestroyPlan(ctx context.Context, changes []*tfjson.ResourceChange) error { - cmdio.LogString(ctx, "The following resources will be removed:") - for _, c := range changes { - if c.Change.Actions.Delete() { - cmdio.Log(ctx, &PlanResourceChange{ - ResourceType: c.Type, - Action: "delete", - ResourceName: c.Name, - }) - } - } - return nil -} - type destroy struct{} func (w *destroy) Name() string { @@ -66,7 +19,7 @@ func (w *destroy) Name() string { func (w *destroy) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { // return early if plan is empty if b.Plan.IsEmpty { - cmdio.LogString(ctx, "No resources to destroy in plan. Skipping destroy!") + cmdio.LogString(ctx, "No resources to destroy`") return nil } @@ -75,45 +28,19 @@ func (w *destroy) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics return diag.Errorf("terraform not initialized") } - // read plan file - plan, err := tf.ShowPlanFile(ctx, b.Plan.Path) - if err != nil { - return diag.FromErr(err) - } - - // print the resources that will be destroyed - err = logDestroyPlan(ctx, plan.ResourceChanges) - if err != nil { - return diag.FromErr(err) - } - - // Ask for confirmation, if needed - if !b.Plan.ConfirmApply { - red := color.New(color.FgRed).SprintFunc() - b.Plan.ConfirmApply, err = cmdio.AskYesOrNo(ctx, fmt.Sprintf("\nThis will permanently %s resources! Proceed?", red("destroy"))) - if err != nil { - return diag.FromErr(err) - } - } - - // return if confirmation was not provided - if !b.Plan.ConfirmApply { - return nil - } - if b.Plan.Path == "" { return diag.Errorf("no plan found") } - cmdio.LogString(ctx, "Starting to destroy resources") + log.Debugf(ctx, "Starting to destroy resources") // Apply terraform according to the computed destroy plan - err = tf.Apply(ctx, tfexec.DirOrPlan(b.Plan.Path)) + err := tf.Apply(ctx, tfexec.DirOrPlan(b.Plan.Path)) if err != nil { return diag.Errorf("terraform destroy: %v", err) } - cmdio.LogString(ctx, "Successfully destroyed resources!") + log.Debugf(ctx, "Successfully destroyed resources!") return nil } diff --git a/bundle/deploy/terraform/plan.go b/bundle/deploy/terraform/plan.go index 50e0f78ca2e..d01c6abab51 100644 --- a/bundle/deploy/terraform/plan.go +++ b/bundle/deploy/terraform/plan.go @@ -6,8 +6,8 @@ import ( "path/filepath" "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/cli/libs/terraform" "github.com/hashicorp/terraform-exec/tfexec" ) @@ -33,7 +33,7 @@ func (p *plan) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return diag.Errorf("terraform not initialized") } - cmdio.LogString(ctx, "Starting plan computation") + log.Debugf(ctx, "Starting plan computation") err := tf.Init(ctx, tfexec.Upgrade(true)) if err != nil { @@ -55,12 +55,11 @@ func (p *plan) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { // Set plan in main bundle struct for downstream mutators b.Plan = &terraform.Plan{ - Path: planPath, - ConfirmApply: b.AutoApprove, - IsEmpty: !notEmpty, + Path: planPath, + IsEmpty: !notEmpty, } - cmdio.LogString(ctx, fmt.Sprintf("Planning complete and persisted at %s\n", planPath)) + log.Debugf(ctx, fmt.Sprintf("Planning complete and persisted at %s\n", planPath)) return nil } diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index f1beace848a..47c11595f0d 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -3,13 +3,20 @@ package phases import ( "context" "errors" + "fmt" "net/http" "github.com/databricks/cli/bundle" "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/libs/cmdio" "github.com/databricks/cli/libs/log" + + // TODO: Rename the module itself? + + terraformlib "github.com/databricks/cli/libs/terraform" "github.com/databricks/databricks-sdk-go/apierr" ) @@ -26,8 +33,63 @@ func assertRootPathExists(ctx context.Context, b *bundle.Bundle) (bool, error) { return true, err } +func approvalForDestroy(ctx context.Context, b *bundle.Bundle) (bool, error) { + tf := b.Terraform + if tf == nil { + return false, fmt.Errorf("terraform not initialized") + } + + // read plan file + plan, err := tf.ShowPlanFile(ctx, b.Plan.Path) + if err != nil { + return false, err + } + + deleteActions := make([]terraformlib.Action, 0) + for _, rc := range plan.ResourceChanges { + if rc.Change.Actions.Delete() { + deleteActions = append(deleteActions, terraformlib.Action{ + Action: "delete", + ResourceType: rc.Type, + ResourceName: rc.Name, + }) + } + } + + if len(deleteActions) > 0 { + cmdio.LogString(ctx, "The following resources will be deleted:") + for _, a := range deleteActions { + cmdio.Log(ctx, a) + } + cmdio.LogString(ctx, "") + + } + + cmdio.LogString(ctx, fmt.Sprintf("All files and directories at the following location will be deleted: %s", b.Config.Workspace.RootPath)) + cmdio.LogString(ctx, "") + + if b.AutoApprove { + return true, nil + } + + approved, err := cmdio.AskYesOrNo(ctx, "Would you like to proceed?") + if err != nil { + return false, err + } + + return approved, nil +} + // The destroy phase deletes artifacts and resources. func Destroy() bundle.Mutator { + // Core destructive mutators for destroy. These require informed user consent. + destroyCore := bundle.Seq( + terraform.Destroy(), + terraform.StatePush(), + files.Delete(), + bundle.LogString("Destroy complete!"), + ) + destroyMutator := bundle.Seq( lock.Acquire(), bundle.Defer( @@ -36,13 +98,14 @@ func Destroy() bundle.Mutator { terraform.Interpolate(), terraform.Write(), terraform.Plan(terraform.PlanGoal("destroy")), - terraform.Destroy(), - terraform.StatePush(), - files.Delete(), + bundle.If( + approvalForDestroy, + destroyCore, + bundle.LogString("Destroy cancelled!"), + ), ), lock.Release(lock.GoalDestroy), ), - bundle.LogString("Destroy complete!"), ) return newPhase( diff --git a/libs/terraform/plan.go b/libs/terraform/plan.go index 22fea620637..476b8fd54bb 100644 --- a/libs/terraform/plan.go +++ b/libs/terraform/plan.go @@ -1,13 +1,40 @@ package terraform +import "strings" + type Plan struct { // Path to the plan Path string - // Holds whether the user can consented to destruction. Either by interactive - // confirmation or by passing a command line flag - ConfirmApply bool - // If true, the plan is empty and applying it will not do anything IsEmpty bool } + +type Action struct { + // Type and name of the resource + ResourceType string `json:"resource_type"` + ResourceName string `json:"resource_name"` + + Action ActionType `json:"action"` +} + +func (a Action) String() string { + // terraform resources have the databricks_ prefix, which is not needed. + rtype := strings.TrimPrefix(a.ResourceType, "databricks_") + return strings.Join([]string{" ", string(a.Action), rtype, a.ResourceName}, " ") +} + +func (c Action) IsInplaceSupported() bool { + return false +} + +type ActionType string + +const ( + ActionTypeCreate ActionType = "create" + ActionTypeDelete ActionType = "delete" + ActionTypeUpdate ActionType = "update" + ActionTypeNoOp ActionType = "no-op" + ActionTypeRead ActionType = "read" + ActionTypeRecreate ActionType = "recreate" +) From c9d5eb04d1bb0de1104ba0ab14de57261845aacb Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 19:38:31 +0200 Subject: [PATCH 12/20] remove unnecessary flag --- bundle/bundle.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index d5f6ba8a566..032d98abc85 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -69,11 +69,6 @@ type Bundle struct { // files AutoApprove bool - // If true, we require user approval to deploy. This is - // TODO: On both destroy and deploy, error with suggesting `--auto-approve` - // if operating from a non-tty. - RequireApprovalForDeploy bool - // Tagging is used to normalize tag keys and values. // The implementation depends on the cloud being targeted. Tagging tags.Cloud From 4f15fe9d38fb6f26598b2a1bb0aeaa685d198a1f Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 20:01:40 +0200 Subject: [PATCH 13/20] - --- bundle/deploy/terraform/destroy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/deploy/terraform/destroy.go b/bundle/deploy/terraform/destroy.go index cfab2a8add7..1eed99934e2 100644 --- a/bundle/deploy/terraform/destroy.go +++ b/bundle/deploy/terraform/destroy.go @@ -19,7 +19,7 @@ func (w *destroy) Name() string { func (w *destroy) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { // return early if plan is empty if b.Plan.IsEmpty { - cmdio.LogString(ctx, "No resources to destroy`") + cmdio.LogString(ctx, "No resources to destroy") return nil } From 82fe4337fda827dcd8e7413acc34e0d1bd3b0ad0 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Tue, 9 Jul 2024 20:02:43 +0200 Subject: [PATCH 14/20] - --- bundle/phases/destroy.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index 0c3a2eca358..b8960c6932b 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -13,8 +13,6 @@ import ( "github.com/databricks/cli/libs/cmdio" - // TODO: Rename the module itself? - "github.com/databricks/cli/libs/log" terraformlib "github.com/databricks/cli/libs/terraform" "github.com/databricks/databricks-sdk-go/apierr" From 0231e0da4439123ed254ef2befd305f6e6f40d8a Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 15:56:18 +0200 Subject: [PATCH 15/20] remove logs --- bundle/deploy/files/delete.go | 3 --- bundle/deploy/terraform/destroy.go | 6 ------ bundle/deploy/terraform/plan.go | 2 -- 3 files changed, 11 deletions(-) diff --git a/bundle/deploy/files/delete.go b/bundle/deploy/files/delete.go index 7ae407e4954..bb28c2722ca 100644 --- a/bundle/deploy/files/delete.go +++ b/bundle/deploy/files/delete.go @@ -10,7 +10,6 @@ import ( "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/cli/libs/sync" "github.com/databricks/databricks-sdk-go/service/workspace" ) @@ -37,8 +36,6 @@ func (m *delete) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { if err != nil { return diag.FromErr(err) } - - log.Debugf(ctx, "Successfully deleted files!") return nil } diff --git a/bundle/deploy/terraform/destroy.go b/bundle/deploy/terraform/destroy.go index 1eed99934e2..0fad190f631 100644 --- a/bundle/deploy/terraform/destroy.go +++ b/bundle/deploy/terraform/destroy.go @@ -6,7 +6,6 @@ import ( "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/hashicorp/terraform-exec/tfexec" ) @@ -19,7 +18,6 @@ func (w *destroy) Name() string { func (w *destroy) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { // return early if plan is empty if b.Plan.IsEmpty { - cmdio.LogString(ctx, "No resources to destroy") return nil } @@ -32,15 +30,11 @@ func (w *destroy) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics return diag.Errorf("no plan found") } - log.Debugf(ctx, "Starting to destroy resources") - // Apply terraform according to the computed destroy plan err := tf.Apply(ctx, tfexec.DirOrPlan(b.Plan.Path)) if err != nil { return diag.Errorf("terraform destroy: %v", err) } - - log.Debugf(ctx, "Successfully destroyed resources!") return nil } diff --git a/bundle/deploy/terraform/plan.go b/bundle/deploy/terraform/plan.go index d01c6abab51..72f0b49a89d 100644 --- a/bundle/deploy/terraform/plan.go +++ b/bundle/deploy/terraform/plan.go @@ -33,8 +33,6 @@ func (p *plan) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { return diag.Errorf("terraform not initialized") } - log.Debugf(ctx, "Starting plan computation") - err := tf.Init(ctx, tfexec.Upgrade(true)) if err != nil { return diag.Errorf("terraform init: %v", err) From d25d9048b0126af47963697bbc9f728152e65bcd Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 16:18:10 +0200 Subject: [PATCH 16/20] add link to action enum: --- libs/terraform/plan.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/terraform/plan.go b/libs/terraform/plan.go index 476b8fd54bb..c5597a064ce 100644 --- a/libs/terraform/plan.go +++ b/libs/terraform/plan.go @@ -28,6 +28,9 @@ func (c Action) IsInplaceSupported() bool { return false } +// These enum values correspond to action types defined in the tfjson library. +// "recreate" maps to the tfjson.Action.Replace() function. source: +// https://github.com/hashicorp/terraform-json/blob/0104004301ca8e7046d089cdc2e2db2179d225be/action.go#L14 type ActionType string const ( From fb02ea69b5d2777906d39aedb38762f4992a3a09 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 16:19:36 +0200 Subject: [PATCH 17/20] - --- libs/terraform/plan.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libs/terraform/plan.go b/libs/terraform/plan.go index c5597a064ce..36383cc248f 100644 --- a/libs/terraform/plan.go +++ b/libs/terraform/plan.go @@ -29,7 +29,8 @@ func (c Action) IsInplaceSupported() bool { } // These enum values correspond to action types defined in the tfjson library. -// "recreate" maps to the tfjson.Action.Replace() function. source: +// "recreate" maps to the tfjson.Actions.Replace() function. +// "update" maps to tfjson.Actions.Update() and so on. source: // https://github.com/hashicorp/terraform-json/blob/0104004301ca8e7046d089cdc2e2db2179d225be/action.go#L14 type ActionType string From c49e685d24aa74a6df4372efe0fe16a1cdd073e4 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 17 Jul 2024 16:20:48 +0200 Subject: [PATCH 18/20] fix build --- bundle/deploy/terraform/destroy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/bundle/deploy/terraform/destroy.go b/bundle/deploy/terraform/destroy.go index 0fad190f631..2db4c59b5c2 100644 --- a/bundle/deploy/terraform/destroy.go +++ b/bundle/deploy/terraform/destroy.go @@ -4,7 +4,6 @@ import ( "context" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/diag" "github.com/hashicorp/terraform-exec/tfexec" ) From 64baad99bb1bc3f8aacdc3f914af584efcb7cdad Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 24 Jul 2024 13:01:30 +0200 Subject: [PATCH 19/20] debug statement when there are no resources to destroy --- bundle/deploy/terraform/destroy.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bundle/deploy/terraform/destroy.go b/bundle/deploy/terraform/destroy.go index 2db4c59b5c2..9c63a0b3791 100644 --- a/bundle/deploy/terraform/destroy.go +++ b/bundle/deploy/terraform/destroy.go @@ -5,6 +5,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/log" "github.com/hashicorp/terraform-exec/tfexec" ) @@ -17,6 +18,7 @@ func (w *destroy) Name() string { func (w *destroy) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { // return early if plan is empty if b.Plan.IsEmpty { + log.Debugf(ctx, "No resources to destroy in plan. Skipping destroy.") return nil } From 9167e15f31be2adcbf4c2a33d17a482c793275a2 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 24 Jul 2024 13:03:38 +0200 Subject: [PATCH 20/20] use enum value --- bundle/phases/destroy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index b8960c6932b..bd99af789b9 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -47,7 +47,7 @@ func approvalForDestroy(ctx context.Context, b *bundle.Bundle) (bool, error) { for _, rc := range plan.ResourceChanges { if rc.Change.Actions.Delete() { deleteActions = append(deleteActions, terraformlib.Action{ - Action: "delete", + Action: terraformlib.ActionTypeDelete, ResourceType: rc.Type, ResourceName: rc.Name, })