@bacongobbler - I'm ready for a review on this.

Hi @vladfr. Most of my time is spent working on another project, so I only have enough time to weigh in on issues and review smaller feature enhancements or security-related issues. I'd suggest reaching out on the Helm developer call on Thursdays to try and see if someone else can review this. Thanks!

Thanks a lot for this PR!

Feedback is welcomed! I know there has been a lot of interest for this issue, so I am looking forward for your input.
If you don't know where to start, here are the open issues with this PR:

* currently, files are parsed first, then dirs; if a file overlaps somehow, the dir will overwrite it. What if we parse files last, so you can potentially overwrite an unwanted file in a dir? This one is easy to do

I think parsing files after dirs is a good idea. IMHO, one wouldn't expect an explicit file include to be overwritten by a less specific dir include.

* if a path is not found, is unreadable, or if a glob yields no files, the helm command currently fails. Should we continue the install with a warning?

Failing when a path is not found or is unreadable seems fine to me. But the warning maybe better in case of a glob returning nothing. It would introduce behavior inconsistency though. No strong opinion on this one, sorry!

* all external files are added to the root of the chart. This means you can overwrite **any** chart file, including any template. Should we add these files to a special `_external` dir?  Chart authors will need to append it to every lookup: `{{ (.Files.Glob "_external/certs/") }}`

What about forbidding some special paths like the templates and the charts directories, the Chart.yaml, Chart.lock, requirements.yaml and requirements.lock files and maybe the values.yaml file?

First congratulations on PR ❤️ 🚀

I am testing this implementation and trying to make some changes to make it work in my scenarios.

Pass the instClient.ExternalFiles = client.ExternalFiles in the newUpgradeCmd method in cmd/helm/upgrade.go so that it works in a helm upgrade - installation.

It is also necessary to call the loadExternalFilesTemp method in cmd/helm/upgrade.go to run new versions; it is interesting to centralize this method somewhere accessible for install and upgrade.

I would like to help you with implementation suggestions.

@hunsche - thank you so much for your notes, you are correct on both points. I've added your suggestions and it seems to work nicely.

I've also increased test coverage. The only part I'm missing is the actual upgrade command with the new flags; I'll try to catch that in a test.

I mistakenly thought that upgrade would run runInstall but it does not. Right now I don't think it's worth refactoring this, as both install and upgrade commands are in the main package - I was able to simply call loadExternalFiles from upgrade. Maybe the function will look better in another go file though.

@yann-soubeyrand thanks for your replies, I'll go ahead and switch the ordering.

We still need to solve the problem of all external files being added to the root of the chart. Your suggestion of blocking paths is reasonable, but I'd like some more feedback on it, since it's not very forward-compatible.

Slating for 3.4.0 for review as discussed on the call

Hi,

We still need to solve the problem of all external files being added to the root of the chart.

Forbidding paths is a possibility, but I would rather opt for your other proposition and place all the external files in a sub-folder.

--include-file my_license.conf=../license.conf would thus make it available under _external: {{ (.Files.Glob "_external/my_license.conf") }}. There are several motivations:

• As a Helm package developer, I want to give some flexibility to my package. Some files cannot just be templatized and need to be adapted to deployment contexts. Having these files along with the values.yaml, separated from the Helm repository, is a good approach. It fits for certificates (although there are other ways to manage this), but most of all for some very complex configuration files (we have an excellent example for a mail server).
• On the other hand, I do not want users to be able to override everything: that would be a nightmare for support. In particular when those that use the package are not those who developed and maintain it. Allowing to override templates or static files is dangerous and could have unknown side effects. Packages must simplify usage and configuration by users, but also set boundaries to prevent errors from users. If there is a file to adapt or update, that will be done in another version of the package: either by making it a template, or by allowing the use of an external file, or even let the choice to the user through a parameter in the values.yaml file.

The initial issue is about adding files from outside the package, not about overriding what is in it. Just my 2 cents. :)

Anyway, thanks for working on this. We have just met the requirement on our project and we will be glad to use this feature as soon it is available. Soon, hopefully. 👍

Anyway, thanks for working on this. We have just met the requirement on our project and we will be glad to use this feature as soon it is available. Soon, hopefully. 👍

Thanks for your feedback @vincent-zurczak, there is more work on this, so the PR is slotted for the 3.4.0 release.

@vladfr can you please rebase from the master branch. I'm getting a test error about something in pkg/release/mock.go that's been fixed there. There is also a difference in cmd/helm/install.go that needs to be handled.

@mattfarina - Unfortunately, I am not able to tend to this in the next few weeks to a month. I have made an attempt, but I messed it up. Sorry about that :(

If someone else can pick it up, I'm ok with that. If this still open when I return, I'll clean it up.

I can do it is a feature that I am using.

I opened the new PR in #8841

It appears this PR is overcome by #8841. So, I am going to close it. If this is not the case we can re-open.