-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add prompt when a pipeline recreation or deletion happens #1672
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
105a3df
Add prompt when a pipeline recreation happens
shreyas-goenka d270727
grammer nit
shreyas-goenka 27af70e
rename
shreyas-goenka a0bc111
Merge remote-tracking branch 'origin' into prompt-on-pipeline-recreate
shreyas-goenka d6c564d
add the why
shreyas-goenka 7e3a453
fix test
shreyas-goenka d9ed8eb
add multiline message
shreyas-goenka 8d2c2ab
Merge remote-tracking branch 'origin' into prompt-on-pipeline-recreate
shreyas-goenka 2f7fa97
add test for pipeline delete as well
shreyas-goenka ec413af
Merge remote-tracking branch 'origin' into prompt-on-pipeline-recreate
shreyas-goenka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,55 +19,95 @@ import ( | |
"github.com/databricks/cli/bundle/scripts" | ||
"github.com/databricks/cli/libs/cmdio" | ||
terraformlib "github.com/databricks/cli/libs/terraform" | ||
tfjson "github.com/hashicorp/terraform-json" | ||
) | ||
|
||
func approvalForUcSchemaDelete(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 | ||
} | ||
|
||
actions := make([]terraformlib.Action, 0) | ||
for _, rc := range plan.ResourceChanges { | ||
// We only care about destructive actions on UC schema resources. | ||
if rc.Type != "databricks_schema" { | ||
func parseTerraformActions(changes []*tfjson.ResourceChange, toInclude func(typ string, actions tfjson.Actions) bool) []terraformlib.Action { | ||
res := make([]terraformlib.Action, 0) | ||
for _, rc := range changes { | ||
if !toInclude(rc.Type, rc.Change.Actions) { | ||
continue | ||
} | ||
|
||
var actionType terraformlib.ActionType | ||
|
||
switch { | ||
case rc.Change.Actions.Delete(): | ||
actionType = terraformlib.ActionTypeDelete | ||
case rc.Change.Actions.Replace(): | ||
actionType = terraformlib.ActionTypeRecreate | ||
default: | ||
// We don't need a prompt for non-destructive actions like creating | ||
// or updating a schema. | ||
// No use case for other action types yet. | ||
continue | ||
} | ||
|
||
actions = append(actions, terraformlib.Action{ | ||
res = append(res, terraformlib.Action{ | ||
Action: actionType, | ||
ResourceType: rc.Type, | ||
ResourceName: rc.Name, | ||
}) | ||
} | ||
|
||
// No restricted actions planned. No need for approval. | ||
if len(actions) == 0 { | ||
return res | ||
} | ||
|
||
func approvalForDeploy(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 | ||
} | ||
|
||
schemaActions := parseTerraformActions(plan.ResourceChanges, func(typ string, actions tfjson.Actions) bool { | ||
// Filter in only UC schema resources. | ||
if typ != "databricks_schema" { | ||
return false | ||
} | ||
|
||
// We only display prompts for destructive actions like deleting or | ||
// recreating a schema. | ||
return actions.Delete() || actions.Replace() | ||
}) | ||
|
||
dltActions := parseTerraformActions(plan.ResourceChanges, func(typ string, actions tfjson.Actions) bool { | ||
// Filter in only DLT pipeline resources. | ||
if typ != "databricks_pipeline" { | ||
return false | ||
} | ||
|
||
// Recreating DLT pipeline leads to metadata loss and for a transient period | ||
// the underling tables will be unavailable. | ||
return actions.Replace() || actions.Delete() | ||
}) | ||
|
||
// We don't need to display any prompts in this case. | ||
if len(dltActions) == 0 && len(schemaActions) == 0 { | ||
return true, nil | ||
} | ||
|
||
cmdio.LogString(ctx, "The following UC schemas will be deleted or recreated. Any underlying data may be lost:") | ||
for _, action := range actions { | ||
cmdio.Log(ctx, action) | ||
// One or more UC schema resources will be deleted or recreated. | ||
if len(schemaActions) != 0 { | ||
cmdio.LogString(ctx, "The following UC schemas will be deleted or recreated. Any underlying data may be lost:") | ||
for _, action := range schemaActions { | ||
cmdio.Log(ctx, action) | ||
} | ||
} | ||
|
||
// One or more DLT pipelines is being recreated. | ||
if len(dltActions) != 0 { | ||
msg := ` | ||
This action will result in the deletion or recreation of the following DLT Pipelines along with the | ||
Streaming Tables (STs) and Materialized Views (MVs) managed by them. Recreating the Pipelines will | ||
restore the defined STs and MVs through full refresh. Note that recreation is necessary when pipeline | ||
properties such as the 'catalog' or 'storage' are changed:` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The note about recreation should only be shown on actual recreation. Can be a follow-up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. Will do. |
||
cmdio.LogString(ctx, msg) | ||
for _, action := range dltActions { | ||
cmdio.Log(ctx, action) | ||
} | ||
} | ||
|
||
if b.AutoApprove { | ||
|
@@ -126,7 +166,7 @@ func Deploy() bundle.Mutator { | |
terraform.CheckRunningResource(), | ||
terraform.Plan(terraform.PlanGoal("deploy")), | ||
bundle.If( | ||
approvalForUcSchemaDelete, | ||
approvalForDeploy, | ||
deployCore, | ||
bundle.LogString("Deployment cancelled!"), | ||
), | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
package phases | ||
|
||
import ( | ||
"testing" | ||
|
||
terraformlib "github.com/databricks/cli/libs/terraform" | ||
tfjson "github.com/hashicorp/terraform-json" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestParseTerraformActions(t *testing.T) { | ||
changes := []*tfjson.ResourceChange{ | ||
{ | ||
Type: "databricks_pipeline", | ||
Change: &tfjson.Change{ | ||
Actions: tfjson.Actions{tfjson.ActionCreate}, | ||
}, | ||
Name: "create pipeline", | ||
}, | ||
{ | ||
Type: "databricks_pipeline", | ||
Change: &tfjson.Change{ | ||
Actions: tfjson.Actions{tfjson.ActionDelete}, | ||
}, | ||
Name: "delete pipeline", | ||
}, | ||
{ | ||
Type: "databricks_pipeline", | ||
Change: &tfjson.Change{ | ||
Actions: tfjson.Actions{tfjson.ActionDelete, tfjson.ActionCreate}, | ||
}, | ||
Name: "recreate pipeline", | ||
}, | ||
{ | ||
Type: "databricks_whatever", | ||
Change: &tfjson.Change{ | ||
Actions: tfjson.Actions{tfjson.ActionDelete, tfjson.ActionCreate}, | ||
}, | ||
Name: "recreate whatever", | ||
}, | ||
} | ||
|
||
res := parseTerraformActions(changes, func(typ string, actions tfjson.Actions) bool { | ||
if typ != "databricks_pipeline" { | ||
return false | ||
} | ||
|
||
if actions.Delete() || actions.Replace() { | ||
return true | ||
} | ||
|
||
return false | ||
}) | ||
|
||
assert.Equal(t, []terraformlib.Action{ | ||
{ | ||
Action: terraformlib.ActionTypeDelete, | ||
ResourceType: "databricks_pipeline", | ||
ResourceName: "delete pipeline", | ||
}, | ||
{ | ||
Action: terraformlib.ActionTypeRecreate, | ||
ResourceType: "databricks_pipeline", | ||
ResourceName: "recreate pipeline", | ||
}, | ||
}, res) | ||
} |
8 changes: 8 additions & 0 deletions
8
internal/bundle/bundles/recreate_pipeline/databricks_template_schema.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"properties": { | ||
"unique_id": { | ||
"type": "string", | ||
"description": "Unique ID for the schema and pipeline names" | ||
} | ||
} | ||
} |
25 changes: 25 additions & 0 deletions
25
internal/bundle/bundles/recreate_pipeline/template/databricks.yml.tmpl
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
bundle: | ||
name: "bundle-playground" | ||
|
||
variables: | ||
catalog: | ||
description: The catalog the DLT pipeline should use. | ||
default: main | ||
|
||
|
||
resources: | ||
pipelines: | ||
foo: | ||
name: test-pipeline-{{.unique_id}} | ||
libraries: | ||
- notebook: | ||
path: ./nb.sql | ||
development: true | ||
catalog: ${var.catalog} | ||
|
||
include: | ||
- "*.yml" | ||
|
||
targets: | ||
development: | ||
default: true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
-- Databricks notebook source | ||
select 1 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this always true per the switch/case on the action type in
parseTerraformActions
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct. This structure is from when this PR was just for recreates of DLT. I can clean this up in a followup.