Fix bugs in OneOf to Object conversion #4555
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
theunrepentantgeek
requested review from
davefellows,
matthchr,
babbageclunk and
super-harsh
as code owners
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
matthchr
approved these changes
Feb 3, 2025
v2/tools/generator/internal/codegen/pipeline/convert_allof_and_oneof_to_objects.go
Outdated
Show resolved
Hide resolved
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
Fixes a few small errors that I uncovered while troubleshooting issues with Kusto resources.
Leaf Merging
Previously we've always merged object properties into the root of a one-of, then pushed them down to the leaves. Due to the way Kusto resources are structured, we now also need to support merging object properties directly into leaves. (Instead of embedding a OneOf directly into an AllOff, the Kusto resources instead refer to a separate type definition, triggering the change in behaviour.)
Avoid lost updates
In two locations, the synthesizer was looking up an unmodified type definition when that type had already been modified, resulting in the previous updates being lost. All type lookup now goes through a helper method (
lookupType) that avoids this propblem.OneOf Equality
We hadn't updated equality of one-of types, causing lost object updates when additional properties were added. (Two one-of objects, one containing three object-property types, the other four, were comparing as equal.)
Special notes
Also updates CRD documentation for ContainerRegistry - now that documentation has been merged, the code generator has introduced linkes.
How does this PR make you feel?
Checklist