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

fix: use update instead of patch to set pod's role #8865

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

xuriwuyun
Copy link
Contributor

No description provided.

@xuriwuyun xuriwuyun requested a review from a team as a code owner January 23, 2025 10:55
@github-actions github-actions bot added the size/XS Denotes a PR that changes 0-9 lines. label Jan 23, 2025
@github-actions github-actions bot added size/S Denotes a PR that changes 10-29 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Jan 23, 2025
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 60.35%. Comparing base (851f1b8) to head (eba4b65).

Files with missing lines Patch % Lines
...g/controller/instanceset/pod_role_event_handler.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8865      +/-   ##
==========================================
+ Coverage   60.16%   60.35%   +0.19%     
==========================================
  Files         382      382              
  Lines       46293    46293              
==========================================
+ Hits        27850    27938      +88     
+ Misses      15771    15688      -83     
+ Partials     2672     2667       -5     
Flag Coverage Δ
unittests 60.35% <85.71%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added size/M Denotes a PR that changes 30-99 lines. and removed size/S Denotes a PR that changes 10-29 lines. labels Jan 24, 2025
@apecloud-bot apecloud-bot added the approved PR Approved Test label Jan 24, 2025
@zjx20 zjx20 changed the title fix: use update instand of patch to set pod's role fix: use update instead of patch to set pod's role Jan 24, 2025
@@ -123,6 +123,44 @@ var _ = Describe("pod role label event handler test", func() {
return nil
}).Times(1)
Expect(handler.Handle(cli, reqCtx, nil, event)).Should(Succeed())

By("read a stale pod")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only explicitly mocked update conflicts are present, not a stale read scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, such test cases have no practical significance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved PR Approved Test size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
4 participants