Skip to content

Commit 21a4c6f

Browse files
Add isLocal parameter to FieldTriggers and ClassifyChanges (#3741)
## Changes: 1. Change ActionTypeUnset to ActionTypeUndefined 2. Add isLocal parameter to FieldTriggers and ClassifyChanges ## Why Resources like dashboards need to have differentiated local and remote diff computations. For example, the `serialized_dashboard` field should only be compared in local diff, and `etag` should only be compared in remote drift. Thus we need different triggers based on whether the execution is local or remote. ## Tests Existing tests.
1 parent 16cfbd7 commit 21a4c6f

File tree

14 files changed

+132
-61
lines changed

14 files changed

+132
-61
lines changed

bundle/deployplan/action.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ type ActionType int
3030
// If case of several options, action with highest severity wins.
3131
// Note, Create/Delete are handled explicitly and never compared.
3232
const (
33-
ActionTypeUnset ActionType = iota
33+
ActionTypeUndefined ActionType = iota
3434
ActionTypeSkip
3535
ActionTypeResize
3636
ActionTypeUpdate
@@ -86,7 +86,7 @@ func (a ActionType) String() string {
8686
func ActionTypeFromString(s string) ActionType {
8787
actionType, ok := nameToAction[s]
8888
if !ok {
89-
return ActionTypeUnset
89+
return ActionTypeUndefined
9090
}
9191
return actionType
9292
}

bundle/direct/bundle_apply.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func (b *DeploymentBundle) Apply(ctx context.Context, client *databricks.Workspa
5050
errorPrefix := fmt.Sprintf("cannot %s %s", action, resourceKey)
5151

5252
at := deployplan.ActionTypeFromString(action)
53-
if at == deployplan.ActionTypeUnset {
53+
if at == deployplan.ActionTypeUndefined {
5454
logdiag.LogError(ctx, fmt.Errorf("cannot deploy %s: unknown action %q", resourceKey, action))
5555
return false
5656
}
@@ -150,7 +150,7 @@ func (b *DeploymentBundle) LookupReferenceRemote(ctx context.Context, path *stru
150150
defer b.Plan.ReadUnlockEntry(targetResourceKey)
151151

152152
targetAction := deployplan.ActionTypeFromString(targetEntry.Action)
153-
if targetAction == deployplan.ActionTypeUnset {
153+
if targetAction == deployplan.ActionTypeUndefined {
154154
return nil, fmt.Errorf("internal error: %s: missing action in the plan", targetResourceKey)
155155
}
156156

bundle/direct/bundle_plan.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks
164164
}
165165
}
166166

167-
localAction, localChangeMap := convertChangesToTriggersMap(ctx, adapter, localDiff, remoteState)
167+
localAction, localChangeMap := localChangesToTriggers(ctx, adapter, localDiff, remoteState)
168168
if localAction == deployplan.ActionTypeRecreate {
169169
entry.Action = localAction.String()
170170
if len(localChangeMap) > 0 {
@@ -225,12 +225,12 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks
225225
return plan, nil
226226
}
227227

228-
func convertChangesToTriggersMap(ctx context.Context, adapter *dresources.Adapter, diff []structdiff.Change, remoteState any) (deployplan.ActionType, map[string]deployplan.Trigger) {
228+
func localChangesToTriggers(ctx context.Context, adapter *dresources.Adapter, diff []structdiff.Change, remoteState any) (deployplan.ActionType, map[string]deployplan.Trigger) {
229229
action := deployplan.ActionTypeSkip
230230
var m map[string]deployplan.Trigger
231231

232232
for _, ch := range diff {
233-
fieldAction, err := adapter.ClassifyChange(ch, remoteState)
233+
fieldAction, err := adapter.ClassifyChange(ch, remoteState, true)
234234
if err != nil {
235235
logdiag.LogError(ctx, fmt.Errorf("internal error: failed to classify change: %w", err))
236236
continue
@@ -262,7 +262,7 @@ func interpretOldStateVsRemoteState(ctx context.Context, adapter *dresources.Ada
262262
}
263263
continue
264264
}
265-
fieldAction, err := adapter.ClassifyChange(ch, remoteState)
265+
fieldAction, err := adapter.ClassifyChange(ch, remoteState, false)
266266
if err != nil {
267267
logdiag.LogError(ctx, fmt.Errorf("internal error: failed to classify change: %w", err))
268268
continue
@@ -297,7 +297,7 @@ func (b *DeploymentBundle) LookupReferenceLocal(ctx context.Context, path *struc
297297
defer b.Plan.ReadUnlockEntry(targetResourceKey)
298298

299299
targetAction := deployplan.ActionTypeFromString(targetEntry.Action)
300-
if targetAction == deployplan.ActionTypeUnset {
300+
if targetAction == deployplan.ActionTypeUndefined {
301301
return nil, fmt.Errorf("internal error: %s: missing action in the plan", targetResourceKey)
302302
}
303303

bundle/direct/dresources/adapter.go

Lines changed: 101 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"errors"
66
"fmt"
7+
"maps"
78
"reflect"
89

910
"github.com/databricks/cli/bundle/deployplan"
@@ -43,7 +44,16 @@ type IResource interface {
4344
// [Optional] FieldTriggers returns actions to trigger when given fields are changed.
4445
// Keys are field paths (e.g., "name", "catalog_name"). Values are actions.
4546
// Unspecified changed fields default to ActionTypeUpdate.
46-
FieldTriggers() map[string]deployplan.ActionType
47+
//
48+
// FieldTriggers(true) is applied on every change between state (last deployed config)
49+
// and new state (current config) to determine action based on config changes.
50+
//
51+
// FieldTriggers(false) is called on every change between state and remote state to
52+
// determine action based on remote drift.
53+
//
54+
// Note: these functions are called once per resource implementation initialization,
55+
// not once per resource.
56+
FieldTriggers(isLocal bool) map[string]deployplan.ActionType
4757
}
4858

4959
// IResourceNoRefresh describes additional methods for resource to implement.
@@ -77,8 +87,9 @@ type IResourceNoRefresh interface {
7787
// [Optional] WaitAfterUpdate waits for the resource to become ready after update.
7888
WaitAfterUpdate(ctx context.Context, newState any) error
7989

80-
// [Optional] ClassifyChange classifies a set of changes using custom logic.
81-
ClassifyChange(change structdiff.Change, remoteState any) (deployplan.ActionType, error)
90+
// [Optional] ClassifyChange classifies a change using custom logic.
91+
// The isLocal parameter indicates whether this is a local change (true) or remote change (false).
92+
ClassifyChange(change structdiff.Change, remoteState any, isLocal bool) (deployplan.ActionType, error)
8293
}
8394

8495
// IResourceWithRefresh is an alternative to IResourceNoRefresh but every method can return remoteState.
@@ -121,7 +132,8 @@ type Adapter struct {
121132
classifyChange *calladapt.BoundCaller
122133
doResize *calladapt.BoundCaller
123134

124-
fieldTriggers map[string]deployplan.ActionType
135+
fieldTriggersLocal map[string]deployplan.ActionType
136+
fieldTriggersRemote map[string]deployplan.ActionType
125137
}
126138

127139
func NewAdapter(typedNil any, client *databricks.WorkspaceClient) (*Adapter, error) {
@@ -138,18 +150,19 @@ func NewAdapter(typedNil any, client *databricks.WorkspaceClient) (*Adapter, err
138150
}
139151
impl := outs[0]
140152
adapter := &Adapter{
141-
prepareState: nil,
142-
remapState: nil,
143-
doRefresh: nil,
144-
doDelete: nil,
145-
doCreate: nil,
146-
doUpdate: nil,
147-
doUpdateWithID: nil,
148-
doResize: nil,
149-
waitAfterCreate: nil,
150-
waitAfterUpdate: nil,
151-
classifyChange: nil,
152-
fieldTriggers: map[string]deployplan.ActionType{},
153+
prepareState: nil,
154+
remapState: nil,
155+
doRefresh: nil,
156+
doDelete: nil,
157+
doCreate: nil,
158+
doUpdate: nil,
159+
doUpdateWithID: nil,
160+
doResize: nil,
161+
waitAfterCreate: nil,
162+
waitAfterUpdate: nil,
163+
classifyChange: nil,
164+
fieldTriggersLocal: map[string]deployplan.ActionType{},
165+
fieldTriggersRemote: map[string]deployplan.ActionType{},
153166
}
154167

155168
err = adapter.initMethods(impl)
@@ -163,14 +176,28 @@ func NewAdapter(typedNil any, client *databricks.WorkspaceClient) (*Adapter, err
163176
return nil, err
164177
}
165178
if triggerCall != nil {
166-
outs, err := triggerCall.Call()
167-
if err != nil || len(outs) != 1 {
168-
return nil, fmt.Errorf("failed to call FieldTriggers: %w", err)
179+
// Validate FieldTriggers signature: func(bool) map[string]deployplan.ActionType
180+
if len(triggerCall.InTypes) != 1 || triggerCall.InTypes[0] != reflect.TypeOf(false) {
181+
return nil, errors.New("FieldTriggers must take a single bool parameter (isLocal)")
169182
}
170-
fields := outs[0].(map[string]deployplan.ActionType)
171-
adapter.fieldTriggers = make(map[string]deployplan.ActionType, len(fields))
172-
for k, v := range fields {
173-
adapter.fieldTriggers[k] = v
183+
if len(triggerCall.OutTypes) != 1 {
184+
return nil, errors.New("FieldTriggers must return a single value")
185+
}
186+
expectedReturnType := reflect.TypeOf(map[string]deployplan.ActionType{})
187+
if triggerCall.OutTypes[0] != expectedReturnType {
188+
return nil, fmt.Errorf("FieldTriggers must return map[string]deployplan.ActionType, got %v", triggerCall.OutTypes[0])
189+
}
190+
191+
// Call with isLocal=true for local triggers
192+
adapter.fieldTriggersLocal, err = loadFieldTriggers(triggerCall, true)
193+
if err != nil {
194+
return nil, err
195+
}
196+
197+
// Call with isLocal=false for remote triggers
198+
adapter.fieldTriggersRemote, err = loadFieldTriggers(triggerCall, false)
199+
if err != nil {
200+
return nil, err
174201
}
175202
}
176203

@@ -182,6 +209,18 @@ func NewAdapter(typedNil any, client *databricks.WorkspaceClient) (*Adapter, err
182209
return adapter, nil
183210
}
184211

212+
// loadFieldTriggers calls FieldTriggers with isLocal parameter and returns the resulting map.
213+
func loadFieldTriggers(triggerCall *calladapt.BoundCaller, isLocal bool) (map[string]deployplan.ActionType, error) {
214+
outs, err := triggerCall.Call(isLocal)
215+
if err != nil || len(outs) != 1 {
216+
return nil, fmt.Errorf("failed to call FieldTriggers(%v): %w", isLocal, err)
217+
}
218+
fields := outs[0].(map[string]deployplan.ActionType)
219+
result := make(map[string]deployplan.ActionType, len(fields))
220+
maps.Copy(result, fields)
221+
return result, nil
222+
}
223+
185224
func (a *Adapter) initMethods(resource any) error {
186225
err := calladapt.EnsureNoExtraMethods(resource, calladapt.TypeOf[IResource](), calladapt.TypeOf[IResourceNoRefresh](), calladapt.TypeOf[IResourceWithRefresh]())
187226
if err != nil {
@@ -332,7 +371,10 @@ func (a *Adapter) validate() error {
332371
}
333372

334373
if a.classifyChange != nil {
335-
validations = append(validations, "ClassifyChange remoteState", a.classifyChange.InTypes[1], remoteType)
374+
validations = append(validations,
375+
"ClassifyChange remoteState", a.classifyChange.InTypes[1], remoteType,
376+
"ClassifyChange isLocal", a.classifyChange.InTypes[2], reflect.TypeOf(false),
377+
)
336378
}
337379

338380
err = validateTypes(validations...)
@@ -342,7 +384,12 @@ func (a *Adapter) validate() error {
342384

343385
// FieldTriggers validation
344386
hasUpdateWithIDTrigger := false
345-
for _, action := range a.fieldTriggers {
387+
for _, action := range a.fieldTriggersLocal {
388+
if action == deployplan.ActionTypeUpdateWithID {
389+
hasUpdateWithIDTrigger = true
390+
}
391+
}
392+
for _, action := range a.fieldTriggersRemote {
346393
if action == deployplan.ActionTypeUpdateWithID {
347394
hasUpdateWithIDTrigger = true
348395
}
@@ -485,10 +532,20 @@ func (a *Adapter) DoResize(ctx context.Context, id string, newState any) error {
485532
return err
486533
}
487534

488-
// ClassifyByTriggers classifies a single using FieldTriggers.
535+
// classifyByTriggers classifies a change using FieldTriggers.
489536
// Defaults to ActionTypeUpdate.
490-
func (a *Adapter) ClassifyByTriggers(change structdiff.Change) deployplan.ActionType {
491-
action, ok := a.fieldTriggers[change.Path.String()]
537+
// The isLocal parameter determines which trigger map to use:
538+
// - isLocal=true uses triggers from FieldTriggers(true)
539+
// - isLocal=false uses triggers from FieldTriggers(false)
540+
func (a *Adapter) classifyByTriggers(change structdiff.Change, isLocal bool) deployplan.ActionType {
541+
var triggers map[string]deployplan.ActionType
542+
if isLocal {
543+
triggers = a.fieldTriggersLocal
544+
} else {
545+
triggers = a.fieldTriggersRemote
546+
}
547+
548+
action, ok := triggers[change.Path.String()]
492549
if ok {
493550
return action
494551
}
@@ -539,21 +596,25 @@ func (a *Adapter) WaitAfterUpdate(ctx context.Context, newState any) (any, error
539596
}
540597
}
541598

542-
func (a *Adapter) ClassifyChange(change structdiff.Change, remoteState any) (deployplan.ActionType, error) {
543-
// If ClassifyChange is not implemented, use FieldTriggers.
544-
if a.classifyChange == nil {
545-
return a.ClassifyByTriggers(change), nil
546-
}
599+
// ClassifyChange classifies a change using custom logic or FieldTriggers.
600+
// The isLocal parameter determines whether this is a local or remote change:
601+
// - isLocal=true: classifying local changes (user modifications)
602+
// - isLocal=false: classifying remote changes (drift detection)
603+
func (a *Adapter) ClassifyChange(change structdiff.Change, remoteState any, isLocal bool) (deployplan.ActionType, error) {
604+
actionType := deployplan.ActionTypeUndefined
547605

548-
outs, err := a.classifyChange.Call(change, remoteState)
549-
if err != nil {
550-
return deployplan.ActionTypeSkip, err
606+
// If ClassifyChange is implemented, use it.
607+
if a.classifyChange != nil {
608+
outs, err := a.classifyChange.Call(change, remoteState, isLocal)
609+
if err != nil {
610+
return deployplan.ActionTypeUndefined, err
611+
}
612+
actionType = outs[0].(deployplan.ActionType)
551613
}
552614

553-
actionType := outs[0].(deployplan.ActionType)
554-
// If the action type is unset, use FieldTriggers.
555-
if actionType == deployplan.ActionTypeUnset {
556-
return a.ClassifyByTriggers(change), nil
615+
// If ClassifyChange is not implemented or is implemented but returns undefined, use FieldTriggers.
616+
if actionType == deployplan.ActionTypeUndefined {
617+
return a.classifyByTriggers(change, isLocal), nil
557618
}
558619
return actionType, nil
559620
}

bundle/direct/dresources/all_test.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,14 @@ func testCRUD(t *testing.T, group string, adapter *Adapter, client *databricks.W
230230
Path: path,
231231
Old: nil,
232232
New: "mynewname",
233-
}, remote)
233+
}, remote, true)
234+
require.NoError(t, err)
235+
236+
_, err = adapter.ClassifyChange(structdiff.Change{
237+
Path: path,
238+
Old: nil,
239+
New: "mynewname",
240+
}, remote, false)
234241
require.NoError(t, err)
235242
}
236243

@@ -258,8 +265,11 @@ func TestFieldTriggers(t *testing.T) {
258265
adapter, err := NewAdapter(resource, nil)
259266
require.NoError(t, err)
260267

261-
t.Run(resourceName, func(t *testing.T) {
262-
validateFields(t, adapter.InputConfigType(), adapter.fieldTriggers)
268+
t.Run(resourceName+"_local", func(t *testing.T) {
269+
validateFields(t, adapter.InputConfigType(), adapter.fieldTriggersLocal)
270+
})
271+
t.Run(resourceName+"_remote", func(t *testing.T) {
272+
validateFields(t, adapter.InputConfigType(), adapter.fieldTriggersRemote)
263273
})
264274
}
265275
}

bundle/direct/dresources/app.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func (r *ResourceApp) DoDelete(ctx context.Context, id string) error {
6464
return err
6565
}
6666

67-
func (*ResourceApp) FieldTriggers() map[string]deployplan.ActionType {
67+
func (*ResourceApp) FieldTriggers(_ bool) map[string]deployplan.ActionType {
6868
return map[string]deployplan.ActionType{
6969
"name": deployplan.ActionTypeRecreate,
7070
}

bundle/direct/dresources/cluster.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ func (r *ResourceCluster) DoDelete(ctx context.Context, id string) error {
101101
return r.client.Clusters.PermanentDeleteByClusterId(ctx, id)
102102
}
103103

104-
func (r *ResourceCluster) ClassifyChange(change structdiff.Change, remoteState *compute.ClusterDetails) (deployplan.ActionType, error) {
104+
func (r *ResourceCluster) ClassifyChange(change structdiff.Change, remoteState *compute.ClusterDetails, _ bool) (deployplan.ActionType, error) {
105105
// Always update if the cluster is not running.
106106
if remoteState.State != compute.StateRunning {
107107
return deployplan.ActionTypeUpdate, nil

bundle/direct/dresources/experiment.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func (r *ResourceExperiment) DoDelete(ctx context.Context, id string) error {
7171
})
7272
}
7373

74-
func (*ResourceExperiment) FieldTriggers() map[string]deployplan.ActionType {
74+
func (*ResourceExperiment) FieldTriggers(_ bool) map[string]deployplan.ActionType {
7575
// TF implementation: https://github.com/databricks/terraform-provider-databricks/blob/6c106e8e7052bb2726148d66309fd460ed444236/mlflow/resource_mlflow_experiment.go#L22
7676
return map[string]deployplan.ActionType{
7777
"name": deployplan.ActionTypeUpdate,

bundle/direct/dresources/model.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func (r *ResourceMlflowModel) DoDelete(ctx context.Context, id string) error {
107107
})
108108
}
109109

110-
func (*ResourceMlflowModel) FieldTriggers() map[string]deployplan.ActionType {
110+
func (*ResourceMlflowModel) FieldTriggers(_ bool) map[string]deployplan.ActionType {
111111
return map[string]deployplan.ActionType{
112112
// Recreate matches current behavior of Terraform. It is possible to rename without recreate
113113
// but that would require dynamic select of the method during update since

bundle/direct/dresources/pipeline.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ func (r *ResourcePipeline) DoDelete(ctx context.Context, id string) error {
117117
return r.client.Pipelines.DeleteByPipelineId(ctx, id)
118118
}
119119

120-
func (*ResourcePipeline) FieldTriggers() map[string]deployplan.ActionType {
120+
func (*ResourcePipeline) FieldTriggers(_ bool) map[string]deployplan.ActionType {
121121
return map[string]deployplan.ActionType{
122122
"storage": deployplan.ActionTypeRecreate,
123123
"catalog": deployplan.ActionTypeRecreate,

0 commit comments

Comments
 (0)