Skip to content
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.

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

remove dirname constraint on helm package #1979

Closed
bacongobbler opened this issue Feb 15, 2017 · 24 comments
Closed

remove dirname constraint on helm package #1979

bacongobbler opened this issue Feb 15, 2017 · 24 comments

Comments

@bacongobbler
Copy link
Member

When I run helm package on a directory that contains chart information, for example chart/:

><> helm package chart/
Error: directory name (chart) and Chart.yaml name (foo) must match

This is inconsistent with helm install, as I can install any arbitrary directory:

><> helm install chart/
NAME:   foo
...

From a conversation on Slack, there's no technical reason why this is a limitation other than technical debt that wasn't removed before helm v2.0.0 was released. For Helm 3.0, I propose this limitation should be removed so that I may be able to name my chart package under any directory so long as it follows the chart conventions in the design doc.

@bacongobbler
Copy link
Member Author

bacongobbler commented Feb 15, 2017

One workaround for others is to tar/sign the package yourself outside of helm package.

$ tar czf foo-0.1.0.tgz chart/
$ helm install foo-0.1.0.tgz

The provenance file can be created following this doc: https://github.com/kubernetes/helm/blob/master/docs/provenance.md#the-provenance-file

@iameli
Copy link
Contributor

iameli commented Feb 16, 2017

Would this actually need to wait until 3.0? It doesn't seem to be backwards-breaking.

@technosophos technosophos added this to the 3.0.0 - Triage milestone Feb 17, 2017
@technosophos
Copy link
Member

It is "breaking" in the sense that it violates the chart spec, which specifies that a directory name must match the name. helm package must create charts that follow the spec.

One of the reasons to not drift from the spec is that it is reasonable to assume that an unpackaging tool may follow this logic:

  • get package foo
  • untar the file
  • cd into directory foo
  • read the Chart.yaml

It is worth noting that hem install was written to purposefully be tolerant of slightly malformed charts. It is not a bug that Helm will search for the Chart.yaml file in an archive. Just like HTML parsers may be lenient with malformed documents, so Helm is lenient with malformed charts.

Finally, the reasoning behind this decision in the first place (which I had forgotten when we talked on Slack) is that the chart spec we developed in early 2016 was designed to allow other files and directories to exist inside of the root of a package to make Helm forward-compatibile should we ever decide to do anything crazy (like storing the image in an image/ directory inside the chart -- which was an early idea).

@iameli
Copy link
Contributor

iameli commented Feb 17, 2017

All of that seems reasonable, but this ticket could still be accommodated without modifying the resulting tgz files -- helm package would just ignore the name of the chart directory, and package its contents into the appropriate subdirectory within the archive. helm package ./chart would package ./chart/* into chart-name/* within the tar archive.

I understand how that's slightly less consistent, but I think there's a legitimate use case here: if I write a little microservice and want to publish it as a GitHub project, a chart subdirectory is the logical place to put it. An example directory structure:

chart/
src/
Dockerfile
Makefile
Readme.md

The alternatives would be to have a nested chart/example-project extra subdirectory, which feels unnecessary, or have the repo root itself be the chart directory, which clutters my repo with a bunch of files and directories and requires me to manually .helmignore the actual source code of the project. Not wild about either of those.

@seh
Copy link
Contributor

seh commented Apr 10, 2017

I ran into this problem too today, as I mentioned in the "helm" Slack channel.

@bacongobbler
Copy link
Member Author

Now that it has been publicly announced, the main reason why I opened this issue is because of draft. Draft expects the application's chart to be inside chart/, and the main use case is to hack on the chart in dev then use helm package on the app for production. Unfortunately, this is not possible today due to helm package being unable to package the chart/ directory.

@iameli nailed it on the head here as how draft expects the filesystem layout to look like.

@iameli
Copy link
Contributor

iameli commented Jun 6, 2017

Yeah, I looked at Draft a bit and was squinting at y'all Deis folks for releasing a product that violates your own linter. 🙃

@michelleN
Copy link
Member

hahahahaha @iameli

bacongobbler referenced this issue Nov 15, 2017
'helm package' now produces an error if the directory name and the name
in Chart.yaml are not the same.
@fejta-bot
Copy link

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.

Prevent issues from auto-closing with an /lifecycle frozen comment.

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

@bacongobbler
Copy link
Member Author

/remove-lifecycle stale

@bacongobbler
Copy link
Member Author

/lifecycle frozen

@AmundsenJunior
Copy link

Is this issue still open for contribution, or only for further discussion of the feature proposed?

@bacongobbler
Copy link
Member Author

We’re keeping it open as a discussion point. It’s not something we can accept in the 2.0 milestone as mentioned before, but it’s still good to keep open for when we do planning for a v3.0. :)

#2954 is the proposed PR to fix this; it’s just a few lines of code that need to be removed to remove this constraint.

AmundsenJunior pushed a commit to AmundsenJunior/helm that referenced this issue Jan 20, 2018
The existing requirement that the chart directory name and the
name of the chart (Chart.yaml "name") is removed.

Closes helm#1979
AmundsenJunior pushed a commit to AmundsenJunior/helm that referenced this issue Jan 20, 2018
Added test (and needed new test chart) to cover the removed
chart directory name constraint.

Closes helm#1979
AmundsenJunior pushed a commit to AmundsenJunior/helm that referenced this issue Jan 20, 2018
As part of the removal of the chart name and chart directory
name matching constraint, this updates the linting rules to no
longer error on a mismatch. The requirement that there be a Chart
name, covered by validateChartName rule, also covers the removed
test case in TestValidateChartNameDirMatch where the Chart name is
missing.

Closes helm#1979
@AmundsenJunior
Copy link

Oh I don't know how I missed seeing that PR. Thanks for pointing it out @bacongobbler.

I had worked on this last night, and just put up a branch on my fork for consideration anyway (master...AmundsenJunior:feat/1979-remove-chart-dirname-constraint). The changeset includes an added test for 'package' to cover the removed constraint, as well as updates to 'pkg/lint' to no longer error on the chart/directory name matching rule.

@sstarcher
Copy link
Contributor

I would to echo a statement similar to what others have said. The current constraint on the directory matching what is listed in the Chart.yaml does not really add value and forces a structure that makes several valid layout cases harder to deal with. Similar to my usecase in #3845

@jstoja
Copy link

jstoja commented Aug 29, 2018

Since this is something that will be possible in the v3, maybe the best way to sovle it for the v2 is to add a warning? It's "breaking" the spec, but it's going to change anyway in the (near?) future.

@luisdavim
Copy link
Contributor

why not just convert the error into a warning and not block the process?

@towolf
Copy link

towolf commented Sep 2, 2018

We would also really like to use chart names that look like this:

group/project/chartname

Where group and project refer to Gitlab groups and projects respectively.

This would ensure that projects that upload charts to our internal helm repo (Artifactory implementation, it supports subdirectories), will not collide with their chart names.

Currently we have to suggest chart names like this, just to disambiguate them:
example-projects-helm-hello-world.

Giving us the freedom to allow / in the names would side-step this problem:

image

image

So instead of this:

  example-projects-helm-hello-world:
  - apiVersion: v1
    created: 2018-09-02T09:58:36.300Z
    description: A Helm chart for Kubernetes
    digest: 647a9e1578499daf665f842e680f80ec27c613301f0eb2bd99d98cd8623650af
    name: example-projects-helm-hello-world
    urls:
    - local://example-projects/helm-hello-world/example-projects-helm-hello-world-0.0.1.tgz
    version: 0.0.1

we could just as well use this

  example-projects/helm-hello-world/demo:
  - apiVersion: v1
    name: example-projects/helm-hello-world/demo
    urls:
    - local://example-projects/helm-hello-world/demo-0.0.1.tgz
    version: 0.0.1

I can make a tarball myself, of course, but I would like to be able to use helm lint and helm package --dependency-update

@dee-kryvenko
Copy link

I have mentioned my use case #4683, it was closed as a duplicate, but I still have a question to address. As I mentioned, the issue basically making hard such a basic task as building CI pipeline for a chart. I am not really getting what does it has to do with a specs or something - to me the name of parent directory basically is not a property of the chart and that directory is not a jurisdiction of helm. I'm glad it doesn't try to check the name of my user's home directory...
What I'm trying to say is - waiting for 3.0 doesn't making any sense to me without providing some workaround. Feature toggler, configurable warning. Other ideas how to fool it other than a symlink or fixing this hang when it is under a symlink. Anything. Please help.

@bacongobbler
Copy link
Member Author

this was implemented in #4141 which was merged into the Helm 3 branch. Closing

@bacongobbler
Copy link
Member Author

@llibicpep see #1979 (comment) for a workaround.

@matthewerwin
Copy link

@bacongobbler why not add a CLI flag so that CI can bypass this check for all cases where v2 is being used without affecting default behavior? Having to set a different chart path for every CI configuration and project is hugely annoying & v3 may not be available on Azure devops tools and others for some time. A --flag would be a simple solution that retains v2 behavior by default.

@romilpunetha
Copy link

romilpunetha commented Mar 5, 2019

So what is the final outcome ? How do we overcome this problem? My CI is breaking because of this issue.
I have a step: jx step helm build --verbose,
where I get the directory name and chart name mismatch.

@michelleN
Copy link
Member

@matthewerwin please open a separate issue for your proposal. thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests