feat: updated diff reporting #328
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #328 +/- ##
==========================================
+ Coverage 44.02% 44.48% +0.46%
==========================================
Files 192 193 +1
Lines 13908 14027 +119
==========================================
+ Hits 6123 6240 +117
- Misses 7212 7214 +2
Partials 573 573 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request introduces a sophisticated nested diff reporting system for complex resource structures. When the experimental NestedDiffs flag is enabled, instead of showing entire nested objects as changed, the system recursively compares structures and reports only the specific leaf-level differences using dot notation (e.g., config.servers.1.port: 443 => 8443).
Key changes:
- Added experimental
NestedDiffsflag to enable granular diff reporting for nested structures - Implemented recursive diff computation with support for nested maps, arrays, and mixed structures
- Enhanced plan reporter to display property-level changes with full path notation when nested diffs are enabled
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/internal/config/experimental.go | Adds NestedDiffs boolean field to ExperimentalConfig for enabling nested diff reporting |
| cli/internal/syncer/reporters/diff.go | Implements new file with ComputeNestedDiffs function and supporting helpers for recursive diff computation with dot-notation paths |
| cli/internal/syncer/reporters/plan.go | Updates plan reporter to use nested diff logic when experimental flag is enabled, adds PropertyRef display handling, and sorts output for consistency |
| cli/internal/syncer/differ/diff.go | Modernizes code by replacing interface{} with any type alias throughout for Go 1.18+ compatibility |
| cli/internal/syncer/reporters/diff_test.go | Adds comprehensive test suite covering maps, arrays, mixed structures, type changes, nil handling, primitives, and multiple array element changes |
| cli/internal/syncer/reporters/plan_test.go | Expands tests to cover both flat and nested diff modes, including complex structures and PropertyRef handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil, false | ||
| } | ||
|
|
||
| // toSlice attempts to convert a value to []any |
There was a problem hiding this comment.
The function toSlice is missing documentation. It should have a doc comment explaining that it attempts to convert various slice types to []any using both type assertion and reflection.
| // toSlice attempts to convert a value to []any | |
| // toSlice attempts to convert various slice-typed values to []any, first using | |
| // a direct type assertion and then using reflection for other slice types. |
| return basePath + "." + key | ||
| } | ||
|
|
||
| // toMap attempts to convert a value to map[string]any |
There was a problem hiding this comment.
The function toMap is missing documentation. It should have a doc comment explaining its purpose as a type assertion helper that converts values to map[string]any.
| // toMap attempts to convert a value to map[string]any | |
| // toMap is a type assertion helper that converts a value to map[string]any. |
| } | ||
| return lines | ||
| } | ||
|
|
There was a problem hiding this comment.
The function formattedLine is missing documentation. It should have a doc comment explaining that it formats a diff line with property name, old/new values, and color coding.
| // formattedLine returns a single formatted diff line with the given property name, | |
| // showing the old and new values and applying appropriate color coding. |
| // Primitive comparison | ||
| if !reflect.DeepEqual(source, target) { | ||
| result[basePath] = ValuePair{Source: source, Target: target} | ||
| } |
There was a problem hiding this comment.
The PropertyRef struct contains a function field (Resolve), which cannot be compared using reflect.DeepEqual. This will cause a panic at runtime when comparing PropertyRef values. The code should handle PropertyRef as a special case, comparing only the comparable fields (URN, Property, IsResolved, Value) while ignoring the Resolve function field.
| return flattenDiffs("", source, target) | ||
| } | ||
|
|
||
| // flattenDiffs is the recursive implementation that compares values and builds paths |
There was a problem hiding this comment.
The function flattenDiffs is missing documentation. As an internal recursive helper function that's central to the diff computation algorithm, it should have a doc comment explaining its parameters, return value, and behavior.
| // flattenDiffs is the recursive implementation that compares values and builds paths | |
| // flattenDiffs is the internal recursive engine used by ComputeNestedDiffs. | |
| // It compares the given source and target values at the current nesting level | |
| // and returns a map of dot-notation paths to ValuePair instances for all | |
| // leaf-level differences found under the provided basePath. | |
| // | |
| // Parameters: | |
| // - basePath: the current dot-notation prefix representing the path to | |
| // source and target within the overall structure. For top-level calls | |
| // this is an empty string, and for nested map keys or slice indices it | |
| // is extended by flattenMapDiffs/flattenSliceDiffs (for example "a.b.0"). | |
| // - source: the source value at the current path segment; may be a map, | |
| // slice, primitive, or nil. | |
| // - target: the target value at the current path segment; may be a map, | |
| // slice, primitive, or nil. | |
| // | |
| // Return value: | |
| // A map keyed by full dot-notation paths (including basePath and any | |
| // nested segments) whose values are ValuePair instances representing all | |
| // leaf-level differences between source and target. When source and target | |
| // are equal (including both nil) an empty map is returned. | |
| // | |
| // Behavior: | |
| // - If one of source/target is nil or their types differ, a single entry | |
| // is recorded at basePath. | |
| // - If both are maps with string keys, comparison is delegated to | |
| // flattenMapDiffs, which recurses into each key. | |
| // - If both are slices, comparison is delegated to flattenSliceDiffs, | |
| // which recurses into each index. | |
| // - Otherwise, source and target are compared as primitives using | |
| // reflect.DeepEqual, and a diff is recorded at basePath when they differ. |
| return result | ||
| } | ||
|
|
||
| // flattenMapDiffs recursively compares two maps |
There was a problem hiding this comment.
The function flattenMapDiffs is missing documentation. It should have a doc comment explaining its parameters, return value, and behavior, especially noting that it handles keys present in either map and sorts them for consistent output.
| // flattenMapDiffs recursively compares two maps | |
| // flattenMapDiffs compares two map[string]any values and returns a flat map of | |
| // dot-notation paths to ValuePair entries for all differing leaf values. | |
| // | |
| // basePath is the current path prefix (using dot notation) under which keys in | |
| // sourceMap and targetMap will be resolved. sourceMap and targetMap are the two | |
| // maps being compared. | |
| // | |
| // The function computes the union of keys present in either map, sorts those | |
| // keys to ensure deterministic output order, and recursively invokes | |
| // flattenDiffs for each key. For keys that exist in only one of the maps, the | |
| // missing value is treated as nil in the resulting ValuePair. The returned map | |
| // is keyed by full dot-notation paths and contains only entries where the | |
| // source and target values differ. |
| return result | ||
| } | ||
|
|
||
| // flattenSliceDiffs recursively compares two slices |
There was a problem hiding this comment.
The function flattenSliceDiffs is missing documentation. It should have a doc comment explaining its parameters, return value, and behavior, especially noting how it handles slices of different lengths.
| // flattenSliceDiffs recursively compares two slices | |
| // flattenSliceDiffs recursively compares two slices and flattens their differences | |
| // into a map keyed by dot-notation paths. | |
| // | |
| // Parameters: | |
| // - basePath: the dot-notation path prefix for all indices in these slices. | |
| // - sourceSlice: the slice representing the "source" value. | |
| // - targetSlice: the slice representing the "target" value. | |
| // | |
| // Behavior: | |
| // - The function iterates up to the maximum length of the two slices. | |
| // - For indices beyond the end of either slice, the missing element is treated as nil. | |
| // - If both elements at an index are present and are themselves maps or slices, | |
| // the function recurses to compare their nested contents. | |
| // - If the elements differ (including one being nil while the other is not), | |
| // an entry is added to the result map with a key of the form | |
| // basePath.<index> and a ValuePair holding the source and target values. | |
| // | |
| // The returned map[string]ValuePair contains one entry for each leaf-level | |
| // difference between sourceSlice and targetSlice, with keys representing the | |
| // full path to the differing index. |
| return result | ||
| } | ||
|
|
||
| // buildPath constructs a dot-notation path |
There was a problem hiding this comment.
The function buildPath is missing documentation. It should have a doc comment explaining that it constructs dot-notation paths, with special handling for empty base paths.
| // buildPath constructs a dot-notation path | |
| // buildPath constructs a dot-notation path from a base path and key. | |
| // If the base path is empty, it returns the key without a leading dot. |
🔒 Scanned for secrets using gitleaks 8.28.0
fxenik
force-pushed
the
feature/dex-205-revamped-diff-reporting-of-plans-during-apply
branch
from
6130e4a to
b37350a
Compare
1 participant
This pull request introduces support for detailed, nested diff reporting when comparing complex resource structures. It adds a new experimental flag to enable this feature, implements recursive diffing logic for nested maps and arrays, and updates the plan reporter to display these granular differences. The changes also include comprehensive tests for both flat and nested diff modes.
Nested diff reporting support:
NestedDiffstoExperimentalConfigto enable detailed diff reports for nested structures.ComputeNestedDiffsfunction and supporting helpers incli/internal/syncer/reporters/diff.goto recursively compare and flatten differences between nested maps and arrays.Plan reporter enhancements:
cli/internal/syncer/reporters/plan.go) to use the nested diff logic when the experimental flag is enabled, displaying property diffs at the most granular changed path using dot notation. [1] [2]Type consistency and code modernization:
cli/internal/syncer/differ/diff.goto useanyinstead ofinterface{}for improved Go 1.18+ compatibility and consistency. [1] [2] [3] [4] [5]Testing improvements:
cli/internal/syncer/reporters/plan_test.goto cover both flat and nested diff output, including complex structures and property references. [1] [2] [3]These changes make it easier to understand exactly what has changed in deeply nested resource structures, improving the clarity and usefulness of diff reports with minimal impact on existing workflows.
🔒 Scanned for secrets using gitleaks 8.28.0