Make config-remote-sync patching logic more generic (#4400)

ilyakuz-db · web-flow · commit e46c3fbd331c · 2026-01-30T15:26:59.000Z
## Changes

Split the config-remote-sync command into 3 separate phases. Previously,
some DAB-specific logic was embedded in the patcher, now it

1. Detect (diff.go) - Compare states, handle server-side and CLI
defaults. Convert value to the internal struct. Compute which operation
type should be applied to the config

2. Resolve (resolve.go) - Determine file locations and field paths.
Replace [task_key=foo] selectors

3. Patch (patch.go) - Now only handles YAML modifications, doesn't hold
any config-specific logic


Changes:

1. new ConfigChangeDesc struct 
2. normalizeValue() moved from patch.go to diff.go
3. selector handling moved to `resolve.go`  
4. /targets/ prefixes moved from `patch.go` to `resolve.go`          
5. new explicit type for operations which we also show in the output
6. removed bunch of redundant tests - this logic is covered in
acceptance suites
 
## Why
Preparation before the next PR, where I plan to properly handle CLI
defaults

## Tests
1. Updated snapshots to capture new debug output
2. Removed redundant unit tests - these cases are already covered with
acceptance tests

&lt;!-- If your PR needs to be included in the release notes for next
release,
add a separate entry in NEXT_CHANGELOG.md as part of your PR. --&gt;
diff --git a/acceptance/bundle/config-remote-sync/config_edits/output.txt b/acceptance/bundle/config-remote-sync/config_edits/output.txt
@@ -22,9 +22,10 @@ Deployment complete!
 Detected changes in 1 resource(s):
 
 Resource: resources.jobs.my_job
-  email_notifications.on_failure[0]: update
-  max_concurrent_runs: update
-  tags['env']: update
+  email_notifications.on_failure[0]: replace
+  max_concurrent_runs: replace
+  tags['env']: remove
+
 
 
 === Configuration changes
diff --git a/acceptance/bundle/config-remote-sync/formatting_preserved/output.txt b/acceptance/bundle/config-remote-sync/formatting_preserved/output.txt
@@ -8,7 +8,8 @@ Deployment complete!
 Detected changes in 1 resource(s):
 
 Resource: resources.jobs.my_job
-  max_concurrent_runs: update
+  max_concurrent_runs: replace
+
 
 
 === Configuration changes
diff --git a/acceptance/bundle/config-remote-sync/job_fields/output.txt b/acceptance/bundle/config-remote-sync/job_fields/output.txt
@@ -10,11 +10,12 @@ Deployment complete!
 Detected changes in 1 resource(s):
 
 Resource: resources.jobs.my_job
-  email_notifications.no_alert_for_skipped_runs: skip
-  email_notifications.on_failure: skip
-  parameters: update
-  tags['team']: update
-  trigger.periodic.interval: update
+  email_notifications.no_alert_for_skipped_runs: add
+  email_notifications.on_failure: add
+  parameters: replace
+  tags['team']: add
+  trigger.periodic.interval: replace
+
 
 
 === Configuration changes
diff --git a/acceptance/bundle/config-remote-sync/job_multiple_tasks/output.txt b/acceptance/bundle/config-remote-sync/job_multiple_tasks/output.txt
@@ -8,10 +8,11 @@ Deployment complete!
 Detected changes in 1 resource(s):
 
 Resource: resources.jobs.my_job
-  tasks[task_key='task2']: update
-  tasks[task_key='task3'].depends_on[0].task_key: update
-  tasks[task_key='task3'].new_cluster.num_workers: update
-  tasks[task_key='task3'].timeout_seconds: skip
+  tasks[task_key='task2']: remove
+  tasks[task_key='task3'].depends_on[0].task_key: replace
+  tasks[task_key='task3'].new_cluster.num_workers: replace
+  tasks[task_key='task3'].timeout_seconds: add
+
 
 
 === Configuration changes
diff --git a/acceptance/bundle/config-remote-sync/job_pipeline_task/output.txt b/acceptance/bundle/config-remote-sync/job_pipeline_task/output.txt
@@ -9,9 +9,11 @@ Deployment complete!
 Detected changes in 2 resource(s):
 
 Resource: resources.jobs.my_job
-  tasks[task_key='run_pipeline'].pipeline_task.full_refresh: update
+  tasks[task_key='run_pipeline'].pipeline_task.full_refresh: replace
+
 Resource: resources.pipelines.my_pipeline
-  development: update
+  development: replace
+
 
 
 === Configuration changes
diff --git a/acceptance/bundle/config-remote-sync/multiple_files/output.txt b/acceptance/bundle/config-remote-sync/multiple_files/output.txt
@@ -8,9 +8,11 @@ Deployment complete!
 Detected changes in 2 resource(s):
 
 Resource: resources.jobs.job_one
-  max_concurrent_runs: update
+  max_concurrent_runs: replace
+
 Resource: resources.jobs.job_two
-  max_concurrent_runs: update
+  max_concurrent_runs: replace
+
 
 
 === Changes in job1.yml
diff --git a/acceptance/bundle/config-remote-sync/multiple_resources/output.txt b/acceptance/bundle/config-remote-sync/multiple_resources/output.txt
@@ -8,11 +8,13 @@ Deployment complete!
 Detected changes in 2 resource(s):
 
 Resource: resources.jobs.job_one
-  max_concurrent_runs: update
-  tags: skip
+  max_concurrent_runs: replace
+  tags: add
+
 Resource: resources.jobs.job_two
-  max_concurrent_runs: update
-  tags: skip
+  max_concurrent_runs: replace
+  tags: add
+
 
 
 === Configuration changes
diff --git a/acceptance/bundle/config-remote-sync/output_json/output.txt b/acceptance/bundle/config-remote-sync/output_json/output.txt
@@ -14,15 +14,12 @@ Deployment complete!
   "changes": {
     "resources.jobs.test_job": {
       "max_concurrent_runs": {
-        "action": "update",
-        "old": 1,
-        "new": 1,
-        "remote": 3
+        "operation": "replace",
+        "value": 3
       },
       "tags": {
-        "action": "skip",
-        "reason": "server_side_default",
-        "remote": {
+        "operation": "add",
+        "value": {
           "env": "test"
         }
       }
diff --git a/acceptance/bundle/config-remote-sync/output_no_changes/output.txt b/acceptance/bundle/config-remote-sync/output_no_changes/output.txt
@@ -3,13 +3,16 @@ Deploying resources...
 Updating deployment state...
 Deployment complete!
 
-=== Check for changes immediately after deploymentNo changes detected.
+=== Check for changes immediately after deployment
+No changes detected.
 
 
-=== Text outputNo changes detected.
+=== Text output
+No changes detected.
 
 
-=== JSON output{
+=== JSON output
+{
   "files": null,
   "changes": {}
 }
diff --git a/acceptance/bundle/config-remote-sync/output_no_changes/script b/acceptance/bundle/config-remote-sync/output_no_changes/script
@@ -6,11 +6,14 @@ touch dummy.whl
 $CLI bundle deploy
 
 title "Check for changes immediately after deployment"
+echo
 $CLI bundle config-remote-sync
 
 title "Text output"
+echo
 $CLI bundle config-remote-sync | contains.py "No changes detected"
 
 title "JSON output"
+echo
 $CLI bundle config-remote-sync -o json > out.json
 cat out.json
diff --git a/acceptance/bundle/config-remote-sync/pipeline_fields/output.txt b/acceptance/bundle/config-remote-sync/pipeline_fields/output.txt
@@ -8,11 +8,12 @@ Deployment complete!
 Detected changes in 1 resource(s):
 
 Resource: resources.pipelines.my_pipeline
-  configuration['key2']: update
-  notifications[0].alerts: update
-  notifications[0].email_recipients: update
-  schema: update
-  tags['foo']: update
+  configuration['key2']: add
+  notifications[0].alerts: replace
+  notifications[0].email_recipients: replace
+  schema: replace
+  tags['foo']: add
+
 
 
 === Configuration changes
diff --git a/acceptance/bundle/config-remote-sync/target_override/output.txt b/acceptance/bundle/config-remote-sync/target_override/output.txt
@@ -8,9 +8,10 @@ Deployment complete!
 Detected changes in 1 resource(s):
 
 Resource: resources.jobs.my_job
-  max_concurrent_runs: update
-  tags['env']: update
-  tags['owner']: update
+  max_concurrent_runs: replace
+  tags['env']: replace
+  tags['owner']: add
+
 
 
 === Configuration changes
diff --git a/bundle/configsync/diff.go b/bundle/configsync/diff.go
@@ -2,20 +2,118 @@ package configsync
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
+	"reflect"
 	"strings"
 
 	"github.com/databricks/cli/bundle"
 	"github.com/databricks/cli/bundle/config/engine"
 	"github.com/databricks/cli/bundle/deployplan"
 	"github.com/databricks/cli/bundle/direct"
 	"github.com/databricks/cli/libs/log"
+	"github.com/databricks/databricks-sdk-go/marshal"
 )
 
+type OperationType string
+
+const (
+	OperationUnknown OperationType = "unknown"
+	OperationAdd     OperationType = "add"
+	OperationRemove  OperationType = "remove"
+	OperationReplace OperationType = "replace"
+	OperationSkip    OperationType = "skip"
+)
+
+type ConfigChangeDesc struct {
+	Operation OperationType `json:"operation"`
+	Value     any           `json:"value,omitempty"` // Normalized remote value (nil for remove operations)
+}
+
+type ResourceChanges map[string]*ConfigChangeDesc
+
+type Changes map[string]ResourceChanges
+
+// normalizeValue converts values to plain Go types suitable for YAML patching
+// by using SDK marshaling which properly handles ForceSendFields and other annotations.
+func normalizeValue(v any) (any, error) {
+	if v == nil {
+		return nil, nil
+	}
+
+	switch v.(type) {
+	case bool, string, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64:
+		return v, nil
+	}
+
+	rv := reflect.ValueOf(v)
+	rt := rv.Type()
+
+	if rt.Kind() == reflect.Ptr {
+		rt = rt.Elem()
+	}
+
+	var data []byte
+	var err error
+
+	if rt.Kind() == reflect.Struct {
+		data, err = marshal.Marshal(v)
+	} else {
+		data, err = json.Marshal(v)
+	}
+
+	if err != nil {
+		return v, fmt.Errorf("failed to marshal value of type %T: %w", v, err)
+	}
+
+	var normalized any
+	err = json.Unmarshal(data, &normalized)
+	if err != nil {
+		return v, fmt.Errorf("failed to unmarshal value: %w", err)
+	}
+
+	return normalized, nil
+}
+
+func convertChangeDesc(path string, cd *deployplan.ChangeDesc) (*ConfigChangeDesc, error) {
+	hasConfigValue := cd.Old != nil || cd.New != nil
+
+	op := OperationUnknown
+	if shouldSkipField(path, cd) {
+		return &ConfigChangeDesc{
+			Operation: OperationSkip,
+		}, nil
+	}
+
+	if cd.Remote == nil && hasConfigValue {
+		op = OperationRemove
+	}
+	if cd.Remote != nil && hasConfigValue {
+		op = OperationReplace
+	}
+	if cd.Remote != nil && !hasConfigValue {
+		op = OperationAdd
+	}
+
+	var normalizedValue any
+	var err error
+	if op != OperationRemove {
+		normalizedValue, err = normalizeValue(cd.Remote)
+		if err != nil {
+			return nil, fmt.Errorf("failed to normalize remote value: %w", err)
+		}
+	}
+
+	return &ConfigChangeDesc{
+		Operation: op,
+		Value:     normalizedValue,
+	}, nil
+}
+
 // DetectChanges compares current remote state with the last deployed state
 // and returns a map of resource changes.
-func DetectChanges(ctx context.Context, b *bundle.Bundle, engine engine.EngineType) (map[string]deployplan.Changes, error) {
-	changes := make(map[string]deployplan.Changes)
+func DetectChanges(ctx context.Context, b *bundle.Bundle, engine engine.EngineType) (Changes, error) {
+	changes := make(Changes)
 
 	deployBundle := &direct.DeploymentBundle{}
 	var statePath string
@@ -31,14 +129,18 @@ func DetectChanges(ctx context.Context, b *bundle.Bundle, engine engine.EngineTy
 	}
 
 	for resourceKey, entry := range plan.Plan {
-		resourceChanges := make(deployplan.Changes)
+		resourceChanges := make(ResourceChanges)
 
 		if entry.Changes != nil {
 			for path, changeDesc := range entry.Changes {
-				if shouldSkipField(path, changeDesc) {
+				change, err := convertChangeDesc(path, changeDesc)
+				if err != nil {
+					return nil, fmt.Errorf("failed to compute config change for path %s: %w", path, err)
+				}
+				if change.Operation == OperationSkip {
 					continue
 				}
-				resourceChanges[path] = changeDesc
+				resourceChanges[path] = change
 			}
 		}
 
diff --git a/bundle/configsync/format.go b/bundle/configsync/format.go
@@ -4,12 +4,10 @@ import (
 	"fmt"
 	"sort"
 	"strings"
-
-	"github.com/databricks/cli/bundle/deployplan"
 )
 
 // FormatTextOutput formats the config changes as human-readable text. Useful for debugging
-func FormatTextOutput(changes map[string]deployplan.Changes) string {
+func FormatTextOutput(changes Changes) string {
 	var output strings.Builder
 
 	if len(changes) == 0 {
@@ -36,9 +34,11 @@ func FormatTextOutput(changes map[string]deployplan.Changes) string {
 		sort.Strings(paths)
 
 		for _, path := range paths {
-			changeDesc := resourceChanges[path]
-			output.WriteString(fmt.Sprintf("  %s: %s\n", path, changeDesc.Action))
+			configChange := resourceChanges[path]
+			output.WriteString(fmt.Sprintf("  %s: %s\n", path, configChange.Operation))
 		}
+
+		output.WriteString("\n")
 	}
 
 	return output.String()
diff --git a/bundle/configsync/output.go b/bundle/configsync/output.go
@@ -6,7 +6,6 @@ import (
 	"path/filepath"
 
 	"github.com/databricks/cli/bundle"
-	"github.com/databricks/cli/bundle/deployplan"
 )
 
 // FileChange represents a change to a bundle configuration file
@@ -18,8 +17,8 @@ type FileChange struct {
 
 // DiffOutput represents the complete output of the config-remote-sync command
 type DiffOutput struct {
-	Files   []FileChange                  `json:"files"`
-	Changes map[string]deployplan.Changes `json:"changes"`
+	Files   []FileChange `json:"files"`
+	Changes Changes      `json:"changes"`
 }
 
 // SaveFiles writes all file changes to disk.
diff --git a/bundle/configsync/patch.go b/bundle/configsync/patch.go
diff --git a/bundle/configsync/patch_test.go b/bundle/configsync/patch_test.go
diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go
diff --git a/bundle/configsync/resolve_test.go b/bundle/configsync/resolve_test.go
diff --git a/cmd/bundle/config_remote_sync.go b/cmd/bundle/config_remote_sync.go
diff --git a/go.mod b/go.mod
