Allow resources to have additional properties #1558
Conversation
theunrepentantgeek
requested review from
babbageclunk,
davefellows,
matthchr and
Porges
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1558 +/- ##
==========================================
+ Coverage 63.38% 63.47% +0.09%
==========================================
Files 176 176
Lines 11670 11694 +24
==========================================
+ Hits 7397 7423 +26
+ Misses 3609 3607 -2
Partials 664 664
Continue to review full report at Codecov.
|
| statusProperty := NewPropertyDefinition("Status", "status", resource.status). | ||
| WithTag("json", "omitempty") | ||
| result = append(result, statusProperty) | ||
| result = append(result, resource.createStatusProperty()) | ||
| } | ||
|
|
||
| return result |
There was a problem hiding this comment.
Shouldn't Properties() return the non-spec/status properties too?
There was a problem hiding this comment.
And thus we prove that I need to write the test even for code I think I can't possibly get wrong! 😊
theunrepentantgeek
force-pushed
the
feature/resource-properties
branch
from
d190653 to
65a3f4e
Compare
| @@ -24,6 +24,7 @@ type ResourceType struct { | |||
| status Type | |||
| isStorageVersion bool | |||
| owner *TypeName | |||
| properties map[PropertyName]*PropertyDefinition | |||
There was a problem hiding this comment.
@theunrepentantgeek - Sorry I missed looking at this PR on my Friday, but why do we need this change for the storage versions? The property doesn't have to be on the top level resource type I don't think. It really should be in spec in the storage version too, shouldn't it?
Maybe we can sync up offline or in the meeting today?
What this PR does / why we need it:
Currently resources only have
SpecandStatusproperties, but we have a need (for storage versions) to have additional properties as well. This PR extends support to allow additional properties on resources, but treatingSpecandStatusas special cases.First consumer will be for Storage versions where the original group-version-kind of the API type needs to be preserved.
How does this PR make you feel:
If applicable: