-
Notifications
You must be signed in to change notification settings - Fork 913
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 Support for Component Priority Class Configuration in Karmada Operator #6068
base: master
Are you sure you want to change the base?
Add Support for Component Priority Class Configuration in Karmada Operator #6068
Conversation
9d89807
to
9a8c510
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 #6068 +/- ##
==========================================
+ Coverage 48.33% 48.36% +0.02%
==========================================
Files 666 666
Lines 54858 54889 +31
==========================================
+ Hits 26518 26545 +27
- Misses 26616 26619 +3
- Partials 1724 1725 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/kind api-change |
thanks @jabellard Generally LGTM. This is merely a suggestion: for the fields in // Patcher defines multiple variables that need to be patched.
type Patcher struct {
labels map[string]string
... ...
resources corev1.ResourceRequirements
priorityClassName string
}
func (p *Patcher) WithPriorityClassName(priorityClassName string) *Patcher {
p.priorityClassName = priorityClassName
return p
}
func (p *Patcher) ForDeployment(deployment *appsv1.Deployment) {
deployment.Annotations = labels.Merge(deployment.Annotations, p.annotations)
deployment.Spec.Template.Annotations = labels.Merge(deployment.Spec.Template.Annotations, p.annotations)
deployment.Spec.Template.Spec.PriorityClassName = p.priorityClassName
... ...
}
func (p *Patcher) ForStatefulSet(sts *appsv1.StatefulSet) {
sts.Labels = labels.Merge(sts.Labels, p.labels)
sts.Spec.Template.Labels = labels.Merge(sts.Spec.Template.Labels, p.labels)
sts.Spec.Template.Spec.PriorityClassName = p.priorityClassName
... ...
} |
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.
/assign
Make sense to me. It seems we have a lesson learned after introducing a new configuration led to configuration chaos because failed to properly handle the order of the variables. |
That's a great idea. Will push some changes shortly to incorporate that. |
[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 |
@RainbowMango , @zhzhuang-zju: Thanks for the constructive feedback. Just pushed changes to address your moments. |
/lgtm |
@jabellard It would be nice to merge the commit |
…onents Signed-off-by: Joe Nathan Abellard <[email protected]>
385338b
to
954ee76
Compare
New changes are detected. LGTM label has been removed. |
Just squashed. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This is an implementation of this proposal to add support for configuring the priority class name of Karmada control plane components.
Which issue(s) this PR fixes:
Fixes #6009
Partof #6042
Special notes for your reviewer:
Does this PR introduce a user-facing change?: