helm: Use kubeProxyReplacement as string #26549
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrajahalme
added
release-blocker/1.14
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
jrajahalme
added
the
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
gandro
approved these changes
Jun 29, 2023
Helm coalesce does not consider a boolean variable with "false" value of
having a value at all, so $defaultKubeProxyReplacement is picked even if
`helm --set kubeProxyReplacement=false` is used with `--set
upgradeCompatibility=1.12` (for which the default value is `disabled`).
Fix this by explicitly converting .Values.kubeProxyReplacement to a
string (also converting "<nil>" to an empty string).
Alternatively we could have asked users to use `helm --set-string
kubeProxyReplacement=false` instead, but requiring that and silently
ignoring `--set kubeProxyReplacement=false` is confusing to users and
would be incompatible with possible transition to a boolean
kubeProxyReplecement in future.
Prior to this PR this helm command gave this output:
$ helm template cilium cilium --namespace kube-system --set upgradeCompatibility=1.12 --set kubeProxyReplacement=false | grep kube-proxy-replacement:
kube-proxy-replacement: "disabled"
After this PR the output is correct:
$ helm template cilium cilium --namespace kube-system --set upgradeCompatibility=1.12 --set kubeProxyReplacement=false | grep kube-proxy-replacement:
kube-proxy-replacement: "false"
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
force-pushed
the
helm-kpr-string
branch
from
538905e
to
ae251d5
Compare
|
/test |
|
checkpatch fail is a false positive due to helm command being executed in |
|
Travis hit #22550, restarting. |
gandro
approved these changes
Jun 29, 2023
|
@jrajahalme Does this need the |
jrajahalme
added
the
needs-backport/1.14
joamaki
added
backport-pending/1.14
jibi
added
backport-done/1.14
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Helm coalesce does not consider a boolean variable with
"false"value of having a value at all, sodisabledis picked even ifhelm --set kubeProxyReplacement=falseis used with--set upgradeCompatibility=1.12(for which the default value isdisabled).Fix this by explicitly converting
.Values.kubeProxyReplacementto a string, treating<nil>conversion for a missing value as an empty string.Alternatively we could have asked users to use
helm --set-string kubeProxyReplacement=falseinstead, but requiring that and silently ignoring--set kubeProxyReplacement=falseis confusing to users and would be incompatible with possible transition to a booleankubeProxyReplecementin future.Prior to this PR this helm command gave this output:
After this PR the output is correct:
Fixes: #26496