-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
✨ Fix roundtripping issue when genearting JSON patch #251
✨ Fix roundtripping issue when genearting JSON patch #251
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mengqiy If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cc: @FillZpp |
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 have one suggestion.
// If the original raw object is provided, it will be used to calculate json patch. | ||
// It is STRONGLY recommended to use the original raw object to avoid the roundtripping | ||
// issue for non-pointer fields. | ||
func NewJSONPatch(original, current runtime.Object, originalRaw ...byte) ([]jsonpatch.JsonPatchOperation, error) { |
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.
Since it is STRONGLY recommended to provide originalRaw, it will help to not to treat this field optional. I know that will introduce a breaking change, but I think thats ok (we can include this for next major release). Also using originalRaw...
is also not intuitive (I know its clever because it keeps this change backward compatible) when you expect a slice of bytes.
So my recommendation would be to introduce it with breaking change with originalRaw []byte
as the last arg.
Closing in favor of #256. |
Add annotation tests into test projects
fixes kubernetes-sigs/kubebuilder#510
The change should be backward compatible
An alternative is in #256.