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

[helm.v4.Chart] Proposal: option to deploy helm hooks #3285

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

awoimbee
Copy link

@awoimbee awoimbee commented Oct 24, 2024

Proposed changes

helm.v3.Chart ignored the helm.sh/hook* annotations but still deployed the resources.
The helm.v4.Chart resource skips the hooks. This is mentioned in the blog but not in the docs BTW.

In this PR I propose to make it configurable so a pulumi user can decide to deploy resources, ignoring the hooks.
The end goal being that helm.v4.Chart comes to feature parity with helm.v3.Chart so we can all move all our charts to v4.

Note that:

  • I don't known what I'm doing here, feel free to take over !
  • CONTRIBUTING.md tells me to install stuff in /opt which is not happening, so I did not run the tests locally

Related issues (optional)

Fixes #3284

Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@awoimbee
Copy link
Author

@EronWright you're the only one in the blame of provider/pkg/provider/helm/v4/chart.go, WDYT ?

Would you accept it ? And if so do you want me to iterate on it or someone @ pulumi ?

Copy link
Contributor

@EronWright EronWright left a comment

Choose a reason for hiding this comment

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

I outlined here the technical challenges in truly supporting hook resources. I believe it is worse to implement hooks wrongly than to not support them at all. We have the Release resource for cases where chart compatibility is an issue.

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

Successfully merging this pull request may close these issues.

helm.v4.Chart: make helm hooks optional (instead of force disabled)
2 participants