-
Notifications
You must be signed in to change notification settings - Fork 1.2k
🐛Use k8s.io/apimachinery/pkg/util/json to unmarshal in fakeclient #3208
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?
🐛Use k8s.io/apimachinery/pkg/util/json to unmarshal in fakeclient #3208
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: troy0820 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 |
Signed-off-by: Troy Connor <[email protected]>
43929d1
to
dde4486
Compare
go.mod
Outdated
@@ -32,6 +32,8 @@ require ( | |||
sigs.k8s.io/yaml v1.4.0 | |||
) | |||
|
|||
require sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 |
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.
Could it be in with the others ones?
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, this is actually in k8s.io/apimachinery/pkg/util/json
which has an Unmarshal
method. That method calls the method UnmarshalCaseSensitivePreserveInts
Signed-off-by: Troy Connor <[email protected]>
73e4dac
to
a4ac626
Compare
@@ -58,6 +57,7 @@ import ( | |||
"k8s.io/apimachinery/pkg/runtime" | |||
"k8s.io/apimachinery/pkg/runtime/schema" | |||
"k8s.io/apimachinery/pkg/types" | |||
"k8s.io/apimachinery/pkg/util/json" |
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.
Lets please add a test for the issue, otherwise this will likely break again in the future
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 might be mixing up stuff]
IIRC this is heavily aligned to the upstream client-go fake client and we want to avoid differences in behavior.
What is the upstream fake client doing here? (or is it not using the json package at all and this is just from additional code that we have here)
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.
IIRC this is heavily aligned to the upstream client-go fake client and we want to avoid differences in behavior.
What is the upstream fake client doing here? (or is it not using the json package at all and this is just from additional code that we have here)
I am not seeing anything in client-go fakeclient where this would be present. I may be overlooking it but it seems they use k8s.io/apimachinery/pkg/util/json
in client-go but not for the fakeclient. They are using instances of encoding/json
to Marshal for the ApplyConfigurations.
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.
@sbueringer I think you are confusing this with json patch, I don't think there is any reason for upstream to do json encoding in the fake client. We need it because of edge cases like "Object gets stored and retrieved in different representations like unstructured/structure", that issue does not exist in upstream fake clients as they don't support that
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.
Sounds good, thx for the additional info!
I know that it's not the json patch case, was just wondering if it's a similar case
/hold |
This will use
k8s.io/apimachinery/pkg/util/json
which callssigs.k8s.io/json
toUnmarshalWithCaseSensitivePreserveInts
in the fakeclient.Resolves #3207
NOTE: https://pkg.go.dev/sigs.k8s.io/json#UnmarshalCaseSensitivePreserveInts