-
Notifications
You must be signed in to change notification settings - Fork 919
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
charts/karmada: fix the creation condition of metrics-adapter related APIService #5378
Conversation
32c37fe
to
5ffd9b2
Compare
/assign @chaosi-zju |
great finding, I 've seen this error before, but I haven't gone into the cause. |
@@ -32,6 +32,7 @@ metadata: | |||
spec: | |||
type: ExternalName | |||
externalName: {{ $name }}-aggregated-apiserver.{{ include "karmada.namespace" . }}.svc.{{ .Values.clusterDomain }} | |||
{{- if has "metricsAdapter" .Values.components }} |
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.
I think it might be in the wrong position.
The outter condition is {{- if eq .Values.installMode "host" }}
, then if my installMode is component, it will not run into this line?
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.
I think this block should be on the same level as the below block {{- if has "search" .Values.components }}
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.
Nice catch! Fixed.
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 #5378 +/- ##
=======================================
Coverage 29.40% 29.40%
=======================================
Files 632 632
Lines 43835 43835
=======================================
+ Hits 12890 12891 +1
+ Misses 30004 30003 -1
Partials 941 941
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
5ffd9b2
to
9847dc3
Compare
e2e failure rely on #5382 /retest |
Hi @a7i @XiShanYongYe-Chang this ci failure have occurred twice, which is introduced recently~ https://github.com/karmada-io/karmada/actions/runs/10401933004/job/28839752385?pr=5378 |
taking a look! Update: #5383 |
9847dc3
to
b5c491b
Compare
… APIService Signed-off-by: Xinzhao Xu <[email protected]>
b5c491b
to
f134f7f
Compare
/lgtm |
/assign @RainbowMango |
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.
/approve
That makes 100% sense. Sorry for the delay.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 bug
What this PR does / why we need it:
I did not enable the
metricsAdapter
component, so the metrics adapter-related deployment was not created:karmada/charts/karmada/templates/karmada-metrics-adapter.yaml
Line 1 in cd99897
However, its
APIService
was still created, causing the controller-manager to continuously output the following error:This patch updates the creation condition of the related
APIService
, ensuring that they will be created together with the deployment.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: