-
Notifications
You must be signed in to change notification settings - Fork 111
✨ Allow RFC6902 patches and strategic merge patches for provider manifests #937
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?
✨ Allow RFC6902 patches and strategic merge patches for provider manifests #937
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 @hjoshi123! |
|
Hi @hjoshi123. Thanks for your PR. I'm waiting for a github.com 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. |
✅ Deploy Preview for kubernetes-sigs-cluster-api-operator ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
5fbe651 to
06a4c41
Compare
nalum
left a comment
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.
Looking good, just a few comments.
One question I have on the merge patch is around the requirement for the apiVersion key. I know you don't likely have an answer for the reasoning behind that but is that still the case for the new patching setup?
| patches: []*operatorv1.Patch{ | ||
| { | ||
| Patch: addServiceAccoungPatchRBAC, | ||
| Target: &operatorv1.PatchSelector{ | ||
| Group: "rbac.authorization.k8s.io", | ||
| Kind: "ClusterRoleBinding", | ||
| }, | ||
| }, |
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.
This looks like it will fail the test based on the existing test.
I would update this to use it's own set of patch data as you want to test all patch types that this supports.
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 I am writing more tests. The one I added was just a basic test to see if merge patch works.. and I ran the tests in local and it does patch and tests pass.. I will add more for RFC6902 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.
I did add a test regarding jsonpatch and it passed.. let me know if the code looks good.
|
Should also update the patching provider manifests page. |
Signed-off-by: Hemant Joshi <[email protected]>
06a4c41 to
3834a50
Compare
What this PR does / why we need it:
Allow JSON merge patches and RFC6902 for provider manifests, this makes it possible to modify provider manifests consistently across releases, for example, if the user needs to change labels/annotations on objects, example usage:
This still uses the same patch library. This PR doesn't remove the existing field but introduces a new one and in the future versions of the operator we can deprecate the existing
manifestPatchesfield in favor of a more genericpatchesfield.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #792