Skip to content

Chart name must be unique per version #7

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.

Sign up for GitHub

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

Closed
technosophos wants to merge 1 commit into from
Closed

Chart name must be unique per version #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions helm-v3/deisgn-proposal.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ In an effort to mirror the construction of Kubernetes manifests, the new Chart.y
apiVersion: helm.sh/v3
kind: Chart
metadata:
name: myChart
name: myChart-0.1.0
Copy link
Contributor

@mattfarina mattfarina Mar 19, 2018

Choose a reason for hiding this comment

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

Ick. For three reasons:

  1. There is a version property and the version in the name.
  2. How many people would expect the name to include a version? It's going to make people have to think.
  3. If we are working with the Application CRD, why does the Chart.yaml need to be install-able? What's the use case?

Copy link
Member

@prydonius prydonius Mar 19, 2018

Choose a reason for hiding this comment

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

I agree with Matt that this will be confusing. With the Application CRD and Release/ReleaseVersion CRDs, I also think that the Chart.yaml doesn't need to be installable. IMO Chart.yaml should be used for chart repo metadata (index.yaml) and used to generate Application, Release and ReleaseVersion objects.

Copy link
Member Author

@technosophos technosophos Mar 19, 2018

Choose a reason for hiding this comment

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

We don't really know if we are going to use the Application CRD. It's still too much in flux.

But if we do, there's probably no compelling reason to turn the Chart.yaml into a resource-like thing anyway.

The problem is that in the present spec, there can only be one such application/version per namespace, which is not at all the desired behavior. And the only way to get around that is to change the metadata.name field.

Copy link
Contributor

@mattfarina mattfarina Mar 19, 2018

Choose a reason for hiding this comment

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

We don't really know if we are going to use the Application CRD. It's still too much in flux.

We should figure out the criteria on go/no-go soon. At least for first pass on direction. If we don't go the Application CRD and everyone else does we end up shifting expectations onto chart developers to create this if they want to do things like display their apps in dashboard.

The problem is that in the present spec, there can only be one such application/version per namespace

I'm not sure why a name needs to have a unique name/version. What if, in the same namespace, I deploy multiple wordpress charts at the same version? Uniqueness is gone for a name in a namespace in this model.

What's the underlying use case?

Copy link
Member Author

@technosophos technosophos Mar 19, 2018

Choose a reason for hiding this comment

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

Unique name is imposed by Kubernetes.

So the bottom line to me is:

  • If we are modeling a Chart.yaml as something installable, then we have to design it to be actually installable.
  • Otherwise, can we just drop the "let's pretend this is a Kubernetes resource" crap and stick with our simpler YAML format from Helm 2?

Copy link
Member Author

@technosophos technosophos Mar 19, 2018

Choose a reason for hiding this comment

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

See #12, which is probably the better course of action.

labels:
chart: myChart
heritage: helm
version: 0.1.0
appVersion: 3.6
Expand All @@ -79,7 +80,8 @@ Commonly accessed properties are moved into labels, while less frequently used o

| Helm 2 | Helm 3 | Notes |
| -------- | -------- | -------- |
| name | metadata.name | |
| -- | metadata.name | The name _of the chart + version_ |
| name | metadata.labels.chart| |
| version | metadata.labels.version | |
| kubeVersion | data.kubeVersion | |
| keywords | data.keywords | Represented as a YAML list frabgment|
Expand All @@ -93,7 +95,8 @@ Commonly accessed properties are moved into labels, while less frequently used o
| deprecated | data.deprecated | |
| tillerVersion | REMOVED | There is no Tiller |

For the sake of completeness, we will provide a CRD defining this type.
For the sake of completeness, we will provide a CRD defining this type. If a Chart.yaml is installed into the cluster,
it should be installed _once per name + version_, not once per instance.

*Note:* If the Application CRD proposal is workable during the course of early Helm 3 development, we will consider using that.

Expand Down