-
Notifications
You must be signed in to change notification settings - Fork 280
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
rename insecure-skip-tls-verify to helm-insecure-skip-tls-verify #568
Conversation
Signed-off-by: yxxhero <[email protected]>
WDYT? |
@@ -253,7 +253,8 @@ func newChartCommand() *cobra.Command { | |||
f.BoolVar(&diff.enableDNS, "enable-dns", false, "enable DNS lookups when rendering templates") | |||
f.StringVar(&diff.postRenderer, "post-renderer", "", "the path to an executable to be used for post rendering. If it exists in $PATH, the binary will be used, otherwise it will try to look for the executable at the given path") | |||
f.StringArrayVar(&diff.postRendererArgs, "post-renderer-args", []string{}, "an argument to the post-renderer (can specify multiple)") | |||
f.BoolVar(&diff.insecureSkipTLSVerify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download") | |||
// see https://github.com/databus23/helm-diff/issues/503 | |||
f.BoolVar(&diff.insecureSkipTLSVerify, "helm-insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download") |
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.
Thanks! Just looking into it; How is this supposed to fix the mentioned issue?
Is it that helm
somehow thinks --insecure-skip-tls-verify
is for helm, not for the helm plugin(=helm-diff in this case)?
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.
kvargs := []string{"--kube-context", "--namespace", "-n", "--kubeconfig", "--kube-apiserver", "--kube-token", "--kube-as-user", "--kube-as-group", "--kube-ca-file", "--registry-config", "--repository-cache", "--repository-config", "--insecure-skip-tls-verify", "--tls-server-name"}
--insecure-skip-tls-verify
is so special, helm will not pass it into plugin. it's helm's rule.
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.
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.
Thanks! I believe I understand that. My questions stems from that; I don't understand why that helm behavior result in unknown flag: --insecure-skip-tls-verify
we are facing.
If helm doesn't process it AND doesn't send it to helm-diff, neither helm nor helm-diff should result in unknown flag: --insecure-skip-tls-verify
.
Given the helm rule you mentioned, how does it result in this error, and why renaming this flag in helm-diff fixes that?
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 key reason is helm will set it as global flag, but helm has no the global flag. @mumoshu
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.
@yxxhero Thanks again! Could you confirm this; Is it a helm bug that helm thinks --insecure-skip-tls-verify
is "known" to helm in preprocessing, then helm fails by thinking it is actually "unknown" to helm when invoking a plugin? Helm messed up the plugin invocation path?
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.
@mumoshu yeah. I get the result by reviewing the source code. I don't know why helm do this.
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.
@mumoshu WDYT?
see: helm/helm#12856 |
closed: helm/helm#12856 |
see: #503