Chart md #561

sparkprime · 2016-04-05T16:41:11Z

I added the new chart.yaml fields, removed fields not supported in the current backend. Also changed some of the language to reflect this is no longer a proposal but an existing thing.

edit: The intention here is to document existing behavior for users cloning the repo right now.

sebgoa · 2016-04-05T16:43:53Z

docs/design/chart_format.md

-                extensions:
-                  - extensions/v1beta1
+expander:
+  name: GoTemplates


does this work for current helm/charts that are not GoTemplates. In the sense that they are not parameterized ?

The expander is mandatory in the current implementation, so no. I suppose we could modify the behavior such that if no expander is given in the chart, the chart is simply not expanded. However this still takes some code to parse it as YAML, handle the errors, and no code exists in manager to do that right now.

Also the string "GoTemplates" may change after Jack integrates that expander.

fine, but what happens to the current chart that we are talking about merging which don't need expansion.

They won't work for several reasons including that. Literally the only thing that actually works right now is DM-based charts. When the go templates expander is integrated, in order to work in the current implementation, they need to be changed to return DM types instead of K8s resources. I believe then they will work. That change is one of the items in #520

ok, I think it is important to keep existing charts running.
Otherwise, why merge them ?

if we can pass through when no expander is specified then fine. but this needs to work in MVP.

so LGTM

+1 to having a semantics for "no expander". There is probably a good pattern here -- charts with no expander can live at the root and be closed functions. I will have to change the doc though to reflect that design.

FWIW We want to merge the charts and modify them so they work and are really useful. E.g. we will probably want to parameterise most of them, and will have to convert them all to return DM types. So the expander field isn't a big deal in the scheme of things.

Not sure what you mean by "live at the root". All content should still be in the templates directory, although we might want to rename that directory to content as proposed in a comment on an earlier version of this file.

The root of the layout, not the chart.

The layout? Do you mean the templates directory?

sparkprime · 2016-04-05T16:50:30Z

/cc @technosophos

technosophos · 2016-04-05T17:02:59Z

I am good with the changes, but will leave the LGTM to @Runseb, who had a few questions.

sparkprime · 2016-04-05T17:13:52Z

@Runseb please take a look at last commit

sparkprime · 2016-04-05T17:26:30Z

Fixes #554

jackgr · 2016-04-05T21:21:01Z

docs/design/chart_format.md

-We have attempted to build a proposal that does not lose any of the functionality of DM, and which loses only Helm functionality that we believe is disposable. But both tools will need to change substantial portions of their chart-handling logic.
-### Non-Goals
-This proposal doesn’t define how either the client or the server must use this data, though we make some non-normative recommendations (e.g. “this data may be displayed to the user” or “a suitable backend will be able to translate this to a fully qualified URL”).
+This design does not lose any of the functionality of DM, and which loses only Helm functionality that we believe is disposable. But both tools have changed substantial portions of their chart-handling logic.


Since this is the first time the acronym DM is used, the meaning should be spelled out, and the acronym placed in parentheses, like this:

Deployment Manager (DM)

How about deleting the paragraph entirely?

Hmmm. I think it's actually useful, because it provides context for people coming from the previous projects. That said, it's the kind of thing that could live in README.md, not here.

sparkprime · 2016-04-07T20:14:18Z

So I think I addressed all of these comments except the

Chart Developer: Responsible for building new charts.
Application Developer: Responsible for building applications that use charts.

suggestion which I'm not confident is a good idea. These are roles that we would be defining here, rather than roles people already understand. I agree that section could use more work though. It is now in a separate file.

jackgr · 2016-04-07T22:36:44Z

What's wrong with Chart Developer and Application Developer?

People definitely understand Application Developer.

Chart Developer was a proposal to replace Definition Developer, which is totally vague.

sparkprime · 2016-04-07T23:35:17Z

Ok I'll change

Service Dev: Responsible for developing new charts

to Chart developer

sparkprime · 2016-04-07T23:36:43Z

I'm not sure about application developer because we haven't defined what an application is, and the term is used in a lot of other contexts with very specific meanings.

jackgr · 2016-04-08T05:24:31Z

OK. Then how about Chart User?

sparkprime · 2016-04-08T19:35:21Z

Looks like I did in fact change it to "App dev" and just forgot about it, and then argued against doing what I'd already done. Kinda shows you how much context switching I'm doing right now :)
Ready to LGTM?

jackgr · 2016-04-08T19:52:52Z

LOL. Taking another look...

jackgr · 2016-04-08T21:30:29Z

docs/design/chart_format.md

-Based on these, our design goal is to make it **easy to find, use, author, release, and maintain charts**, while still making it possible for advanced users to build sophisticated charts. Furthermore, we have begun the process of **attaching provenance to charts**.
+Based on these, our design goal is to make it **easy to find, use, author, release, and maintain charts**, while still making it possible for advanced users to build sophisticated charts.

+This design does not lose any of the functionality of Deployment Manager (DM), and which loses only Helm functionality that we believe is disposable. But both tools have changed substantial portions of their chart-handling logic.


This design is based on the integration of Deployment Manager (DM) and Helm, formerly at github.com/deis/helm. It does not lose any of the functionality of DM, and loses only Helm functionality that we believe is disposable. Substantial portions of the chart and template handling logic from the original implementations of these two tools have changed as a result of this integration. The new chart format, described in this document, is one of the primary drivers of those changes.

jackgr · 2016-04-08T22:07:18Z

A few final nits. Otherwise, LGTM.

* readme example for failing when values file is missing Believe this would assist with helm#548 * Update README.md Using documentation yanked from searching for the key missingFileHandler https://github.com/roboll/helmfile/search?q=missingFileHandler&unscoped_q=missingFileHandler

sparkprime added kind/documentation awaiting review labels Apr 5, 2016

googlebot added the cla: yes label Apr 5, 2016

sparkprime force-pushed the chart_md branch from 7cd1332 to e80ff04 Compare April 5, 2016 16:41

sebgoa reviewed Apr 5, 2016
View reviewed changes

jackgr self-assigned this Apr 5, 2016

sebgoa added the lgtm label Apr 5, 2016

sebgoa mentioned this pull request Apr 5, 2016

Make charts without parameterized values pass through the expander #570

Closed

jackgr reviewed Apr 5, 2016
View reviewed changes

jackgr removed the awaiting review label Apr 5, 2016

sparkprime removed the lgtm label Apr 6, 2016

sparkprime added the awaiting review label Apr 7, 2016

jackgr reviewed Apr 8, 2016
View reviewed changes

jackgr added lgtm and removed awaiting review labels Apr 8, 2016

Major rework of chart_format.md

bdfedaf

sparkprime force-pushed the chart_md branch from fe0e05b to bdfedaf Compare April 11, 2016 21:04

sparkprime merged commit 4d7c681 into helm:master Apr 11, 2016

mattfarina unassigned jackgr Jul 18, 2018

Chart md #561

Chart md #561

Uh oh!

Conversation

sparkprime commented Apr 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparkprime commented Apr 5, 2016

Uh oh!

technosophos commented Apr 5, 2016

Uh oh!

sparkprime commented Apr 5, 2016

Uh oh!

sparkprime commented Apr 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparkprime commented Apr 7, 2016

Uh oh!

jackgr commented Apr 7, 2016

Uh oh!

sparkprime commented Apr 7, 2016

Uh oh!

sparkprime commented Apr 7, 2016

Uh oh!

jackgr commented Apr 8, 2016 via email

Uh oh!

sparkprime commented Apr 8, 2016

Uh oh!

jackgr commented Apr 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackgr commented Apr 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants