Add support for diffing field sets #12330
Conversation
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
|
@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
There was a problem hiding this comment.
This feels a lot simpler than I remember the previous round being. I think it could still be simplified a bit more - notes below.
Please also add additional tests to cover all the cases. I'd recommend specifically adding new test cases that only cover the behavior of this new functionality (but test it thoroughly) rather than modifying existing test cases, so that we're not testing too many things in each test case.
tools/diff-processor/diff/diff.go
Outdated
| } | ||
| if fieldDiff, fieldSetsDiff, changed := diffFields(oldField, newField, key); changed { | ||
| resourceDiff.Fields[key] = fieldDiff | ||
| resourceDiff.FieldSets = mergeFieldSetsDiff(fieldSetsDiff, resourceDiff.FieldSets) |
There was a problem hiding this comment.
I think you could make this implementation a bit more efficient (and clearer) by using an intermediate structure here in ComputeSchemaDiff. For example, something like (ignoring whitespace issues):
for resource := range union(oldResourceMap, newResourceMap) {
conflictsWithSets := map[string][]string
// etc...
if fieldDiff, fieldSetsDiff, changed := diffFields(oldField, newField, key); changed {
conflictsWithSets[setKey(fieldSetsDiff.Old.ConflictsWith)] = fieldSetsDiff.Old.ConflictsWith
// then at the end of the loop
resourceDiff.FieldSets = ResourceFieldSets{
ConflictsWith: maps.Values(conflictsWithSets)
// etc...
}Right now you're keeping fieldSets in the fieldSet structure from beginning to end, even though that means you need to convert them to a map of key: fieldSet every time a new fieldset is changed, to do the comparison. Instead, you could track them directly in a key: value map in the ComputeSchemaDiff function so that the key only has to be computed once per field, then convert those mappings to the correct output structure once per resource.
There's also a question of what the output structure should be. My instinct would be to have each type of fieldset be [][]string of the field names (minus zero padding?)... But I don't care strongly about this since (assuming it's built once per resource) it would be relatively easy to change later if we want to (and I don't have a strong idea in my head of what will be most useful / easiest to understand in the breaking change detector.)
tools/diff-processor/diff/diff.go
Outdated
| func mergeFieldSetsDiff(a, b ResourceFieldSetsDiff) ResourceFieldSetsDiff { | ||
| a.Old = mergeResourceFieldSets(a.Old, b.Old) | ||
| a.New = mergeResourceFieldSets(a.New, b.New) | ||
| return a | ||
| } | ||
|
|
||
| func mergeResourceFieldSets(a, b ResourceFieldSets) ResourceFieldSets { | ||
| a.ConflictsWith = mergeFieldSets(a.ConflictsWith, b.ConflictsWith) | ||
| a.ExactlyOneOf = mergeFieldSets(a.ExactlyOneOf, b.ExactlyOneOf) | ||
| a.AtLeastOneOf = mergeFieldSets(a.AtLeastOneOf, b.AtLeastOneOf) | ||
| a.RequiredWith = mergeFieldSets(a.RequiredWith, b.RequiredWith) | ||
| return a | ||
| } | ||
|
|
||
| func mergeFieldSets(a, b []FieldSet) []FieldSet { | ||
| keys := make(map[string]struct{}) | ||
| for _, set := range a { | ||
| slice := setToSortedSlice(set) | ||
| key := strings.Join(slice, ",") | ||
| keys[key] = struct{}{} | ||
| } | ||
| for _, set := range b { | ||
| slice := setToSortedSlice(set) | ||
| key := strings.Join(slice, ",") | ||
| if _, ok := keys[key]; ok { | ||
| continue | ||
| } | ||
| keys[key] = struct{}{} | ||
| a = append(a, set) | ||
| } | ||
| return a | ||
| } |
There was a problem hiding this comment.
just as a style note, these functions end up being hard to read because they are too generic and because (relatedly) the variable names are just a and b. Trying to read through them quickly, it's easy to lose track of what a and b represent. I'm not sure these functions will still be around in the next version of this PR but - just something to keep in mind.
|
@trodge, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
|
@trodge, this PR is waiting for action from you. If no action is taken, this PR will be closed in 14 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
|
@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @melinath This PR has been waiting for review for 1 week. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @melinath This PR has been waiting for review for 2 weeks. Please take a look! Use the label |
There was a problem hiding this comment.
It looks like the unit tests are failing: https://github.com/GoogleCloudPlatform/magic-modules/actions/runs/12508694806/job/34897154818?pr=12330#step:5:472
| } | ||
|
|
||
| // field1.0.field2 -> field1.field2 | ||
| func removeZeroPadding(zeroPadded string) string { |
There was a problem hiding this comment.
Please add unit tests for this function.
| return trimmed[:len(trimmed)-1] | ||
| } | ||
|
|
||
| func setToSortedSlice(set map[string]struct{}) []string { |
There was a problem hiding this comment.
nit: I'm not sure it makes sense to split this out into a separate function since it's only used once. Not a blocker though.
|
@trodge, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR hasn't generated any diffs, but I'll let you know if a future commit does. |
This is a less complicated approach to diffing conflict sets, which can be used to detect breaking changes in
ExactlyOneOforAtLeastOneOf.Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.