-
Notifications
You must be signed in to change notification settings - Fork 131
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
allow defining ipFamilyPolicy for external service #362
base: master
Are you sure you want to change the base?
Conversation
b52e938
to
bb6fbac
Compare
bb6fbac
to
da26800
Compare
CI/CD fails for reasons unknown to me, but I think the PR itself should be okay. |
@@ -19,6 +19,9 @@ spec: | |||
{{- with .Values.front.externalService }} | |||
type: {{ .type | default "ClusterIP" }} | |||
externalTrafficPolicy: {{ .externalTrafficPolicy | default "Local" }} | |||
{{- with .ipFamilyPolicy }} |
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.
Why use with
here and not if
?
It would seem that it's not certain that ipFamilyPolicy
exists.
with
assumes your variable is present: https://helm.sh/docs/chart_template_guide/control_structures/#modifying-scope-using-with
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.
With and if should behave the same if the variable is absent (not yielding anything), but with
only makes you write the name of the field (ipFamilyPolicy
) once. So the chance to make a typo is reduced by 50% 😁
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.
The "typo chance" is not a joke, I think the vast majority of helm chart bugs I've seen, fixed, and introduced come from copypasting snippets and forget to change part of them.
@fastlorenzo this looks good to me, can we get it merged? |
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
Unstale |
For dual stack (IPv4 and IPv6) clusters, services need to be explicitly opted-in into having two addresses. This is done through the
ipFamilyPolicy
field, as documented in https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services.I've wired the templates in a way where if this field is not present, nothing is rendered. This preserves the usual behavior of not specifying this value altogether for services.