Report config-remote-sync changes that were not written back

Co-authored-by: Isaac

Author: ilyakuz-db

acceptance/bundle/config-remote-sync/split/keyed_rename/output.txt

@@ -75,13 +75,20 @@ Resource: resources.jobs.shift_job
 
 === Rename the two-block task AND edit one of its fields in the same run
 === Sync
+Warn: not written back: resources.jobs.rename_job.tasks[task_key='a_shared']: a split element cannot be removed and recreated in one run
+Warn: not written back: resources.jobs.rename_job.tasks[task_key='b_shared']: a split element cannot be removed and recreated in one run
 Detected changes in 1 resource(s):
 
 Resource: resources.jobs.rename_job
   tasks[task_key='a_shared']: remove
   tasks[task_key='b_shared']: add
   tasks[task_key='z_solo']: replace
 
+Not written back (2 change(s)):
+
+  resources.jobs.rename_job.tasks[task_key='a_shared']: a split element cannot be removed and recreated in one run
+  resources.jobs.rename_job.tasks[task_key='b_shared']: a split element cannot be removed and recreated in one run
+
 
 
 === Left unapplied: the split is intact and timeout_seconds stays target-scoped
@@ -104,6 +111,18 @@ Resource: resources.jobs.rename_job
 >>> grep -c timeout_seconds: 45 databricks.yml
 1
 
+=== The same run in JSON reports what it did not write
+Warn: not written back: resources.jobs.rename_job.tasks[task_key='a_shared']: a split element cannot be removed and recreated in one run
+Warn: not written back: resources.jobs.rename_job.tasks[task_key='b_shared']: a split element cannot be removed and recreated in one run
+
+>>> gron.py
+json.skipped[0].resourceKey = "resources.jobs.rename_job";
+json.skipped[0].fieldPath = "tasks[task_key='a_shared']";
+json.skipped[0].reason = "a split element cannot be removed and recreated in one run";
+json.skipped[1].resourceKey = "resources.jobs.rename_job";
+json.skipped[1].fieldPath = "tasks[task_key='b_shared']";
+json.skipped[1].reason = "a split element cannot be removed and recreated in one run";
+
 >>> [CLI] bundle destroy --auto-approve -t dev
 The following resources will be deleted:
   delete resources.jobs.rename_job

acceptance/bundle/config-remote-sync/split/keyed_rename/script

@@ -65,6 +65,10 @@ rm databricks.yml.backup
 # rename, so the halves would route apart: the removal deletes the element from
 # both blocks while the addition recreates it in one, collapsing the split and
 # moving timeout_seconds out of the target scope. It has to be left for a later run.
+#
+# The command still reports the change, so both halves have to appear under
+# "Not written back" -- and in the JSON output, which the in-workspace UI reads --
+# or the caller cannot tell an unapplied change from an applied one.
 title "Rename the two-block task AND edit one of its fields in the same run"
 edit_resource.py jobs $job_id <<EOF
 for task in r["tasks"]:
@@ -84,3 +88,9 @@ trace diff.py databricks.yml.backup databricks.yml
 errcode trace grep -c "task_key: a_shared" databricks.yml
 errcode trace grep -c "timeout_seconds: 45" databricks.yml
 rm databricks.yml.backup
+
+title "The same run in JSON reports what it did not write"
+echo
+$CLI bundle config-remote-sync -t dev --select-ids "jobs:$job_id" -o json > tmp.json
+trace gron.py < tmp.json | grep skipped
+rm tmp.json

acceptance/bundle/config-remote-sync/split/rename_ambiguous_pairing/output.txt

@@ -5,6 +5,10 @@ Deployment complete!
 
 === alpha (top-level) -> zzz and beta (target block) -> aaa, bodies identical
 === Sync
+Warn: not written back: resources.jobs.ambiguous_rename_job.tasks[task_key='alpha']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
+Warn: not written back: resources.jobs.ambiguous_rename_job.tasks[task_key='beta']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
+Warn: not written back: resources.jobs.ambiguous_rename_job.tasks[task_key='aaa']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
+Warn: not written back: resources.jobs.ambiguous_rename_job.tasks[task_key='zzz']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
 Detected changes in 1 resource(s):
 
 Resource: resources.jobs.ambiguous_rename_job
@@ -13,6 +17,13 @@ Resource: resources.jobs.ambiguous_rename_job
   tasks[task_key='beta']: remove
   tasks[task_key='zzz']: add
 
+Not written back (4 change(s)):
+
+  resources.jobs.ambiguous_rename_job.tasks[task_key='alpha']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
+  resources.jobs.ambiguous_rename_job.tasks[task_key='beta']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
+  resources.jobs.ambiguous_rename_job.tasks[task_key='aaa']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
+  resources.jobs.ambiguous_rename_job.tasks[task_key='zzz']: several elements with the same contents were renamed in one run, so the new keys cannot be matched to them
+
 
 
 === Neither rename is applied, and both blocks keep their own task

