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

pkg/chart/chart: add Manifests field for untemplated manifests #7405

Closed

Conversation

invidian
Copy link

This commit adds an ability to skip rendering engine for certain
manfiests, when building the chart object programmatically.

This allows to completely skip templating part of helm, while still be
able to use hooks, versioning, upgrades etc. functionality.

Assembling charts programmatically is useful, for example when user wants
to apply post-templating changes to the chart, like kustomize, to avoid
modifying upstream helm chart, but still be able to use helm for
managing the installation.

While for many charts it is fine to be processed by templating engine
multiple times, for others, it is problematic.
The example here are charts, which use similar templating syntax in ConfigMap
objects, so they have to escape {{ }} templating syntax, which is
already non-trivial with helm. The example of such chart is
prometheus-opereator, which brings alerting rules, which also use {{ }}
syntax.

Given that the implementation is very simple, I decided to create a Pull Request directly rather than creating an issue first, to emphasize, that the actual modification needed is very small.

Please let me know what you think about it.

Signed-off-by: Mateusz Gozdek mateusz@kinvolk.io

This commit adds an ability to skip rendering engine for certain
manfiests, when building the chart object programmatically.

This allows to completely skip templating part of helm, while still be
able to use hooks, versioning, upgrades etc. functionality.

Assembling charts programmatically is useful, for example when user wants
to apply post-templating changes to the chart, like kustomize, to avoid
modifying upstream helm chart, but still be able to use helm for
managing the installation.

While for many charts it is fine to be processed by templating engine
multiple times, for others, it is problematic.
The example here are charts, which use similar templating syntax in ConfigMap
objects, so they have to escape {{ }} templating syntax, which is
already non-trivial with helm. The example of such chart is
prometheus-opereator, which brings alerting rules, which also use {{ }}
syntax.

Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>
@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 15, 2020
@invidian
Copy link
Author

@bacongobbler @marckhouzam could you take a look?

@bacongobbler
Copy link
Member

bacongobbler commented Jul 7, 2020

hi @invidian. A couple of things pop out here.

As per the CONTIRBUTING guide, we ask users to please open an issue when opening a PR, regardless of its size. That way we can have a discussion before any code is written so that you're not wasting time writing code that would eventually be thrown out or not accepted. It also allows us time to coordinate reviews across multiple PRs that may be touching the same code, or trying to fix the same areas of the codebase to address conflicting issues which can be amalgamated into a simpler fix.

We also ask for unit tests with every PR. In some cases it may be difficult to test certain behaviour, but this PR looks fairly clear in scope for testing. It gives us an opportunity to ensure regressions aren't introduced in the future.

Without any test cases, I cannot verify how I am supposed to test and verify this PR, which is why there has not been a response on it for a while.

Hope this helps.

@invidian
Copy link
Author

Thanks for the reply @bacongobbler. I created #8497 to discuss it then. It seems that the PR might not be needed, as in the meanwhile post-rendering functionality has been added to Helm and I may utilize that. If I can get my things to work without this patch, I'll close the PR.

@bacongobbler
Copy link
Member

okay. Let's close this for now and continue the discussion in #8497 before we come to a solution. Thanks.

@invidian
Copy link
Author

@bacongobbler sorry for keeping it unanswered for so long. I created a draft PR with mentioned changes (kinvolk/lokomotive#727), but I couldn't get it to work for some reason. I'll try to figure it out and then get back to #8497. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants