Skip to content

Commit 602bff9

Browse files
pieternmgyucht
pietern
and
mgyucht
authored
Fix webhook_notifications diff suppression for databricks_job (#3309)
* Demonstrate diff suppression issue * work * only suppress current field diff * suppress_diff -> comptued * marking as computd * minimal fix * work * integration test * "integration test" --------- Co-authored-by: Miles Yucht <[email protected]>
1 parent 689cc85 commit 602bff9

File tree

8 files changed

+140
-37
lines changed

8 files changed

+140
-37
lines changed

‎common/customizable_schema.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88

99
type CustomizableSchema struct {
1010
Schema *schema.Schema
11+
path []string
1112
isSuppressDiff bool
1213
}
1314

@@ -23,7 +24,7 @@ func CustomizeSchemaPath(s map[string]*schema.Schema, path ...string) *Customiza
2324
return &CustomizableSchema{Schema: wrappedSch}
2425
}
2526
sch := MustSchemaPath(s, path...)
26-
return &CustomizableSchema{Schema: sch}
27+
return &CustomizableSchema{Schema: sch, path: path}
2728
}
2829

2930
func (s *CustomizableSchema) SetOptional() *CustomizableSchema {
@@ -64,7 +65,7 @@ func (s *CustomizableSchema) SetRequired() *CustomizableSchema {
6465
}
6566

6667
func (s *CustomizableSchema) SetSuppressDiff() *CustomizableSchema {
67-
s.Schema.DiffSuppressFunc = diffSuppressor(s.Schema)
68+
s.Schema.DiffSuppressFunc = diffSuppressor(s.path[len(s.path)-1], s.Schema)
6869
s.isSuppressDiff = true
6970
if s.Schema.Type == schema.TypeList && s.Schema.MaxItems == 1 {
7071
// If it is a list with max items = 1, it means the corresponding sdk schema type is a struct or a ptr.
@@ -74,8 +75,8 @@ func (s *CustomizableSchema) SetSuppressDiff() *CustomizableSchema {
7475
panic("Cannot cast Elem into Resource type.")
7576
}
7677
nestedSchema := resource.Schema
77-
for _, v := range nestedSchema {
78-
v.DiffSuppressFunc = diffSuppressor(v)
78+
for k, v := range nestedSchema {
79+
v.DiffSuppressFunc = diffSuppressor(k, v)
7980
}
8081
}
8182
return s
@@ -164,7 +165,7 @@ func (s *CustomizableSchema) AddNewField(key string, newField *schema.Schema) *C
164165
}
165166
cv.Schema[key] = newField
166167
if s.isSuppressDiff {
167-
newField.DiffSuppressFunc = diffSuppressor(newField)
168+
newField.DiffSuppressFunc = diffSuppressor(key, newField)
168169
}
169170
return s
170171
}

‎common/customizable_schema_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func TestCustomizableSchemaSetSuppressDiff(t *testing.T) {
4444
}
4545

4646
func TestCustomizableSchemaSetCustomSuppressDiff(t *testing.T) {
47-
CustomizeSchemaPath(testCustomizableSchemaScm, "non_optional").SetCustomSuppressDiff(diffSuppressor(CustomizeSchemaPath(testCustomizableSchemaScm, "non_optional").Schema))
47+
CustomizeSchemaPath(testCustomizableSchemaScm, "non_optional").SetCustomSuppressDiff(diffSuppressor("test", CustomizeSchemaPath(testCustomizableSchemaScm, "non_optional").Schema))
4848
assert.Truef(t, testCustomizableSchemaScm["non_optional"].DiffSuppressFunc != nil, "DiffSuppressfunc should be set in field: non_optional")
4949
}
5050

‎common/reflect_resource.go‎

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -133,13 +133,6 @@ func StructToSchema(v any, customize func(map[string]*schema.Schema) map[string]
133133
return scm
134134
}
135135

136-
// SetSuppressDiff adds diff suppression to a schema. This is necessary for non-computed
137-
// fields for which the platform returns a value, but the user has not configured any value.
138-
// For example: the REST API returns `{"tags": {}}` for a resource with no tags.
139-
func SetSuppressDiff(v *schema.Schema) {
140-
v.DiffSuppressFunc = diffSuppressor(v)
141-
}
142-
143136
// SetDefault sets the default value for a schema.
144137
func SetDefault(v *schema.Schema, value any) {
145138
v.Default = value
@@ -216,11 +209,11 @@ func handleSensitive(typeField reflect.StructField, schema *schema.Schema) {
216209
}
217210
}
218211

219-
func handleSuppressDiff(typeField reflect.StructField, v *schema.Schema) {
212+
func handleSuppressDiff(typeField reflect.StructField, fieldName string, v *schema.Schema) {
220213
tfTags := strings.Split(typeField.Tag.Get("tf"), ",")
221214
for _, tag := range tfTags {
222215
if tag == "suppress_diff" {
223-
v.DiffSuppressFunc = diffSuppressor(v)
216+
v.DiffSuppressFunc = diffSuppressor(fieldName, v)
224217
break
225218
}
226219
}
@@ -244,16 +237,18 @@ func chooseFieldName(typeField reflect.StructField) string {
244237
return getJsonFieldName(typeField)
245238
}
246239

247-
func diffSuppressor(v *schema.Schema) func(k, old, new string, d *schema.ResourceData) bool {
240+
func diffSuppressor(fieldName string, v *schema.Schema) func(k, old, new string, d *schema.ResourceData) bool {
248241
zero := fmt.Sprintf("%v", v.Type.Zero())
249242
return func(k, old, new string, d *schema.ResourceData) bool {
250243
if new == zero && old != zero {
251244
log.Printf("[DEBUG] Suppressing diff for %v: platform=%#v config=%#v", k, old, new)
252245
return true
253246
}
254-
if strings.HasSuffix(k, ".#") && new == "0" && old != "0" {
255-
field := strings.TrimSuffix(k, ".#")
256-
log.Printf("[DEBUG] Suppressing diff for list or set %v: no value configured but platform returned some value (likely {})", field)
247+
// When suppressing diffs for a list of attributes, SuppressDiffFunc is called for each diff
248+
// recursively. To verify that the list is empty, we need to ensure that the key being diffed
249+
// is exactly the list's attribute itself and not one of its elements.
250+
if strings.HasSuffix(k, fmt.Sprintf("%s.#", fieldName)) && new == "0" && old != "0" {
251+
log.Printf("[DEBUG] Suppressing diff for list or set %v: no value configured but platform returned some value (likely {})", fieldName)
257252
return true
258253
}
259254
return false
@@ -334,17 +329,17 @@ func typeToSchema(v reflect.Value, aliases map[string]string) map[string]*schema
334329
case reflect.Int, reflect.Int32, reflect.Int64:
335330
scm[fieldName].Type = schema.TypeInt
336331
// diff suppression needs type for zero value
337-
handleSuppressDiff(typeField, scm[fieldName])
332+
handleSuppressDiff(typeField, fieldName, scm[fieldName])
338333
case reflect.Float64:
339334
scm[fieldName].Type = schema.TypeFloat
340335
// diff suppression needs type for zero value
341-
handleSuppressDiff(typeField, scm[fieldName])
336+
handleSuppressDiff(typeField, fieldName, scm[fieldName])
342337
case reflect.Bool:
343338
scm[fieldName].Type = schema.TypeBool
344339
case reflect.String:
345340
scm[fieldName].Type = schema.TypeString
346341
// diff suppression needs type for zero value
347-
handleSuppressDiff(typeField, scm[fieldName])
342+
handleSuppressDiff(typeField, fieldName, scm[fieldName])
348343
case reflect.Map:
349344
scm[fieldName].Type = schema.TypeMap
350345
elem := typeField.Type.Elem()
@@ -363,10 +358,9 @@ func typeToSchema(v reflect.Value, aliases map[string]string) map[string]*schema
363358
sv := reflect.New(elem).Elem()
364359
nestedSchema := typeToSchema(sv, unwrappedAliases)
365360
if strings.Contains(tfTag, "suppress_diff") {
366-
scm[fieldName].DiffSuppressFunc = diffSuppressor(scm[fieldName])
367-
for _, v := range nestedSchema {
368-
// to those relatively new to GoLang: we must explicitly pass down v by copy
369-
v.DiffSuppressFunc = diffSuppressor(v)
361+
scm[fieldName].DiffSuppressFunc = diffSuppressor(fieldName, scm[fieldName])
362+
for k, v := range nestedSchema {
363+
v.DiffSuppressFunc = diffSuppressor(k, v)
370364
}
371365
}
372366
scm[fieldName].Elem = &schema.Resource{
@@ -381,10 +375,9 @@ func typeToSchema(v reflect.Value, aliases map[string]string) map[string]*schema
381375

382376
nestedSchema := typeToSchema(sv, unwrappedAliases)
383377
if strings.Contains(tfTag, "suppress_diff") {
384-
scm[fieldName].DiffSuppressFunc = diffSuppressor(scm[fieldName])
385-
for _, v := range nestedSchema {
386-
// to those relatively new to GoLang: we must explicitly pass down v by copy
387-
v.DiffSuppressFunc = diffSuppressor(v)
378+
scm[fieldName].DiffSuppressFunc = diffSuppressor(fieldName, scm[fieldName])
379+
for k, v := range nestedSchema {
380+
v.DiffSuppressFunc = diffSuppressor(k, v)
388381
}
389382
}
390383
scm[fieldName].Elem = &schema.Resource{

‎common/reflect_resource_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -565,7 +565,7 @@ func TestDiffSuppressor(t *testing.T) {
565565
stringSchema := &schema.Schema{
566566
Type: schema.TypeString,
567567
}
568-
dsf := diffSuppressor(stringSchema)
568+
dsf := diffSuppressor("foo", stringSchema)
569569
d := schema.TestResourceDataRaw(t, map[string]*schema.Schema{
570570
"foo": {
571571
Type: schema.TypeBool,

‎internal/acceptance/job_test.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,3 +313,22 @@ func TestAccJobRunAsMutations(t *testing.T) {
313313
},
314314
)
315315
}
316+
317+
func TestRemoveWebhooks(t *testing.T) {
318+
// skipf(t)("There is no API to create notification destinations. Once available, add here and enable this test.")
319+
workspaceLevel(t, step{
320+
Template: `
321+
resource databricks_job test {
322+
webhook_notifications {
323+
on_success {
324+
id = "a90cc1be-a29e-4eb7-a7e9-e4b0d4a7e7ae"
325+
}
326+
}
327+
}
328+
`,
329+
}, step{
330+
Template: `
331+
resource databricks_job test {}
332+
`,
333+
})
334+
}

‎jobs/resource_job.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -752,6 +752,11 @@ var jobSchema = common.StructToSchema(JobSettings{},
752752
common.MustSchemaPath(s, "run_as", "user_name").ExactlyOneOf = run_as_eoo
753753
common.MustSchemaPath(s, "run_as", "service_principal_name").ExactlyOneOf = run_as_eoo
754754

755+
// Clear the implied diff suppression for the webhook notification lists
756+
for _, n := range []string{"on_start", "on_failure", "on_success", "on_duration_warning_threshold_exceeded"} {
757+
common.MustSchemaPath(s, "webhook_notifications", n).DiffSuppressFunc = nil
758+
}
759+
755760
return s
756761
})
757762

‎jobs/resource_job_webhook_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
package jobs
2+
3+
import (
4+
"testing"
5+
6+
"github.com/databricks/databricks-sdk-go/service/jobs"
7+
"github.com/databricks/terraform-provider-databricks/qa"
8+
)
9+
10+
func TestResourceJobUpdate_WebhookNotifications(t *testing.T) {
11+
qa.ResourceFixture{
12+
Fixtures: []qa.HTTPFixture{
13+
{
14+
Method: "POST",
15+
Resource: "/api/2.1/jobs/reset",
16+
ExpectedRequest: UpdateJobRequest{
17+
JobID: 789,
18+
NewSettings: &JobSettings{
19+
Name: "Webhook test",
20+
Tasks: []JobTaskSettings{
21+
{
22+
ExistingClusterID: "abc",
23+
},
24+
},
25+
WebhookNotifications: &jobs.WebhookNotifications{
26+
OnSuccess: []jobs.Webhook{
27+
{Id: "id1"},
28+
},
29+
},
30+
MaxConcurrentRuns: 1,
31+
},
32+
},
33+
Response: Job{
34+
JobID: 789,
35+
},
36+
},
37+
{
38+
Method: "GET",
39+
Resource: "/api/2.1/jobs/get?job_id=789",
40+
Response: Job{
41+
Settings: &JobSettings{
42+
Name: "Webhook test",
43+
Tasks: []JobTaskSettings{
44+
{
45+
ExistingClusterID: "abc",
46+
},
47+
},
48+
WebhookNotifications: &jobs.WebhookNotifications{
49+
OnSuccess: []jobs.Webhook{
50+
{Id: "id1"},
51+
},
52+
},
53+
MaxConcurrentRuns: 1,
54+
},
55+
},
56+
},
57+
},
58+
ID: "789",
59+
Update: true,
60+
Resource: ResourceJob(),
61+
InstanceState: map[string]string{
62+
"webhook_notifications.#": "1",
63+
"webhook_notifications.0.on_duration_warning_threshold_exceeded.#": "1",
64+
"webhook_notifications.0.on_duration_warning_threshold_exceeded.0.id": "id1",
65+
"webhook_notifications.0.on_failure.#": "1",
66+
"webhook_notifications.0.on_failure.0.id": "id1",
67+
"webhook_notifications.0.on_start.#": "1",
68+
"webhook_notifications.0.on_start.0.id": "id1",
69+
"webhook_notifications.0.on_success.#": "1",
70+
"webhook_notifications.0.on_success.0.id": "id1",
71+
},
72+
HCL: `
73+
name = "Webhook test"
74+
task {
75+
existing_cluster_id = "abc"
76+
}
77+
webhook_notifications {
78+
// Remove everything but "on_success"
79+
on_success {
80+
id = "id1"
81+
}
82+
}
83+
`,
84+
}.ApplyNoError(t)
85+
}

‎sql/resource_sql_endpoint.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func ResourceSqlEndpoint() common.Resource {
5656
m map[string]*schema.Schema) map[string]*schema.Schema {
5757
m["id"].Computed = true
5858
common.SetDefault(m["auto_stop_mins"], 120)
59-
common.SetSuppressDiff(m["channel"])
59+
common.CustomizeSchemaPath(m, "channel").SetSuppressDiff()
6060
common.MustSchemaPath(m, "channel", "name").Default = "CHANNEL_NAME_CURRENT"
6161
common.SetRequired(m["cluster_size"])
6262
common.SetReadOnly(m["creator_name"])
@@ -69,19 +69,19 @@ func ResourceSqlEndpoint() common.Resource {
6969
common.SetDefault(m["max_num_clusters"], 1)
7070
m["max_num_clusters"].ValidateDiagFunc = validation.ToDiagFunc(
7171
validation.IntBetween(1, MaxNumClusters))
72-
common.SetSuppressDiff(m["min_num_clusters"])
72+
common.CustomizeSchemaPath(m, "min_num_clusters").SetSuppressDiff()
7373
common.SetRequired(m["name"])
7474
common.SetReadOnly(m["num_active_sessions"])
7575
common.SetReadOnly(m["num_clusters"])
7676
common.SetReadOnly(m["odbc_params"])
7777
common.SetDefault(m["spot_instance_policy"], "COST_OPTIMIZED")
7878
common.SetReadOnly(m["state"])
79-
common.SetSuppressDiff(m["tags"])
79+
common.CustomizeSchemaPath(m, "tags").SetSuppressDiff()
8080
common.SetRequired(common.MustSchemaPath(m, "tags", "custom_tags", "key"))
8181
common.SetRequired(common.MustSchemaPath(m, "tags", "custom_tags", "value"))
82-
common.SetSuppressDiff(m["warehouse_type"])
83-
m["warehouse_type"].ValidateDiagFunc = validation.ToDiagFunc(
84-
validation.StringInSlice([]string{"PRO", "CLASSIC"}, false))
82+
common.CustomizeSchemaPath(m, "warehouse_type").
83+
SetSuppressDiff().
84+
SetValidateDiagFunc(validation.ToDiagFunc(validation.StringInSlice([]string{"PRO", "CLASSIC"}, false)))
8585
return m
8686
})
8787
return common.Resource{

0 commit comments

Comments
 (0)