-
Notifications
You must be signed in to change notification settings - Fork 283
fix: Ensure NillableDuration can round-trip with unstructured #2192
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
base: main
Are you sure you want to change the base?
fix: Ensure NillableDuration can round-trip with unstructured #2192
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adammw The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @adammw! |
Hi @adammw. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3301ba3
to
5531f34
Compare
Pull Request Test Coverage Report for Build 14830014839Details
💛 - Coveralls |
Maybe I'm missing something but when does this actually happen? |
We encounter this problem using any of the Karpenter API types that include the NIllableDuration in our internal project that manages the nodepool resources for us. Since the API type claims to support the FromUnstructured/ToUnstructured interface, the suggestion was that the fix should live upstream where those methods are defined. |
/assign @rschalo |
Can you please give an example? Is it something that is being set by the client managing the NodePool? What does the duration and NodePool look like when it fails? |
An example from the test case which will fail with the following without the implementation changes:
returns error
with a valid duration of 30s set. Unstructured conversions are used often to write generic helper methods, we use them to detect and save changes back to Kubernetes in our controller. However without custom handling of UnmarshalJSON, the above error occurs from the byte array being marshalled to base64 and therefore cannot be unmashalled as a time duration. |
@@ -52,6 +53,17 @@ func (d *NillableDuration) UnmarshalJSON(b []byte) error { | |||
if err != nil { | |||
return err | |||
} | |||
|
|||
// check for base64 encoded string, can happen from unstructuredConverter marshaling []byte | |||
base64Decoded, err := base64.StdEncoding.DecodeString(str) |
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 the issue that we need to do a base64 decode here or that we just need to return the string representation of the byte array on L82 -- or maybe better yet, just store the string representation in Raw?
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.
Happy to try alternative implementations - my understanding was that FromUnstructured is hard coded to call MarshalJSON on the []byte
(rather than it being on string) which causes the base64 encoding.
Description
Ensure NillableDuration can round-trip with default unstructured converter.
Previously this failed with "invalid duration" and a base64-encoded duration.
How was this change tested?
Test added to round-trip through the default unstructured converter.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.