-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create conversion graph #1627
Create conversion graph #1627
Conversation
33880e9
to
4c39ccf
Compare
1c75733
to
fa329cf
Compare
b86d27f
to
cf5198b
Compare
b2b1ec5
to
60d27fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great mostly but I think the graph creation stage needs to deal with types from more than one group. (Although I could be wrong!)
for i, ref := range sortedApiReferences { | ||
if i == 0 || !ref.IsPreview() { | ||
continue | ||
} | ||
|
||
conversionGraph.AddLink(sortedStorageReferences[i], sortedStorageReferences[i-1]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this part. Don't the references need to be split by group before you can do this? Otherwise you'll get the earliest version of one group pointing at the latest version of the previous group - something like microsoft.network/v20150101
pointing to microsoft.compute/v20210701
. In my head each stage runs with the full set of loaded types. Is that right? Or is there something structural that means that the stage only runs with (all versions of) the types from one group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're perfectly correct - I transplanted this code from #1533 and put it in the wrong place. 😲
I'll relocate it into GroupConversionGraph
and add some tests to ensure it behaves correctly.
@@ -3,10 +3,20 @@ | |||
// Licensed under the MIT license. | |||
package v20211231 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the package have changed? Or is the destination package an input to the code that this test is checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll double check this after moving the graph creation code; when I reviewed these changes prior to creating the PR I was sure they were correct, but I'm no longer convinced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have no longer changed - I fixed a couple of bugs in the test. Good catch.
type Person struct { | ||
metav1.TypeMeta `json:",inline"` | ||
metav1.ObjectMeta `json:"metadata,omitempty"` | ||
Spec Person_Spec `json:"spec,omitempty"` | ||
Status Person_Status `json:"status,omitempty"` | ||
} | ||
|
||
// AssignPropertiesFromPerson populates our Person from the provided source Person |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did these go away? Aren't they still needed for conversion to storage versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have no longer changed either.
df32912
to
e9e04cf
Compare
Codecov Report
@@ Coverage Diff @@
## master #1627 +/- ##
==========================================
+ Coverage 67.04% 67.22% +0.17%
==========================================
Files 205 208 +3
Lines 14898 14979 +81
==========================================
+ Hits 9989 10069 +80
+ Misses 4152 4151 -1
- Partials 757 759 +2
Continue to review full report at Codecov.
|
3520a61
to
9a876ce
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - some minor comments.
person2022 := test.CreateSpec(test.Pkg2022, "Person", test.FullNameProperty, test.KnownAsProperty, test.FamilyNameProperty) | ||
|
||
types := make(astmodel.Types) | ||
types.AddAll(person2020, person2021, person2022) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth having some unrelated types in here to make sure there aren't any links created between Person and OtherType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted for a followup PR.
createConversionGraphStage := CreateConversionGraph() | ||
state, err := createConversionGraphStage.Run(context.TODO(), state) | ||
g.Expect(err).To(Succeed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be better done as just constructing the intended conversion graph, rather than calling the stage to build it. The current way makes it an integration test for the interaction between the three stages (CreateConversionGraph, CreateStorageTypes and InjectFunctions) . That's good for making sure that they all work together, but makes it hard to pinpoint the problem if it fails. I can see that it would be hard to create the storage types manually, but I think the graph should be reasonably easy to build by hand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a problem occurs with an earlier stage, the tests on that stage should flag it. I tried manually creating the required state but the amount of code required was prohibitive and it didn't seem to provide any value - in fact, it obscured what the test was doing. I have later changes (different PRs) to simplify the tests further.
hack/generator/pkg/codegen/storage/group_conversion_graph_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Muirhead <christian.muirhead@microsoft.com>
What this PR does / why we need it:
Adds a pipeline stage that computes the conversion graph required for our storage versioning.
Prerequisites
master
How does this PR make you feel:
![gif](https://camo.githubusercontent.com/267469361ebe42b96c9c10a167d180288bde09a9cdc850ae109445ef5ff3995a/68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f316954484852384b61694479686b48652f67697068792e676966)
If applicable: