-
Notifications
You must be signed in to change notification settings - Fork 876
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
add e2e test for lazy propagation policy #4804
Conversation
Welcome @hulizhe! It looks like this is your first PR to karmada-io/karmada 🎉 |
/assign @chaosi-zju |
this PR is modified upon #4740, the review can wait until this PR is merged in. |
9f36b9d
to
214171e
Compare
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.
@hulizhe Hello, thanks for your important commit! Review is ongoing, and my viewpoint just for reference only.
others lgtm!
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 your excellent work! my viewpoint just for reference only.
/LGTM you did great job, thank you for your efffort ! |
/lgtm cc @XiShanYongYe-Chang |
@@ -90,6 +103,92 @@ var _ = ginkgo.Describe("Lazy activation policy testing", func() { | |||
}) | |||
}) | |||
|
|||
// Simple Case 3 (Lazy to immediate) | |||
// refer: https://github.com/karmada-io/karmada/blob/master/docs/proposals/scheduling/activation-preference/lazy-activation-preference.md#simple-case-3-lazy-to-immediate |
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.
Here it's best not to use the master branch, you should use a fixed commit ID instead.
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.
// refer: https://github.com/karmada-io/karmada/blob/master/docs/proposals/scheduling/activation-preference/lazy-activation-preference.md#simple-case-3-lazy-to-immediate | |
// refer: https://github.com/karmada-io/karmada/blob/fdad87efce1d088c8002856f5ee7586427f1a989/docs/proposals/scheduling/activation-preference/lazy-activation-preference.md?plain=1#L263 |
@chaunceyjiang Is it like this? The new link loses information such as the tag name, leaving only the number of rows. And the original link can find the title according to the tag name, which is relatively more flexible. So I'm more inclined not to modify. What do you think?
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.
The code in the master branch may change, which could cause the referenced content to become invalid.
Signed-off-by: hulizhe <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4804 +/- ##
==========================================
+ Coverage 51.75% 53.11% +1.35%
==========================================
Files 250 250
Lines 24964 20351 -4613
==========================================
- Hits 12920 10809 -2111
+ Misses 11335 8825 -2510
- Partials 709 717 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@XiShanYongYe-Chang @zhzhuang-zju @chaosi-zju @chaunceyjiang Thanks for the code review, it helped me a lot! |
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
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~
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XiShanYongYe-Chang 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 |
What type of PR is this?
/kind failing-test
What this PR does / why we need it:
add e2e test for lazy propagation policy
Involving:
Simple Case 3 (Lazy to immediate)
Simple Case 4 (Immediate to lazy)
Combined Case 4 (Policy preemption)
Which issue(s) this PR fixes:
part of #4607
Special notes for your reviewer:
Does this PR introduce a user-facing change?: