Use consistent whitespace in template placeholders #1437

fhemberger · 2017-07-05T12:06:18Z

Sorry, I know this a trivial PR, but it would be nice to have consistent use of whitespace inside all charts. 😉

k8s-ci-robot · 2017-07-05T12:06:25Z

Hi @fhemberger. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

unguiculus · 2017-07-05T18:31:45Z

Thanks a lot. This is very much appreciated. But I'm afraid most of this, if not all, has already been done in #1062 which has just been merged today.

fhemberger · 2017-07-05T18:42:56Z

@unguiculus Hmm, the issue you mentioned is about Elasticsearch. 🤔

unguiculus · 2017-07-05T18:45:58Z

Ouch, sorry. I guess I got confused by the merge conflicts. Here's an open PR for Artifactory which should get merged soon: #1314.

fhemberger · 2017-07-05T18:50:28Z

@unguiculus I updated the PR to resolve the merge conflicts. My PR doesn't just cover Artifactory, it fixes the template placeholder whitespace in all charts.

unguiculus · 2017-07-05T18:57:32Z

Wow, I guess I did not look far enough. Did you use some script for this?

Usually we want separate PRs per chart. Do you think you can split this up?

@kubernetes/charts-maintainers How should we go about this? I think this is great.

fhemberger · 2017-07-05T19:06:15Z

Just a simple Regex search & replace in Sublime Text:

{{([^ -]) -> {{ $1
([^ -])}} -> $1 }}
(search for all double curly braces that are not followed by a space or a dash and insert a single space between them and the char found)

Splitting it up would have ended up in 38 separate PRs just adding whitespace, so I left it in one single PR instead. 😉

https://github.com/kubernetes/charts/pull/1437/files?w=1 shows that it's really only whitespace that has changed.

unguiculus · 2017-07-05T19:23:10Z

Maybe we can take this as a whole. Discussing it on Slack.

unguiculus

I just found one place where the replace should not happen.

unguiculus · 2017-07-05T19:26:09Z

incubator/etcd/templates/etcd-statefulset.yaml

@@ -129,7 +129,7 @@ spec:
            fi

            # etcd-SET_ID
-            SET_ID=${HOSTNAME:5:${#HOSTNAME}}
+            SET_ID=${HOSTNAME:5:${#HOSTNAME }}


This is shell not Go templating. We probably don't want to change that.

mgoodness · 2017-07-05T21:04:14Z

/test pull-charts-e2e

fhemberger · 2017-07-06T04:05:30Z

Rebased master, fixed tensorflow helper.

/retest

k8s-ci-robot · 2017-07-06T04:05:44Z

@fhemberger: you can't request testing unless you are a kubernetes member.

In response to this:

Rebased master, fixed tensorflow helper.

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

unguiculus · 2017-07-06T09:07:54Z

/retest

k8s-ci-robot · 2017-07-06T09:17:11Z

@fhemberger: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-charts-e2e	1e8c0b098d4f25c54959b6241894374f0a8d31a5	link	`/test pull-charts-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fhemberger · 2017-07-06T09:59:51Z

@unguiculus Sorry, not sure what's wrong with the Jenkins built, I don't think it's actually related to my changes.

unguiculus · 2017-07-07T05:37:48Z

Yeah, there seems to be some test flakiness. We are going to merge this as a whole. Anything else does not make sense.

unguiculus · 2017-07-07T06:01:07Z

Would you mind covering NOTES.txt files as well? I understand that those will not be as easy because they often contain shell magic. But we can just try and get this in soon and follow up with that in a separate PR.

fhemberger · 2017-07-07T09:08:24Z

Sure, I can do that as well, but I'd prefer a separate PR to minimize impact.

Also make sure paramater order is always `kubectl get svc --namespace {{ .Release.Namespace }} {{ template "fullname" . }} --template "…"`

fhemberger · 2017-07-08T14:47:02Z

@unguiculus Fixing NOTES.txt wasn't so bad, so I added it here. Also fixed parameter order on three occasions, so the command is always identical. Should I also adjust APP_HOST vs. SERVICE_IP? (I'd prefer the latter, it is more precise.)

unguiculus · 2017-07-09T19:10:44Z

I'm going to merge this as is. You may create a separate PR as follow-up. I'd prefer SERVICE_IP as well.

fhemberger · 2017-07-09T19:34:37Z

Awesome, thanks!

rk295 · 2017-07-10T10:11:03Z

Hi, this commit breaks (at least) the grafana chart. It fails a lint with the default values.

[ERROR] templates/: parse error in "grafana/templates/job.yaml": template: grafana/templates/job.yaml:41: unexpected "}" in operand

Error: 1 chart(s) linted, 1 chart(s) failed

rk295 · 2017-07-10T10:11:32Z

As a follow up, would it be worth having CI lint each of the charts in branches before merging?

fhemberger · 2017-07-10T10:57:05Z

@rk295 Sorry, didn't notice the change in the Job.
Fixed in #1458.

* upstream/master: (67 commits) Fix json whitespace (helm#1458) Use consistent whitespace in template placeholders (helm#1437) [stable/selenium] Make hub readiness probe timeout configurable (helm#1391) [stable/kube2iam]: add rbac support (helm#1286) [stable/traefik] Allow enabling traefik access logs (helm#1302) Add Stash chart (helm#1420) Add Gearman G2 chart (helm#1421) add option to include tolerations to daemonset (helm#1364) Moved Artifactory to stable and updated version to 5.3.2 (helm#1314) Concourse postgres conditional dependency (helm#1390) Typo in helm install command for dask-distributed (helm#1413) [stable/fluent-bit] Fluent Bit v0.11.12 (helm#1417) fixed cassandra chart's persistence bug (helm#1245) Prometheus: modify config to support k8s 1.6 by default (helm#1080) Add rocket.chat (helm#752) Fix influxdb deployment (helm#1424) feat(stable/etcd-operator): add support for supplying additional command args (helm#1418) add configurable service annotations helm#1234 (helm#1244) [stable/prometheus] extra environment variable for alert manager (helm#1237) [stable/heapster] Default service name to Heapster (helm#1266) ...

Use consistent whitespace in template placeholders

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 5, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 5, 2017

unguiculus self-assigned this Jul 5, 2017

unguiculus suggested changes Jul 5, 2017

View reviewed changes

unguiculus approved these changes Jul 5, 2017

View reviewed changes

unguiculus added code reviewed lgtm Indicates that a PR is ready to be merged. labels Jul 5, 2017

fhemberger added 3 commits July 8, 2017 06:16

Use consistent whitespace in template placeholders

0def3d7

[incubator/tensorflow-inception] Fix template helper

156c2b0

Fix whitespace in NOTES.txt

b27223c

Also make sure paramater order is always `kubectl get svc --namespace {{ .Release.Namespace }} {{ template "fullname" . }} --template "…"`

unguiculus merged commit 912f50c into helm:master Jul 9, 2017

fhemberger deleted the fix/whitespace branch July 9, 2017 19:34

yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017

Use consistent whitespace in template placeholders (helm#1437)

87dbe49

Use consistent whitespace in template placeholders

wmcdona89 pushed a commit to wmcdona89/charts that referenced this pull request Aug 30, 2020

Use consistent whitespace in template placeholders (helm#1437)

cf13f6a

Use consistent whitespace in template placeholders

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use consistent whitespace in template placeholders #1437

Use consistent whitespace in template placeholders #1437

fhemberger commented Jul 5, 2017 •

edited

Loading

k8s-ci-robot commented Jul 5, 2017

unguiculus commented Jul 5, 2017

fhemberger commented Jul 5, 2017 •

edited

Loading

unguiculus commented Jul 5, 2017

fhemberger commented Jul 5, 2017

unguiculus commented Jul 5, 2017

fhemberger commented Jul 5, 2017 •

edited

Loading

unguiculus commented Jul 5, 2017

unguiculus left a comment •

edited

Loading

unguiculus Jul 5, 2017

fhemberger Jul 5, 2017

mgoodness commented Jul 5, 2017

fhemberger commented Jul 6, 2017

k8s-ci-robot commented Jul 6, 2017

unguiculus commented Jul 6, 2017

k8s-ci-robot commented Jul 6, 2017

fhemberger commented Jul 6, 2017

unguiculus commented Jul 7, 2017

unguiculus commented Jul 7, 2017

fhemberger commented Jul 7, 2017

fhemberger commented Jul 8, 2017

unguiculus commented Jul 9, 2017

fhemberger commented Jul 9, 2017

rk295 commented Jul 10, 2017

rk295 commented Jul 10, 2017

fhemberger commented Jul 10, 2017

Use consistent whitespace in template placeholders #1437

Use consistent whitespace in template placeholders #1437

Conversation

fhemberger commented Jul 5, 2017 • edited Loading

k8s-ci-robot commented Jul 5, 2017

unguiculus commented Jul 5, 2017

fhemberger commented Jul 5, 2017 • edited Loading

unguiculus commented Jul 5, 2017

fhemberger commented Jul 5, 2017

unguiculus commented Jul 5, 2017

fhemberger commented Jul 5, 2017 • edited Loading

unguiculus commented Jul 5, 2017

unguiculus left a comment • edited Loading

Choose a reason for hiding this comment

unguiculus Jul 5, 2017

Choose a reason for hiding this comment

fhemberger Jul 5, 2017

Choose a reason for hiding this comment

mgoodness commented Jul 5, 2017

fhemberger commented Jul 6, 2017

k8s-ci-robot commented Jul 6, 2017

unguiculus commented Jul 6, 2017

k8s-ci-robot commented Jul 6, 2017

fhemberger commented Jul 6, 2017

unguiculus commented Jul 7, 2017

unguiculus commented Jul 7, 2017

fhemberger commented Jul 7, 2017

fhemberger commented Jul 8, 2017

unguiculus commented Jul 9, 2017

fhemberger commented Jul 9, 2017

rk295 commented Jul 10, 2017

rk295 commented Jul 10, 2017

fhemberger commented Jul 10, 2017

fhemberger commented Jul 5, 2017 •

edited

Loading

fhemberger commented Jul 5, 2017 •

edited

Loading

fhemberger commented Jul 5, 2017 •

edited

Loading

unguiculus left a comment •

edited

Loading