-
Notifications
You must be signed in to change notification settings - Fork 299
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
Use Patch instead of Update for k8s Service client #1127
Conversation
Hi @skmatti. Thanks for your PR. I'm waiting for a kubernetes 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/test-infra repository. |
pkg/utils/common/common.go
Outdated
func PatchServiceLoadBalancerStatus(client coreclient.CoreV1Interface, svc *corev1.Service, newStatus corev1.LoadBalancerStatus) error { | ||
newSvc := svc.DeepCopy() | ||
newSvc.Status.LoadBalancer = newStatus | ||
_, err := svchelpers.PatchService(client, svc, newSvc) |
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.
Thanks for reusing this!
/ok-to-test |
All the `update` calls are removed in Ingress-GCE in kubernetes/ingress-gce#1125 and kubernetes/ingress-gce#1127. Also, adds permissions for frontendconfig CRD.
This removes `update` permissions for Pod, Service and Ingress resources. All the `update` calls are removed in Ingress-GCE in kubernetes#1125 and kubernetes#1127.
d607135
to
8ce841f
Compare
Having both utils/common and utils/ doesn't make sense to me. |
Its because we will run into an import cycle if not. Both Namer and Finalizer workflows are in utils. I will move those out of utils and fix the import cycle in a followup PR. |
I have used a different package We created |
I think the only thing that really causes the issue is the import of utils/namer from utils/, which is just one file. If we move that out somewhere else, does that resolve the issue/ |
This is the line that references namer ( ingress-gce/pkg/utils/serviceport.go Line 29 in 6f08bb6
If I move this to say serviceport package, the import cycle would beserviceport -> utils/namer -> utils -> serviceport because of this( ingress-gce/pkg/utils/utils.go Line 434 in 6f08bb6
Same with finalizer workflow because of this ( ingress-gce/pkg/utils/utils.go Line 465 in 6f08bb6
|
This removes `update` permissions for Pod, Service and Ingress resources. All the `update` calls are removed in Ingress-GCE in kubernetes#1125 and kubernetes#1127.
@bowei Can you review this again?. Maybe if its better, I can isolate the refactoring into a separate PR so that the change is small. |
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.
can you rebase?
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, prameshj, skmatti 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 |
This replaces of all instances of update calls of k8s Service resource from this controller.
Also, this uses k8s service helper function to patch Service.
/assign @freehan @prameshj