Skip to content
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

Add useHelm3 flag #1349

Merged
merged 4 commits into from Dec 8, 2019
Merged

Conversation

SimonAlling
Copy link
Contributor

It doesn't work yet, but will be needed later on (see #1309).

As pointed out by @absoludity: "I think it's fine to land with the useHelm2/3 switch available but clearly document that it's not yet working, as long as it isn't changing current behaviour […]."

It doesn't work yet, but will be needed later on (see vmware-tanzu#1309).

vmware-tanzu#1309 (review)
@SimonAlling
Copy link
Contributor Author

I don't understand why the CI failed. Do you, @andresmgot and @absoludity?

Downloading mongodb from repo https://kubernetes-charts.storage.googleapis.com
Deleting outdated charts
Error: release kubeapps-ci failed: Secret in version "v1" cannot be handled as a Secret: v1.Secret.ObjectMeta: v1.ObjectMeta.TypeMeta: Kind: Data: decode base64: illegal base64 data at input byte 4328, error found in #10 byte of ...| useHelm3"},"kind":"|..., bigger context ...|5EIFJTQSBQUklWQVRFIEtFWS0tLS0t # matches useHelm3"},"kind":"Secret","metadata":{"labels":{"app":"kub|...

@absoludity
Copy link
Contributor

I don't understand why the CI failed. Do you, @andresmgot and @absoludity?

Downloading mongodb from repo https://kubernetes-charts.storage.googleapis.com
Deleting outdated charts
Error: release kubeapps-ci failed: Secret in version "v1" cannot be handled as a Secret: v1.Secret.ObjectMeta: v1.ObjectMeta.TypeMeta: Kind: Data: decode base64: illegal base64 data at input byte 4328, error found in #10 byte of ...| useHelm3"},"kind":"|..., bigger context ...|5EIFJTQSBQUklWQVRFIEtFWS0tLS0t # matches useHelm3"},"kind":"Secret","metadata":{"labels":{"app":"kub|...

Yeah - the issue is that you can't have comments in yaml scalar values, but if you template your branch (and in particular, include the tiller tls which ci does), you'll see:

(useHelm3-flag) ~/dev/kubeapps$ helm template ./chart/kubeapps/ --set tillerProxy.tls.key=./script/test-certs/helm.key.pem --set tillerProxy.tls.cert=./script/test-certs/helm.cert.pem  | grep useHelm3
    Li9zY3JpcHQvdGVzdC1jZXJ0cy9oZWxtLmtleS5wZW0= # matches useHelm3
    heritage: Tiller # matches useHelm3
    namespace: default # matches useHelm3
    release: release-name # matches useHelm3
            secretName: release-name-kubeapps-internal-tiller-proxy # matches useHelm3

The main issue there is the secret value (first line), where the comment is included as part of the secret value. I'd remove the matches useHelm3 as we really don't want those in the yaml output. If you want to keep them to help devs, then perhaps use a template comment instead? ie.:

--- a/chart/kubeapps/templates/tiller-proxy-secret.yaml
+++ b/chart/kubeapps/templates/tiller-proxy-secret.yaml
@@ -23,4 +23,4 @@ data:
   tls.key: |-
 {{ .Values.tillerProxy.tls.key | b64enc | indent 4 }}
 {{- end -}}
-{{- end }} # matches useHelm3
+{{- end }}{{/* matches useHelm3 */}}

Otherwise, +1 from me :)

Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm (with fix in comment above)

@SimonAlling
Copy link
Contributor Author

Wow, that's some interesting interaction between YAML and Go templates right there, @absoludity! I fully agree that the comments shouldn't be in the output. The recently pushed commit seems to have done the trick, but since I had forgotten to merge master into the PR branch first, I did that just now instead.

@absoludity absoludity merged commit 58dcae7 into vmware-tanzu:master Dec 8, 2019
@SimonAlling SimonAlling deleted the useHelm3-flag branch December 9, 2019 08:21
@SimonAlling SimonAlling mentioned this pull request Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants