Charts: Namespace defined templates in _helpers.tpl

[kevinschumacher]

Is this a BUG REPORT or FEATURE REQUEST? (choose one):
FEATURE REQUEST

I'd like to be able to depend on charts in the stable repository without having to inspect the charts' defined templates each version.

In the Helm documentation's Chart Best Practices, there is this to say about names of defined templates:

Defined templates (templates created inside a {{ define }} directive) are globally accessible. That means that a chart and all of its subcharts will have access to all of the templates created with {{ define }}.

For that reason, _all defined template names should be namespaced._

Most of the charts in the stable and incubator repositories do not follow this best practice.

As PR's are submitted and merged, I will update this issue to track progress.

Which chart:
All of them except the below 6

List of Charts to be updated

• [x] stable/acs-engine-autoscaler #1976
• [x] stable/artifactory #1977
• [x] stable/aws-cluster-autoscaler #1978 (closed; chart is deprecated)
• [x] stable/buildkite #1979
• [x] stable/centrifugo #1980
• [x] stable/chaoskube (no templates defined)
• [x] stable/chronograf #1981
• [x] stable/cluster-autoscaler #1982
• [x] stable/cockroachdb #1984
• [x] stable/concourse #1983
• [x] stable/consul #1985
• [x] stable/coredns #1986
• [x] stable/coscale #1987
• [x] stable/dask-distributed #2125
• [x] stable/datadog #2126
• [x] stable/dokuwiki #2128
• [x] stable/drupal #2127
• [x] stable/etcd-operator #2129
• [x] stable/external-dns #2130
• [x] stable/factorio #2152
• [x] stable/fluent-bit #2151
• [x] stable/g2 #2150
• [ ] stable/gcloud-endpoints #2149
• [x] stable/gcloud-sqlproxy #2148
• [x] stable/ghost #2147
• [x] stable/gitlab-ce #2146
• [x] stable/gitlab-ee #2145
• [x] stable/grafana
• [x] stable/heapster #2144
• [x] stable/influxdb #2143
• [x] stable/ipfs #2142
• [x] stable/jasperreports #2141
• [x] stable/jenkins #2140
• [x] stable/joomla #2139
• [x] stable/kapacitor #2137
• [x] stable/keel #2136
• [x] stable/kube-lego #2135
• [x] stable/kube-ops-view #2134
• [x] stable/kube-state-metrics #2133
• [x] stable/kube2iam #2132
• [x] stable/kubernetes-dashboard #2131
• [x] stable/linkerd #2138
• [x] stable/locust
• [x] stable/magento #2184
• [x] stable/mailhog #2183
• [x] stable/mariadb #2182
• [x] stable/mediawiki #2181
• [x] stable/memcached #2180
• [x] stable/metabase #2179
• [x] stable/minecraft #2178
• [x] stable/minio #2177
• [x] stable/mongodb #1779
• [x] stable/mongodb-replicaset #2176
• [x] stable/moodle #2175
• [x] stable/mysql #2174
• [x] stable/namerd #2173
• [x] stable/nginx-ingress #2172
• [x] stable/nginx-lego #2171
• [x] stable/odoo #2170
• [x] stable/opencart #2169
• [x] stable/openvpn #2168
• [x] stable/orangehrm #2167
• [x] stable/osclass #2166
• [x] stable/owncloud #2165
• [x] stable/parse #2164
• [x] stable/percona #2163
• [x] stable/phabricator #2162
• [x] stable/phpbb #2161
• [x] stable/postgresql #1777
• [x] stable/prestashop #2160
• [x] stable/prometheus
• [x] stable/rabbitmq #1776
• [x] stable/redis #1758
• [x] stable/redmine #2159
• [x] stable/rethinkdb #2158
• [x] stable/risk-advisor #2157
• [x] stable/rocketchat #2156
• [x] stable/sapho #2155
• [x] stable/selenium #2154
• [x] stable/sensu #2153
• [ ] stable/sentry
• [ ] stable/spark
• [ ] stable/spartakus
• [ ] stable/spinnaker
• [ ] stable/spotify-docker-gc
• [ ] stable/stash
• [ ] stable/sugarcrm
• [ ] stable/suitecrm
• [ ] stable/sumokube
• [ ] stable/sumologic-fluentd
• [ ] stable/sysdig
• [ ] stable/telegraf
• [ ] stable/testlink
• [ ] stable/traefik
• [ ] stable/uchiwa
• [ ] stable/voyager
• [ ] stable/weave-cloud
• [ ] stable/wordpress
• [ ] stable/zetcd
• [x] incubator/cassandra #1786
• [x] incubator/check-mk #1837
• [x] incubator/docker-registry #1836
• [x] incubator/elasticsearch #1787
• [x] incubator/etcd #1835
• [x] incubator/gogs #1834
• [x] incubator/istio #1839)
• [x] incubator/kafka #1788
• [x] incubator/kube-registry-proxy #1833
• [x] incubator/patroni #1832
• [x] incubator/redis-cache #1840
• [x] incubator/tensorflow-inception #1838
• [x] incubator/zookeeper #1789

What happened:
What you expected to happen:
How to reproduce it (as minimally and precisely as possible):
Anything else we need to know:

-- Questions for the chart maintainers:

Are there any charts which should not be updated in this way?

Does this change break the chart's API?

• What does that mean for chart versioning?
• Am I correct to increment the patch version, as I have been doing? See #1777 for example
  Because this is a distruptive change, I am incrementing major when major > 0, else incrementing minor. A few PRs which were opened/merged prior had the patch version incremented only.

Helper scripts here:
https://gist.github.com/kevinschumacher/a10162f18be905b62a02ed65d36ab2cf

[kevinschumacher]

cc @dhilipkumars

[kevinschumacher]

This is really just some grunt work, and I'm about 90% to automating it already just from doing those 4 PRs earlier. I'm willing to take on the effort as long as no one objects.

[dhilipkumars]

cc: @kubernetes/charts-maintainers
Thanks for the issue @kevinschumacher, if you could make your tool such that, it can be run against any chart (future PRs) to check if the definition is template "<workload>.fullname".?

I think it would be useful to add that as a port of ci job.

[dhilipkumars]

@kevinschumacher Thanks thats a lot of work.
Once you create a PR link that here with that entry, once the PR is merged 'check' the box.
That way it will be easier to track.

[kevinschumacher]

@dhilipkumars thanks for the issue formatting help.

re: running check in CI -- great idea. Will post gist later.

Just a question of logistics --
One PR per chart is preferred, right?
Do you mind if all PRs get created at once, or would you rather see them spread out over a week?

[dhilipkumars]

All the PRs created at once is totally okay, we could get them in all quickly,
But yes one PR per chart is safer, if you combine all the changes in one PR, then the PR might take a long time to get it.

[ravishivt]

How can a parent chart reference a dependent child chart that has the updated namespace templates?

For instance, my chart declares redis as a dependency. I want to set an environment variable in my parent chart to the redis URL with the redis hostname.

        - name: REDIS_URL
          value: "redis://{{ template "redis.fullname" . }}:6379/0"

This worked previously to the change but no longer does. I tried {{ template "redis.redis.fullname" . }} but that is also undefined. What is the proper syntax to reference nested templates?

[kevinschumacher]

@ravishivt thanks for chiming in.

https://gist.github.com/kevinschumacher/952c8e656f29a85121fd89e80c14c182

I'm using redis 0.7.1 there (which uses "fullname" and "name" templates) and your example actually fails for me.

What version of helm are you using? Perhaps it is a behavior change in the application and not the change to the redis chart?

edit: clarity

[kevinschumacher]

I am seeing some behavior that I didn't expect actually, but, with the redis 0.9.0 chart, you can definitely reference the subchart's defined templates named as they are (the names are global).

In here, I reference "redis.fullname" in my parent chart's NOTES.txt

https://gist.github.com/kevinschumacher/d82507ba67624b761e65c96ca2c21191

However, it seems that the variables referenced in the defined template are bound to the values of the template that's being currently rendered. If I call "redis.fullname" in my parent chart's deployment (or, NOTES.txt), I still get release-parent even though I passed in a redis.nameOverride

This is with helm 2.5.1

[prydonius]

Yes, I believe this doesn't work quite as is expected, as the template is rendered in the context of the chart it's being called from. This means that if the template uses .Chart.Name or other such values redis.fullname would give you the same result as parent.fullname/fullname in the parent chart. This is the reason we currently define redis.fullname in the parent chart (https://github.com/kubernetes/charts/blob/master/stable/sensu/templates/_helpers.tpl#L14). Note that this helper doesn't rely on .Chart.Name and instead hardcodes the name to "redis". It isn't ideal, but this is currently how we get around this.

[kevinschumacher]

So, I sort of come away with this thinking a few different things.

1. namespacing all the defined templates doesn't really have the desired effect; it just makes it so that a subchart's defnition for a "fullname" template doesn't (silently) conflict with a parent chart's definition of a "fullname" template; but because the defined template functions are _functions, whose values are bound based on the current template/chart_ and not simply evaluated once, I can't use a subchart's defined template function as a shared value or identifier.

2. I can't use the nameOverride flag for what I want, either. In fact, I'm not really sure what it's for. Might be worth a docs edit. To me, it seems like I might as well just change the subchart's defined "name" template, or make my own template (as @prydonius mentioned in the sensu chart), because, I can't actually pass a subchart.nameOverride and use it in a parent template. But I think I just don't understand what it's for.

I'm now wondering if I should submit PRs reverting the changes. @dhilipkumars, @prydonius what do you guys think?

(edited: clarity)

[kevinschumacher]

Closed inadvertently, but above comment relevant.

[dhilipkumars]

@kevinschumacher IMHO reverting your changes to old way is not a good solution especially if template functions have global scope. Please see this discussion.

This might be slightly disruptive now but _(as it has been mentioned in the issue)_ we have to first fix helm create to generate proper default namespaced template functions (so that future charts are created properly)

[kevinschumacher]

I added the helper utils for this effort to a gist (also now linked in description)
https://gist.github.com/kevinschumacher/a10162f18be905b62a02ed65d36ab2cf

@dhilipkumars the "find_non_namespaced_template.py" script prints names of defined templates that do not start with the chart's name (uses the folder name as the chart name; it doesn't inspect the Chart.yaml file)

[kevinschumacher]

Can we reopen this? I don't think @unguiculus closed this intentionally.

[dhilipkumars]

@kevinschumacher Apologies for the delay in merging the PRs. Had i done in time you wouldnt have to re-base some of them.

I was waiting for our Helm PR to get merged. I have no idea why it is taking this much time.

[dhilipkumars]

In some of the charts you have updated the major and in most of them, you have updated the minor. We should be consistent. It would be great if you could update the minor version for those charts which major version was incremented.

[dhilipkumars]

and needless to say awesome work so far. We really appreciate your effort.

[omkensey]

Was this inadvertently closed when #2503 was merged? That PR looks like it fixes the zetcd chart only.

Anyway, I think we potentially need three kinds of name/fullname templates::

• One that refers always to the calling chart. This is how name/fullname work now, but as noted by others above, namespacing them almost seems superfluous if they aren't defined differently. (This is good anytime you just need a name and it need not be either unique or consistently rendered.)
• One that refers always to the top-level chart. This need not be namespaced (since it will intentionally always refer to the same chart regardless), but should be consistently named. I propose toplevel, either as toplevel.{name,fullname}, or without the separator dot. This can be done right now by mangling .Template.BasePath in the template. (This is good when you need two references in different charts to render the same.)
• One that refers always to the chart where that template is defined. Currently the only way to do this is to hardcode the chart name into a namespace-named template in that chart. It could be convenient to be able to call a template that renders with reference to a specific chart to match values set in that chart using the current name/fullname-style templates. I propose sourcename as the name for this, as in foochart.sourcename.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close