Delete the propagation.karmada.io/instruction label #6512
Conversation
karmada-bot
added
the
kind/deprecation
karmada-bot
added
the
size/L
|
Hi @vie-serendipity @RainbowMango can you help take a review? |
|
@XiShanYongYe-Chang: GitHub didn't allow me to request PR reviews from the following users: vie-serendipity. Note that only karmada-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
f574b9c to
fb6b299
Compare
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #6512 +/- ##
==========================================
- Coverage 45.43% 45.41% -0.02%
==========================================
Files 687 687
Lines 56335 56296 -39
==========================================
- Hits 25596 25568 -28
+ Misses 29140 29132 -8
+ Partials 1599 1596 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
fb6b299 to
f574b9c
Compare
XiShanYongYe-Chang
changed the title
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
f574b9c to
2f2584b
Compare
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
2f2584b to
c43fcff
Compare
|
/lgtm |
|
@vie-serendipity: changing LGTM is restricted to collaborators In response to this:
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. |
|
/cc @RainbowMango |
|
/assign @RainbowMango |
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
c43fcff to
fc23cc5
Compare
|
[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 |
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the deprecated propagation.karmada.io/instruction label and related functionality that was used to suppress Work propagation. The functionality has been replaced by the spec.suspendDispatching field in the Work API.
- Removes the deprecated
PropagationInstructionandPropagationInstructionSuppressedconstants from util package - Eliminates checks for the deprecated label from predicate functions and controllers
- Updates tests to use
spec.suspendDispatchinginstead of the deprecated label - Renames and simplifies predicate functions to remove agent-specific variants
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/constants.go | Removes deprecated PropagationInstruction and PropagationInstructionSuppressed constants |
| pkg/util/helper/predicate.go | Removes deprecated label checks and eliminates NewExecutionPredicateOnAgent function |
| pkg/util/helper/predicate_test.go | Updates tests to use spec.suspendDispatching and removes deprecated label test cases |
| pkg/controllers/status/work_status_controller.go | Removes deprecated label check and renames predicate field |
| pkg/controllers/status/work_status_controller_test.go | Updates test configurations with renamed predicate field |
| pkg/controllers/execution/execution_controller.go | Renames predicate field from PredicateFunc to WorkPredicateFunc |
| pkg/controllers/ctrlutil/work.go | Removes conditional logic based on deprecated label |
| pkg/controllers/ctrlutil/work_test.go | Removes test case for deprecated label functionality |
| cmd/controller-manager/app/controllermanager.go | Updates controller initialization to use renamed predicate function |
| cmd/agent/app/agent.go | Removes predicate function assignment for agent controllers |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the deprecated propagation.karmada.io/instruction label and its associated logic. The renaming of functions and variables improves code clarity. A check for spec.suspendDispatching is needed in work_status_controller.go to prevent unintended resource recreation when dispatching is suspended.
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
fc23cc5 to
7ff8015
Compare
karmada-bot
added
the
do-not-merge/hold
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
7ff8015 to
c861bc5
Compare
|
/retest |
karmada-bot
removed
the
do-not-merge/hold
|
@gemini-code-assist |
There was a problem hiding this comment.
Code Review
The code changes introduce the removal of the deprecated propagation.karmada.io/instruction label. The changes are consistent across the codebase, and the renames of functions and fields like NewExecutionPredicate to WorkWithinPushClusterPredicate significantly improve clarity. There are some suggestions to improve the consistency of naming in the test files.
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
c861bc5 to
e914175
Compare
|
/retest |
cmd/agent/app/agent.go
Outdated
| InformerManager: genericmanager.GetInstance(), | ||
| WorkPredicateFunc: helper.AllowAllPredicate(), |
There was a problem hiding this comment.
I'm still feel not good to have a dummy event handler, it's waste for controller-runtime to filter events with a non-opt handler.
There was a problem hiding this comment.
We can refactor the SetupWithManager method in thecontroller like:
ctrlBuilder := controllerruntime.NewControllerManagedBy(mgr).
Named(ControllerName)
if c.WorkPredicateFunc != nil {
ctrlBuilder = ctrlBuilder.For(&workv1alpha1.Work{}, builder.WithPredicates(c.WorkPredicateFunc))
} else {
// In the case of this controller running in karmada-agent,
// usually no event filter (predicate) is needed.
ctrlBuilder = ctrlBuilder.For(&workv1alpha1.Work{})
}
return ctrlBuilder.
WithEventFilter(predicate.GenerationChangedPredicate{}).
WithOptions(controller.Options{
RateLimiter: ratelimiterflag.DefaultControllerRateLimiter[controllerruntime.Request](c.RateLimiterOptions),
}).
Complete(c)You no longer need to define or pass a dummy predicate function that always returns true. This reduces boilerplate and potential confusion.
Signed-off-by: changzhen <[email protected]>
XiShanYongYe-Chang
force-pushed
the
remove-instruction-label
branch
from
e914175 to
027741b
Compare
What type of PR is this?
/kind deprecation
What this PR does / why we need it:
ref #6437
Which issue(s) this PR fixes:
Fixes #6437
Special notes for your reviewer:
Does this PR introduce a user-facing change?: