-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[kube-state-metrics]: add option to set user namespaces #6024
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
base: main
Are you sure you want to change the base?
Conversation
automountServiceAccountToken: true | ||
|
||
# -- Use the host's user namespace available in kubernetes 1.33+. | ||
hostUsers: nil |
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 nil and not false by default?
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.
to allow users to opt-in into setting this value, since its a new field in beta not all k8s distribution support it by default
0488e95
to
cfb8990
Compare
Signed-off-by: AvivGuiser <[email protected]>
sorry, rebased and had some git troubles |
hey @dotdc @tariq1890, would it be possible to get a review here? |
Signed-off-by: AvivGuiser <[email protected]>
{{- if and (semverCompare ">=1.33-0" .Capabilities.KubeVersion.Version) (kindIs "bool" .Values.hostUsers) }} | ||
hostUsers: {{ .Values.hostUsers }} | ||
{{- end }} |
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'd prefer to avoid no-op rendering. Current implementation would only apply the field if the value is a boolean, but Helm will render nothing if it's not, which may be difficult to debug.
I would recommend something more idiomatic, like:
{{- if and (semverCompare ">=1.33-0" .Capabilities.KubeVersion.Version) (kindIs "bool" .Values.hostUsers) }} | |
hostUsers: {{ .Values.hostUsers }} | |
{{- end }} | |
{{- if and (semverCompare ">=1.33-0" .Capabilities.KubeVersion.Version) (not (empty .Values.hostUsers)) }} | |
hostUsers: {{ ternary "true" "false" .Values.hostUsers }} | |
{{- end }} |
This would skip rendering when the key is missing or explicitly nil
, it would not silently fail when the key has any value, but it would still enforce the value of the flag to hold a boolean value.
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.
@dacamposol, I see, this makes a lot of sense, i applied this recommendation
Signed-off-by: AvivGuiser <[email protected]>
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.
@KyriosGN0 Can you bump the chart version plz?
@zanhsieh i did bump the chart's version to 6.5.0 |
Signed-off-by: AvivGuiser [email protected]
What this PR does / why we need it
Which issue this PR fixes
This PR intorduces the option to use user namespaces in k8s version 1.33 and higher, by default it is unset.
(optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged)Special notes for your reviewer
Checklist
[prometheus-couchdb-exporter]
)