fix: target metrics patches to metrics-service only#277
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vitorfloriano The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for node-readiness-controller canceled.
|
AvineshTripathi
left a comment
There was a problem hiding this comment.
thanks for looking into this @vitorfloriano!
| - path: metrics_service_tls_patch.yaml | ||
| target: | ||
| kind: Service | ||
| name: metrics-service |
There was a problem hiding this comment.
probaby we should add namespace(system) too?
There was a problem hiding this comment.
Yeah, the manual test already passed with the current fix, but better be a bit more defensive with these patches! Thanks for pointing that out.
The patch was targetting 'Service', which was too broad and being mistakenly applied to all services, including the webhook service, leading to a misconfig.
a729120 to
bc91236
Compare
|
thank you! /lgtm |
The Prometheus TLS patch was targetting 'Service', which was too broad and being mistakenly applied to all services, including the webhook-service, leading to a misconfig and causing errors when validating custom resources (see #276).
This PR targets the patch to the metrics-service only.
Fixes #276
/kind bug
Testing
This was manually tested by running
make build-manifests-temp ENABLE_METRICS=true ENABLE_TLS=true ENABLE_WEBHOOK=trueand then inspecting the generated manifest, which now features the correct config:bin/build/manifests.yaml:
Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?
Doc #278