-
Notifications
You must be signed in to change notification settings - Fork 830
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
WIP: Hacking on webhook int64-ness #3608
Conversation
Build Failed 😱 Build Id: 2d4beaf9-faa7-4917-8294-424c96a6ba43 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 4a6ef67b-d244-456a-b977-0c5c981cb3bf To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
|
||
patch, err := jsonpatch.CreatePatch(obj.Raw, newGS) | ||
jsonPatch, err := jsonpatch.CreateMergePatch(obj.Raw, newGS) |
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.
So the issue here I think, is that this type of patch isn't supported.
This creates a https://datatracker.ietf.org/doc/html/rfc7396 style merge patch, whereas AdmissionReviews can only handle https://jsonpatch.com/ types of JSON Patches.
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.
So I did a bit more digging, and I wondered "what does controller-runtime
use, because they have the same problem!
Which is: https://github.com/gomodules/jsonpatch
So in theory, should be a drop in replacement without any code changes other than go.mod
Just checking in if you were going to finish this up? Or should we turn this into an issue and close out the PR to be tackled at a later date? |
I think we should abandon it. I'm not even sure it's worth an issue, and here's why: JSON support for high int64 values seems fussy at best. If I had this much trouble in golang, there are probably other languages where it wouldn't translate well, either, which is concerning. :/ |
WIP - just putting it up for debugging