Add various to helm create

[naseemkullah]

What this PR does / why we need it:
Modifies the helm create command as follows:

• Moves all deployment related values to an .Values.deployment umbrella as this makes more sense. (The same way ingress and service values are defined.) <- vetoed
• Adds an easy way to create an hpa for the deployment if the user so chooses to do so
• Adds an easy way to add environment variables for the deployment if the user so chooses to do so
• Makes deployment's probe paths/ports configurable no need really in retrospect, to have them as a value or directly done in the template, no difference for starter chart
• Adds an easy way to add security contexts for deployments, be it for pod or container Already tackled in seperate PR
• Adds an easy way to add volumes for deployments

Special notes for your reviewer:
Feedback is most welcome.

If applicable:

• this PR contains documentation
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[naseemkullah]

bump

[bacongobbler]

Because this changes the "best practices" for a starter chart (helm create), someone from the charts team may be more equipped to review this one. cc @helm/charts-maintainers

[unguiculus]

I'm not sure I like the additional level of nesting, especially since it's of technical nature. Usually, apps group by component if there are multiple.

[naseemkullah]

I'm not sure I like the additional level of nesting, especially since it's of technical nature. Usually, apps group by component if there are multiple.

I see, I see.. do you mean like this?
https://github.com/helm/charts/blob/master/stable/nginx-ingress/values.yaml

[naseemkullah]

@unguiculus I've reverted the additional nesting, what about the other stuff, anything you like/don't like?

[naseemkullah]

Should I change .Values.volumes,.Values.volumeMounts and .Values.env for .Values.extraVolumes,.Values.extraVolumeMounts and .Values.extraEnvVars respectively, or something of the sort?

[naseemkullah]

bump

[naseemkullah]

bump

[naseemkullah]

bump

[naseemkullah]

Hello?

[naseemkullah]

bump (please let me know if there is anything worth keeping, if anything just the hpa stuff maybe??)

[bacongobbler]

Thank you for the contribution.

As a refresher, helm create was originally intended as a command to scaffold a chart directory with the right layout, along with a very basic "getting started" example. It's very similar to hugo new, rails new, or django-admin startproject; It's intended to be a great "out of the box" experience providing some sane defaults, but getting out of the way for the more advanced users who want to extend upon the basics.

With that in mind, helm create was designed to create a chart that can be immediately installable with sane defaults, providing users a good "head start" to write their first chart.

Makes sense?

As a user, I should expect that any and all templates and values should be documented, including

• who is this for?
• what does it do?
• where is this useful?
• when enabled, how is my application affected?
• why should I use this?
• how I can consume/configure these values for my application?

I'm curious to know why there's no supporting documentation provided with the values added with this PR. All of the values provided have been disabled by default (except the liveness/readiness probes, which I left additional feedback below), and as far as I can tell, all of the questions provided above were left unanswered, leaving me confused.

That's the main issue I have with this PR: the "best practices" defined in this PR are all undocumented with no information on how it can be used by first-timers and advanced maintainers alike. The horizontal pod autoscaler, security contexts, volumes, etc. are disabled by default, and it's up to the user to figure out how to use them with no guidance provided. This ends up confusing (and ultimately frustrating) both first-timers and more knowledgeable chart maintainers.

The other issue I have with this PR: all of the values here are disabled by default, and are only useful in certain use cases, but don't always apply. Many applications don't rely on volumes for example. Making these part of the default starter chart only adds more cognitive overhead to the user, as they now have to determine if they actually need to add a horizontal pod autoscaler or volume to their application, and if they don't, how they can remove these "useless" templates from the starter chart.

If you still want to move forward with this, my suggestion would be to start from scratch. Remove all of the values/templates provided here, and start by writing documentation on how users can add horizontal pod autoscalers/security contexts/volumes to their charts under the "Developing Charts" guide in the docs and answer the "5 W's" provided above.

What do you think about that approach?

If that does not work for you, this may be a good starter chart similar to the incubator/common chart. Given that a large amount of the features provided here are only applicable in certain scenarios, closing this PR and going forward with making this part of an opinionated starter chart may be a better long-term solution.

Let me know what you think!

[bacongobbler]

I don't think a readinessProbe should be enabled by default. Not all applications respond to HTTP requests (in fact, Tiller's one such example), and not all of them respond with a 200 OK on the root URL.

[naseemkullah]

true I was just following what comes out of the box with helm create atm

[bacongobbler]

Same comment here. I don't think a livenessProbe should be enabled by default. Not all applications respond to HTTP requests (in fact, Tiller's one such example), and not all of them respond with a 200 OK on the root URL.

[naseemkullah]

true, just following what comes with helm create atm.
I can tweak this (and readiness) to be disabled by default.

Should these also be made configurable to be something else than an httpGet ?

[bacongobbler]

I was suggesting that this should all be removed completely. Did you read the last few paragraphs from my earlier comment?

[naseemkullah]

I did, I did. I'm sorry I don't see where you said to remove the probes completely.

[naseemkullah]

Arguably should we have a deployment template? an ingress template? some charts are just a cronjob... I was just building on what the helm create does now, it creates a typical web api workload. HPA is very useful for this kind of workload.

Security context is very useful for any kind of workload.

As for this PR's approach to volumes, env vars and so on, wanted to open some discussion around this.

[naseemkullah]

Closing. Thanks for feedback everybody.