bundle/configsync/format.go

@@ -7,8 +7,10 @@ import (
 	"strings"
 )
 
-// FormatTextOutput formats the config changes as human-readable text. Useful for debugging
-func FormatTextOutput(changes Changes) string {
+// FormatTextOutput formats the config changes as human-readable text. Useful for debugging.
+// Changes that were detected but not written back are listed separately, so a run that
+// reported changes without applying them is distinguishable from one that applied them.
+func FormatTextOutput(changes Changes, skipped []SkippedChange) string {
 	var output strings.Builder
 
 	if len(changes) == 0 {
@@ -34,5 +36,13 @@ func FormatTextOutput(changes Changes) string {
 		output.WriteString("\n")
 	}
 
+	if len(skipped) > 0 {
+		fmt.Fprintf(&output, "Not written back (%d change(s)):\n\n", len(skipped))
+		for _, s := range skipped {
+			fmt.Fprintf(&output, "  %s.%s: %s\n", s.ResourceKey, s.FieldPath, s.Reason)
+		}
+		output.WriteString("\n")
+	}
+
 	return output.String()
 }

bundle/configsync/output.go

@@ -19,6 +19,9 @@ type FileChange struct {
 type DiffOutput struct {
 	Files   []FileChange `json:"files"`
 	Changes Changes      `json:"changes"`
+	// Skipped lists changes present in Changes that were not written back, so a
+	// consumer of this output does not report them as applied.
+	Skipped []SkippedChange `json:"skipped,omitempty"`
 }
 
 // SaveFiles writes all file changes to disk.

bundle/configsync/patch_test.go

@@ -50,8 +50,9 @@ resources:
 		},
 	}
 
-	fieldChanges, err := ResolveChanges(ctx, b, changes)
+	fieldChanges, skipped, err := ResolveChanges(ctx, b, changes)
 	require.NoError(t, err)
+	require.Empty(t, skipped)
 
 	fileChanges, err := ApplyChangesToYAML(ctx, b, fieldChanges)
 	require.NoError(t, err)

bundle/configsync/resolve.go

@@ -29,6 +29,17 @@ type FieldChange struct {
 	mergedPath string
 }
 
+// SkippedChange is a detected change that was not written back, because doing so
+// would have meant guessing where it belongs.
+type SkippedChange struct {
+	// ResourceKey and FieldPath together identify the change, e.g.
+	// "resources.jobs.foo" and "tasks[task_key='main'].max_retries".
+	ResourceKey string `json:"resourceKey"`
+	FieldPath   string `json:"fieldPath"`
+	// Reason is a short explanation suitable for the command's output.
+	Reason string `json:"reason"`
+}
+
 // sequenceStep records a sequence element that a change path navigated through.
 // The element's index inside a physical block is only known once the block has
 // been chosen, so the merged position is kept until then.
@@ -200,8 +211,14 @@ func adjustArrayIndex(path *structpath.PatternNode, scope string, operations map
 }
 
 // ResolveChanges resolves selectors and computes field path candidates for each change.
-func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes) ([]FieldChange, error) {
+//
+// A change that cannot be attributed to a single source location is left unapplied
+// rather than written to a guessed one, and is returned in skipped. The caller is
+// expected to report those: the command runs unattended, so a change that silently
+// disappears looks identical to one that was written.
+func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes) ([]FieldChange, []SkippedChange, error) {
 	var result []FieldChange
+	var skipped []SkippedChange
 	targetName := b.Config.Bundle.Target
 	blocks := newBlockResolver(ctx, b)
 
@@ -264,13 +281,14 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes
 			}
 			if reason, ok := renames.unpairedPaths[fieldPath]; ok {
 				log.Debugf(ctx, "config-remote-sync: skipping %s: %s", fullPath, reason)
+				skipped = append(skipped, SkippedChange{ResourceKey: resourceKey, FieldPath: fieldPath, Reason: reason})
 				continue
 			}
 			rename, isRename := renames.byRemovePath[fieldPath]
 
 			resolved, err := resolveSelectors(fullPath, b, configChange.Operation)
 			if err != nil {
-				return nil, fmt.Errorf("failed to resolve selectors in path %s: %w", fullPath, err)
+				return nil, nil, fmt.Errorf("failed to resolve selectors in path %s: %w", fullPath, err)
 			}
 			resolvedPath := resolved.path
 
@@ -288,6 +306,9 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes
 					if routeErr == nil && len(destinations) == 0 {
 						// The element could not be attributed to a block; leave the
 						// whole pair for a later run rather than half-applying it.
+						const reason = "the renamed element cannot be attributed to a source location"
+						log.Debugf(ctx, "config-remote-sync: skipping %s: %s", fullPath, reason)
+						skipped = append(skipped, SkippedChange{ResourceKey: resourceKey, FieldPath: fieldPath, Reason: reason})
 						continue
 					}
 				} else {
@@ -298,9 +319,10 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes
 						// Applying this change would mean guessing a location.
 						// Leave it unapplied; a later run can pick it up.
 						log.Debugf(ctx, "config-remote-sync: skipping %s: %v", fullPath, routeErr)
+						skipped = append(skipped, SkippedChange{ResourceKey: resourceKey, FieldPath: fieldPath, Reason: routeErr.Error()})
 						continue
 					}
