-
Notifications
You must be signed in to change notification settings - Fork 203
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
Persist original ARM API version for use by generic controller #1606
Conversation
5e16ac9
to
3f62792
Compare
Codecov Report
@@ Coverage Diff @@
## master #1606 +/- ##
==========================================
+ Coverage 62.92% 64.82% +1.89%
==========================================
Files 187 200 +13
Lines 12359 12542 +183
==========================================
+ Hits 7777 8130 +353
+ Misses 3897 3702 -195
- Partials 685 710 +25
Continue to review full report at Codecov.
|
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.
Not done yet but posting what I have
...en/storage/testdata/Test_StorageTypeFactory_GeneratesExpectedResults-v20211231storage.golden
Show resolved
Hide resolved
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, although I think the change you discussed of breaking StorageTypeFactory into a set of pipeline stages will be nicer.
...en/storage/testdata/Test_StorageTypeFactory_GeneratesExpectedResults-v20211231storage.golden
Show resolved
Hide resolved
f36dd42
to
d2da249
Compare
I've majorly restructured the PR, so this approval is stale.
746553c
to
0bc3f8a
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.
LGTM - Left a bunch of nits and one comment about golden test file structure.
@@ -151,7 +156,14 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration | |||
pipeline.AddCrossplaneEmbeddedResourceSpec(idFactory).UsedFor(pipeline.CrossplaneTarget), | |||
pipeline.AddCrossplaneEmbeddedResourceStatus(idFactory).UsedFor(pipeline.CrossplaneTarget), | |||
|
|||
pipeline.CreateStorageTypes(idFactory).UsedFor(pipeline.ARMTarget), // TODO: For now only used for ARM | |||
// Create Storage types | |||
//TODO: For now only used for ARM |
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.
extra mega minor:
//TODO: For now only used for ARM | |
// TODO: For now only used for ARM |
.../pkg/codegen/pipeline/testdata/TestInjectPropertyAssignmentFunctions-v20211231storage.golden
Show resolved
Hide resolved
type OriginalVersionKind string | ||
|
||
const ( | ||
ReadProperty = OriginalVersionKind("property") |
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 hate that go doesn't have first class enum support, and the solution of consts kinda works but if the consts don't have the enum name in them it's sometimes hard to pick out which values belong to which type.
I know in some places we do something like OriginalVersionKindReadProperty
(which is sorta how you'd do it in C, I think?), but we don't have that consistently and it is awkwardly long.
Does this bug other folks too? What do people prefer?
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 used very simple names here to optimize the API when consuming; adding a unique suffix would make that clunky.
Having first class enum support would be preferable - but I guess Go is designed to push people towards many simpler packages.
74e3f6f
to
3be33df
Compare
What this PR does / why we need it:
Adds
OriginalVersion()
function to API Spec types to return the actual ARM API version requested by users.Adds
OriginalVersion
property to Storage Spec types to store that version for later use.Adds
OriginalGVK()
function to Storage Resource types to return the GVK of the original version.Removes
StorageTypeFactory
in favour of separate pipeline sectionsAlso includes
OriginalVersion()
function generationOriginalGVK()
function generationErroredType
whereWriteDebugDescription()
would crash if there was no wrapped typeHow does this PR make you feel:
If applicable: