✨ Make webhook and metrics ports configurable in Helm chart template #4890
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Okabe-Junya 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 |
k8s-ci-robot
added
the
size/XS
|
/test all |
Okabe-Junya
changed the title
Okabe-Junya
changed the title
| @@ -105,7 +105,7 @@ spec: | |||
| {{- if .HasWebhooks }} | |||
| {{ "{{- if .Values.webhook.enable }}" }} | |||
| ports: | |||
| - containerPort: 9443 | |||
| - containerPort: {{ "{{ .Values.webhook.port }}" }} | |||
There was a problem hiding this comment.
This ports as fixed for golang in the scaffold.
We cannot change the port only for helm
That will not work if you add other port.
Also, see: #4802
There was a problem hiding this comment.
Thank you, ah... I see, Understood.
I'll think about it for a moment 🙇
There was a problem hiding this comment.
Sorry, I finally understand how KubeBuilder's scaffold works.
I believe it's now configurable with this diff.
Could you please review it again? Thank you
k8s-ci-robot
added
size/M
Okabe-Junya
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/cc camilamacedo86 |
|
Oh, I woke up and the e2e test failed... |
|
@Okabe-Junya: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
There was a problem hiding this comment.
Pull Request Overview
This PR makes webhook and metrics ports configurable in Helm chart templates by adding command-line flag support and updating template references. The change allows users to customize port configurations through Helm values instead of being hardcoded.
Key changes:
- Added
--webhook-portcommand-line flag with default value 9443 to all main.go files - Updated Helm chart templates to use configurable port values from
.Values.webhook.portand.Values.metrics.port - Added default port configurations to values.yaml template
Reviewed Changes
Copilot reviewed 11 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Multiple main.go files | Added webhookPort variable and flag configuration with webhook server port setup |
| values.go | Added default port configurations and updated args to use templated port values |
| webhook/service.go | Updated targetPort to use configurable webhook port value |
| metrics/metrics_service.go | Updated port and targetPort to use configurable metrics port value |
| manager/manager.go | Updated containerPort to use configurable webhook port value |
| @@ -61,8 +61,9 @@ controllerManager: | |||
| tag: latest | |||
| args: | |||
| - "--leader-elect" | |||
| - "--metrics-bind-address=:8443" | |||
| - "--health-probe-bind-address=:8081" | |||
| - "--metrics-bind-address=:{{ "{{" }} .Values.metrics.port {{ "}}" }}" | |||
There was a problem hiding this comment.
The metrics bind address argument uses port templating but should maintain the full address format. Consider using --metrics-bind-address=127.0.0.1:{{ .Values.metrics.port }} or :{{ .Values.metrics.port }} consistently with how it was previously formatted.
| - "--metrics-bind-address=:{{ "{{" }} .Values.metrics.port {{ "}}" }}" | |
| - "--metrics-bind-address=127.0.0.1:{{ "{{" }} .Values.metrics.port {{ "}}" }}" |
Copilot uses AI. Check for mistakes.
close #4802
What I changed
webhook.port: 9443andmetrics.port: 8443configuration options{{ .Values.webhook.port }}and{{ .Values.metrics.port }}