-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add support for helm --no-hooks flag #4808
Conversation
Hi @larsks. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
e1f8951
to
a215665
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is fine, as it is in line with our long term support plan for helm. Just one request for a test if that's possible.
plugin/builtin/helmchartinflationgenerator/HelmChartInflationGenerator.go
Show resolved
Hide resolved
/ok-to-test |
a215665
to
8fd9e47
Compare
8fd9e47
to
45eb5de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Could you also add this to to our helm chart documentation? it lives here:
https://github.com/kubernetes-sigs/cli-experimental/blob/master/site/content/en/references/kustomize/builtins/_index.md#helmchartinflationgenerator
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: larsks, natasha41575 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Oh, looks like there's a lint error. I can re-LGTM when you update. |
This commit adds the `skipHooks` option to the helm chart support in order to expose the --no-hooks flag introduced to Helm in [1]. Using Kustomize to inflate a Helm chart would in some situations result in different results than using `helm install`. This is because `helm template`, by default, will render chart tests in the `templates/test` directory, which can lead to undesired resources in the output. See [2] for additional discussion. [1]: helm/helm#6444 [2]: helm/helm#6443 Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com>
45eb5de
to
38da7ca
Compare
Add documentation for the skipHooks option introduced in kubernetes-sigs/kustomize#4808.
@natasha41575 I fixed the linting error and added a docs pull request at kubernetes-sigs/cli-experimental#299 |
/lgtm |
Add documentation for the skipHooks option introduced in kubernetes-sigs/kustomize#4808.
This commit adds the
skipHooks
option to the helm chart support in orderto expose the --no-hooks flag introduced to Helm in 1.
Using Kustomize to inflate a Helm chart would in some situations result in
different results than using
helm install
. This is becausehelm template
, by default, will render chart tests in thetemplates/test
directory, which can lead to undesired resources in the output.
See 2 for additional discussion.
Signed-off-by: Lars Kellogg-Stedman lars@oddbit.com