Remap image_pull_policy field on Container from ImagePullPolicy enum to str
#877
Conversation
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
flaviuvadan
added
semver:patch
|
Turns out autogen removes the |
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Fixed |
|
@vikramsg FYI ^ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #877 +/- ##
=====================================
Coverage 79.6% 79.6%
=====================================
Files 45 45
Lines 3728 3728
Branches 758 758
=====================================
Hits 2970 2970
Misses 563 563
Partials 195 195 ☔ View full report in Codecov by Sentry. |
| # { object name: { field name: ( ( existing field, existing value ) , ( new field, new value ) ) } } | ||
| INT_OR_STRING_FIELD_REMAPPING: Dict[str, Dict[str, Tuple[Tuple[str, str], Tuple[str, str]]]] = { | ||
| FIELD_REMAPPINGS: Dict[ | ||
| str, Dict[str, Tuple[Tuple[str, Union[str, List[str]]], Optional[Tuple[str, Union[str, List[str]]]]]] |
There was a problem hiding this comment.
Not sure the type is helping at this point 😅 The comment is helpful enough, and longer term maybe we should create a helper class within this script?
There was a problem hiding this comment.
Yea... I was very reluctant to add this type, especially as I realized it's growing. Yea, we'll have to refactor this a bit in the future.
| "io.k8s.api.core.v1.Container": { | ||
| "imagePullPolicy": ( | ||
| ("enum", None), | ||
| None, |
There was a problem hiding this comment.
What's this None doing here? 🤔
There was a problem hiding this comment.
First part of the tuple says "original field, original field value" second one says "new field, new field value". New field value of None means delete the original field. So, this is removing the enum from Container, allowing codegen to put a string in place of the Container image pull policy enum, and stop generating the ImagePullPolicy enum altogether.
Later in the script you see we have to add a specification for IPP manually for otherwise it's not fully backwards compatible, even if it's not used by Container!
There was a problem hiding this comment.
Ah of course, I was misreading the outer tuple. Makes sense (almost!), though I would suggest a refactor to helper classes if you have time/want to in a separate PR
Pull Request Checklist
Description of PR
When an autogenerated object is created through the service the service returns auto generated objects. This means the services relies on autogenerated code only. #784 documents how creating an autogen
Workflowobject fails because the value ofAlwaysdoes not fit theTemplate.container.imagePullPolicyfield. This happens because the returned value is of typestr(from the Argo Server) while the Python value is an enum of typeImagePullPolicy. This is caused by an inconsistency in the OpenAPI specification. Some fields usestrfor image pull policy (look at Events, User Container, etc) while Container usesImagePullPolicy. This PR changes the field in the OpenAPI schema specification so that this Container field is also astr. This should solve #784. Our Pydantic configuration is set to use enum values, which means that users who rely on the autogen Container object and pass the ImagePullPolicy object in will have Pydantic automatically extract the.valuefield of that enum. The Hera objects already accommodate both str and ImagePullPolicy, so no breaking change there.