-					return nil, fmt.Errorf("failed to route change %s: %w", fullPath, routeErr)
+					return nil, nil, fmt.Errorf("failed to route change %s: %w", fullPath, routeErr)
 				}
 				routed = true
 			}
@@ -421,7 +443,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes
 						filePath = resourceLocation.File
 					}
 					if filePath == "" {
-						return nil, fmt.Errorf("failed to find location for resource %s for a field %s", resourceKey, fieldPath)
+						return nil, nil, fmt.Errorf("failed to find location for resource %s for a field %s", resourceKey, fieldPath)
 					}
 
 					log.Debugf(ctx, "Field %s has no location, using %s", fullPath, filePath)
@@ -444,7 +466,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes
 		}
 	}
 
-	return result, nil
+	return result, skipped, nil
 }
 
 // translateWorkspacePaths recursively converts absolute workspace paths to relative

bundle/configsync/telemetry.go

@@ -49,6 +49,10 @@ type Stats struct {
 	FilesChangedCount int64
 	FilesWrittenCount int64
 
+	// Changes that were detected and reported but not written back, because they
+	// could not be attributed to a single source location.
+	SkippedChangesCount int64
+
 	Restore RestoreStats
 
 	ErrorMessage  string
@@ -178,6 +182,7 @@ func (s *Stats) LogTelemetry(ctx context.Context) {
 			ResourceChanges:        resourceChanges,
 			FilesChangedCount:      s.FilesChangedCount,
 			FilesWrittenCount:      s.FilesWrittenCount,
+			SkippedChangesCount:    s.SkippedChangesCount,
 			RefsRetargeted:         s.Restore.Retargeted,
 			RefsFromSiblings:       s.Restore.FromSiblings,
 			ErrorMessage:           s.ErrorMessage,

cmd/bundle/config_remote_sync.go

@@ -111,11 +111,17 @@ Examples:
 					changes = configsync.FilterChanges(changes, selected)
 				}
 
-				fieldChanges, err := configsync.ResolveChanges(ctx, b, changes)
+				fieldChanges, skipped, err := configsync.ResolveChanges(ctx, b, changes)
 				if err != nil {
 					stats.ErrorCategory = protos.BundleConfigRemoteSyncErrorCategoryResolveFailed
 					return fmt.Errorf("failed to resolve field changes: %w", err)
 				}
+				stats.SkippedChangesCount = int64(len(skipped))
+				for _, s := range skipped {
+					// The command runs unattended, so a dropped change needs a signal
+					// that survives without debug logging enabled.
+					log.Warnf(ctx, "not written back: %s.%s: %s", s.ResourceKey, s.FieldPath, s.Reason)
+				}
 
 				if err := configsync.RestoreVariableReferences(ctx, b, fieldChanges, &stats.Restore); err != nil {
 					log.Warnf(ctx, "variable restoration skipped: %v", err)
@@ -141,14 +147,15 @@ Examples:
 					diffOutput := &configsync.DiffOutput{
 						Files:   files,
 						Changes: changes,
+						Skipped: skipped,
 					}
 					result, err = json.MarshalIndent(diffOutput, "", "  ")
 					if err != nil {
 						stats.ErrorCategory = protos.BundleConfigRemoteSyncErrorCategoryOutputFailed
 						return fmt.Errorf("failed to marshal output: %w", err)
 					}
 				} else if root.OutputType(cmd) == flags.OutputText {
-					result = []byte(configsync.FormatTextOutput(changes))
+					result = []byte(configsync.FormatTextOutput(changes, skipped))
 				}
 
 				out := cmd.OutOrStdout()

libs/telemetry/protos/bundle_config_remote_sync.go

@@ -54,6 +54,14 @@ type BundleConfigRemoteSyncEvent struct {
 	FilesChangedCount int64 `json:"files_changed_count,omitempty"`
 	FilesWrittenCount int64 `json:"files_written_count,omitempty"`
 
+	// Number of detected changes that were not written back because they could
+	// not be attributed to a single source location (e.g. a sequence element
+	// defined in both a top-level block and a target override). These are
+	// counted in ChangesTotal: a nonzero value means the command reported
+	// changes it did not apply. Distinct from the "skip" operation filtered out
+	// during change detection, which never reaches ChangesTotal.
+	SkippedChangesCount int64 `json:"skipped_changes_count,omitempty"`
+
 	// Variable-reference restoration counts for the two mechanisms that can
 	// write a current-target-scoped reference into a shared file (the source of
 	// the cross-target "reference does not exist" failures).