Skip to content

Commit

Permalink
Only suppress diff for run_if when it is ALL_SUCCESS (#3454)
Browse files Browse the repository at this point in the history 
* Do not suppress diff for run_if if it's not the default value

* add support for for_each_task as well

* abstract default suppress diff into a common interface

* add test cases

* Update common/customizable_schema.go

---------

Co-authored-by: Miles Yucht <[email protected]>
  • Loading branch information
@shreyas-goenka @mgyucht
shreyas-goenka and mgyucht authored Apr 12, 2024
1 parent 3c21b25 commit 1bf97ef
Show file tree
Hide file tree
Showing 4 changed files with 309 additions and 2 deletions.
33 changes: 33 additions & 0 deletions common/customizable_schema.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package common

import (
"fmt"
"slices"

"github.com/hashicorp/go-cty/cty"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -87,6 +90,36 @@ func (s *CustomizableSchema) SetSuppressDiff() *CustomizableSchema {
return s
}

// SetSuppressDiffWithDefault suppresses the diff if the
// new value (ie value from HCL config) is not set and
// the old value (ie value from state / platform) is equal to the default value.
//
// Often Databricks HTTP APIs will return values for fields that were not set by
// the author in their terraform configuration. This function allows us to suppress
// the diff in these cases.
func (s *CustomizableSchema) SetSuppressDiffWithDefault(dv any) *CustomizableSchema {
primitiveTypes := []schema.ValueType{schema.TypeBool, schema.TypeString, schema.TypeInt, schema.TypeFloat}
if !slices.Contains(primitiveTypes, s.Schema.Type) {
panic(fmt.Errorf("expected primitive type, got: %s", s.Schema.Type))
}

// Get zero value for the schema type
zero := fmt.Sprintf("%v", s.Schema.Type.Zero())

// Get string representation of the default value
sv := fmt.Sprintf("%v", dv)

// Suppress diff if the new value (ie value from HCL config) is not set and
// the old value (ie value from state / platform) is equal to the default value.
s.Schema.DiffSuppressFunc = func(k, old, new string, d *schema.ResourceData) bool {
if new == zero && old == sv {
return true
}
return false
}
return s
}

func (s *CustomizableSchema) SetCustomSuppressDiff(suppressor func(k, old, new string, d *schema.ResourceData) bool) *CustomizableSchema {
s.Schema.DiffSuppressFunc = suppressor
return s
Expand Down
93 changes: 93 additions & 0 deletions common/customizable_schema_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,99 @@ func TestCustomizableSchemaSetSuppressDiff(t *testing.T) {
assert.Truef(t, testCustomizableSchemaScm["non_optional"].DiffSuppressFunc != nil, "DiffSuppressfunc should be set in field: non_optional")
}

func TestCustomizableSchemaSetCustomSuppressDiffWithDefault(t *testing.T) {
tc := []struct {
name string
fieldName string
dv any
old string
new string
result bool
}{
{
name: "default string",
fieldName: "string",
dv: "foobar",
old: "foobar",
new: "",
result: true,
},
{
name: "default int",
fieldName: "integer",
dv: 123,
old: "123",
new: "0",
result: true,
},
{
name: "default bool",
fieldName: "bool",
dv: true,
old: "true",
new: "false",
result: true,
},
{
name: "default float",
fieldName: "float",
dv: 123.456,
old: "123.456",
new: "0",
result: true,
},
{
name: "non default string",
fieldName: "string",
dv: "foobar",
old: "non-default-val",
new: "",
result: false,
},
{
name: "non default int",
fieldName: "integer",
dv: 123,
old: "non-default-val",
new: "0",
result: false,
},
{
name: "non default bool",
fieldName: "bool",
dv: true,
old: "non-default-val",
new: "false",
result: false,
},
{
name: "non default float",
fieldName: "float",
dv: 123.456,
old: "non-default-val",
new: "0",
result: false,
},
{
name: "override in config",
fieldName: "string",
dv: "foobar",
old: "foobar",
new: "new-val-in-config",
result: false,
},
}

for _, tt := range tc {
t.Run(tt.name, func(t *testing.T) {
CustomizeSchemaPath(testCustomizableSchemaScm, tt.fieldName).SetSuppressDiffWithDefault(tt.dv)
assert.Truef(t, testCustomizableSchemaScm[tt.fieldName].DiffSuppressFunc != nil, "DiffSuppressFunc should be set in field: %s", tt.fieldName)

assert.Equal(t, tt.result, testCustomizableSchemaScm[tt.fieldName].DiffSuppressFunc("", tt.old, tt.new, nil))
})
}
}

func TestCustomizableSchemaSetCustomSuppressDiff(t *testing.T) {
CustomizeSchemaPath(testCustomizableSchemaScm, "non_optional").SetCustomSuppressDiff(diffSuppressor("test", CustomizeSchemaPath(testCustomizableSchemaScm, "non_optional").Schema))
assert.Truef(t, testCustomizableSchemaScm["non_optional"].DiffSuppressFunc != nil, "DiffSuppressfunc should be set in field: non_optional")
Expand Down
8 changes: 6 additions & 2 deletions jobs/resource_job.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ type ForEachNestedTask struct {
TaskKey string `json:"task_key,omitempty"`
Description string `json:"description,omitempty"`
DependsOn []jobs.TaskDependency `json:"depends_on,omitempty"`
RunIf string `json:"run_if,omitempty" tf:"suppress_diff"`
RunIf string `json:"run_if,omitempty"`

ExistingClusterID string `json:"existing_cluster_id,omitempty" tf:"group:cluster_type"`
NewCluster *clusters.Cluster `json:"new_cluster,omitempty" tf:"group:cluster_type"`
Expand Down Expand Up @@ -211,7 +211,7 @@ type JobTaskSettings struct {
TaskKey string `json:"task_key,omitempty"`
Description string `json:"description,omitempty"`
DependsOn []jobs.TaskDependency `json:"depends_on,omitempty"`
RunIf string `json:"run_if,omitempty" tf:"suppress_diff"`
RunIf string `json:"run_if,omitempty"`

ExistingClusterID string `json:"existing_cluster_id,omitempty" tf:"group:cluster_type"`
NewCluster *clusters.Cluster `json:"new_cluster,omitempty" tf:"group:cluster_type"`
Expand Down Expand Up @@ -766,6 +766,10 @@ var jobSchema = common.StructToSchema(JobSettings{},
fixWebhookNotifications(s)
fixWebhookNotifications(common.MustSchemaMap(s, "task"))

// Suppress diff if the platform returns ALL_SUCCESS for run_if in a task
common.CustomizeSchemaPath(s, "task", "run_if").SetSuppressDiffWithDefault(jobs.RunIfAllSuccess)
common.CustomizeSchemaPath(s, "task", "for_each_task", "task", "run_if").SetSuppressDiffWithDefault(jobs.RunIfAllSuccess)

return s
})

Expand Down
177 changes: 177 additions & 0 deletions jobs/resource_job_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2038,6 +2038,183 @@ func TestResourceJobUpdate(t *testing.T) {
assert.Equal(t, "Featurizer New", d.Get("name"))
}

func TestResourceJobUpdate_RunIfSuppressesDiffIfAllSuccess(t *testing.T) {
_, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "POST",
Resource: "/api/2.1/jobs/reset",
ExpectedRequest: UpdateJobRequest{
JobID: 789,
NewSettings: &JobSettings{
MaxConcurrentRuns: 1,
Tasks: []JobTaskSettings{
{
NotebookTask: &NotebookTask{
NotebookPath: "/foo/bar",
},
// The diff is suppressed here. The API payload
// contains the "run_if" value from the terraform
// state.
RunIf: "ALL_SUCCESS",
},
{
ForEachTask: &ForEachTask{
Inputs: "abc",
Task: ForEachNestedTask{
NotebookTask: &NotebookTask{NotebookPath: "/bar/foo"},
// The diff is suppressed here. Value is from
// the terraform state.
RunIf: "ALL_SUCCESS",
},
},
},
},
Name: "My job",
},
},
},
{
Method: "GET",
Resource: "/api/2.1/jobs/get?job_id=789",
Response: Job{
JobID: 789,
Settings: &JobSettings{
Name: "My job",
Tasks: []JobTaskSettings{
{
NotebookTask: &NotebookTask{NotebookPath: "/foo/bar"},
RunIf: "ALL_SUCCESS",
},
{
ForEachTask: &ForEachTask{
Inputs: "abc",
Task: ForEachNestedTask{
NotebookTask: &NotebookTask{NotebookPath: "/bar/foo"},
RunIf: "ALL_SUCCESS",
},
},
},
},
},
},
},
},
ID: "789",
Update: true,
Resource: ResourceJob(),
InstanceState: map[string]string{
"task.0.run_if": "ALL_SUCCESS",
"task.1.for_each_task.0.task.0.run_if": "ALL_SUCCESS",
},
HCL: `
name = "My job"
task {
notebook_task {
notebook_path = "/foo/bar"
}
}
task {
for_each_task {
inputs = "abc"
task {
notebook_task {
notebook_path = "/bar/foo"
}
}
}
}`,
}.Apply(t)
assert.NoError(t, err)
}

func TestResourceJobUpdate_RunIfDoesNotSuppressIfNotAllSuccess(t *testing.T) {
_, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: "POST",
Resource: "/api/2.1/jobs/reset",
ExpectedRequest: UpdateJobRequest{
JobID: 789,
NewSettings: &JobSettings{
MaxConcurrentRuns: 1,
Tasks: []JobTaskSettings{
{
NotebookTask: &NotebookTask{
NotebookPath: "/foo/bar",
},
// The diff is not suppressed here. Thus the API payload
// explicitly does not set run_if here, to unset it in the
// job definition.
// RunIf is not set, as implied from the HCL config.
},
{
ForEachTask: &ForEachTask{
Inputs: "abc",
Task: ForEachNestedTask{
NotebookTask: &NotebookTask{NotebookPath: "/bar/foo"},
// The diff is not suppressed. RunIf is
// not set, as implied from the HCL config.
},
},
},
},
Name: "My job",
},
},
},
{
Method: "GET",
Resource: "/api/2.1/jobs/get?job_id=789",
Response: Job{
JobID: 789,
Settings: &JobSettings{
Name: "My job",
Tasks: []JobTaskSettings{
{
NotebookTask: &NotebookTask{NotebookPath: "/foo/bar"},
RunIf: "AT_LEAST_ONE_FAILED",
},
{
ForEachTask: &ForEachTask{
Task: ForEachNestedTask{
RunIf: "AT_LEAST_ONE_FAILED",
},
},
},
},
},
},
},
},
ID: "789",
Update: true,
InstanceState: map[string]string{
"task.0.run_if": "AT_LEAST_ONE_FAILED",
"task.1.for_each_task.0.task.0.run_if": "AT_LEAST_ONE_FAILED",
},
Resource: ResourceJob(),
HCL: `
name = "My job"
task {
notebook_task {
notebook_path = "/foo/bar"
}
}
task {
for_each_task {
inputs = "abc"
task {
notebook_task {
notebook_path = "/bar/foo"
}
}
}
}`,
}.Apply(t)
assert.NoError(t, err)
}

func TestResourceJobUpdate_NodeTypeToInstancePool(t *testing.T) {
d, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
Expand Down

0 comments on commit 1bf97ef

Please sign in to comment.