Skip to content
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

replace StrategicMergePatchType with MergePatchType #2694

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Conversation

hongzhen-ma
Copy link
Collaborator

@hongzhen-ma hongzhen-ma commented Apr 21, 2023

What type of this PR

Which issue(s) this PR fixes:

Fixes #2186

WHAT

🤖 Generated by Copilot at 82ebe2d

Improve pod annotation update logic in pod.go by using merge patch and enhancing error logging.

🤖 Generated by Copilot at 82ebe2d

Oh we patch the pods with merge patch method
Heave away, me hearties, heave away
We avoid the conflicts and log the errors well
Heave away, me hearties, heave away

HOW

🤖 Generated by Copilot at 82ebe2d

  • Use merge patch instead of strategic merge patch to update pod annotations in reconcileAllocateSubnets and reconcileRouteSubnets functions to avoid conflicts with other controllers (link, link)
  • Add log message for patch generation error in reconcileAllocateSubnets and reconcileRouteSubnets functions (link, link)

@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative.
  • There is a potential bug in the code. If GenerateMergePatchPayload returns an error, the function will return nil without cleaning up the resources allocated by AllocateSubnets. This could lead to resource leaks. A better approach would be to defer a call to ReleaseSubnets before returning from the function.
  • There is a potential performance issue in the code. The function calls GenerateMergePatchPayload twice, which could be expensive if the pod object is large. It would be better to generate the patch once and reuse it for both patches.
  • The format of the code diff is not consistent. Some lines have trailing spaces while others don't. It would be better to remove all trailing spaces to improve readability.

@hongzhen-ma hongzhen-ma merged commit 9e3f70c into master Apr 21, 2023
@hongzhen-ma hongzhen-ma deleted the patch branch April 21, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants