-
Notifications
You must be signed in to change notification settings - Fork 582
feat(helm): add service type to helm chart #7302
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
Conversation
1a3c1b7 to
285c772
Compare
Signed-off-by: Saska Karsi <[email protected]>
Signed-off-by: Saska Karsi <[email protected]>
Signed-off-by: Saska Karsi <[email protected]>
285c772 to
ad52db0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7302 +/- ##
==========================================
+ Coverage 72.01% 72.05% +0.04%
==========================================
Files 230 230
Lines 33420 33420
==========================================
+ Hits 24066 24081 +15
+ Misses 7601 7589 -12
+ Partials 1753 1750 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| control-plane: envoy-gateway | ||
| {{- include "eg.labels" . | nindent 4 }} | ||
| spec: | ||
| type: {{ .Values.service.type }} |
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.
if I understand correctly, this's the service for CP, why do we need to change it?
if the user case is valid, prefer to use empty as default value, render it when user explicitly set it.
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.
Kubernetes' default service type is ClusterIP; I preferred to do it this way just so the documentation is explicit that it works the same as k8s would. I.e. the docs are
| Key | Type | Default | Description |
|---|---|---|---|
| service.type | string | "ClusterIP" |
instead of
| Key | Type | Default | Description |
|---|---|---|---|
| service.type | string | "" |
Type of the service. Kubernetes will set this to ClusterIP if not provided. |
But either way works for me.
What type of PR is this?
What this PR does / why we need it:
To set the service type when using the helm chart (to NodePort, LoadBalancer to receive external traffic) unless I'm somehow misunderstanding how the helm chart is supposed to be used.
Which issue(s) this PR fixes:
Fixes #
Release Notes: Yes/No