Skip to content

Support lifecycle hooks#124

Merged
willholley merged 12 commits intoapache:mainfrom
yekibud:support-lifecycle-hooks
Mar 18, 2025
Merged

Support lifecycle hooks#124
willholley merged 12 commits intoapache:mainfrom
yekibud:support-lifecycle-hooks

Conversation

@yekibud
Copy link
Contributor

@yekibud yekibud commented Jun 21, 2023

What this PR does / why we need it:

Adds support for container lifecycle hooks which serve different use cases than helm hooks and can be more reliable for similar use cases (e.g. for changes outside of helm management like a pod or statefulset being deleted).

Special notes for your reviewer:

My use case requires a sophisticated postStart hook with variable interpolation from a parent chart, but I figured many users of this chart may not use it as a subchart, so I included the option to pass in simple container hooks via values, as well. I tried to make this opaque to the user so they could just use .Values.lifecycle in both cases, but I couldn't figure out how to get helm to differentiate between a template and a values map since both are map types. So I wound up supporting both use cases with .Values.lifecycle and .Values.lifecycleTemplate.

Furthermore, following the same pattern, I also added .Values.extraEnv and .Values.extraEnvTemplate to support passing secrets to the hooks.

Checklist

  • Chart Version bumped
  • e2e tests pass
  • Variables are documented in the README.md

I get the following error when trying to run make test. Not sure if this is an existing issue, but it looks unrelated to the changes in this PR.

Error: failed linting and installing charts: failed identifying merge base: must be in a git repository
------------------------------------------------------------------------------------------------------------------------
No chart changes detected.
------------------------------------------------------------------------------------------------------------------------
failed linting and installing charts: failed identifying merge base: must be in a git repository
Removing ct container...
Deleting cluster "chart-testing" ...
Done!
make: *** [Makefile:31: test] Error 1

@yekibud
Copy link
Contributor Author

yekibud commented Aug 9, 2023

@willholley any feedback? In case the use case of the PR isn't clear, here's a more concrete example:

Let's say you need to iterate through a list of values to create admin users, or some other dynamic process after your couchdb instance is stood up. This PR enables you to do that by providing a template to the statefulset that will allow you to populate those values.

Sound useful?

@willholley
Copy link
Member

Thanks for the PR @yekibud. I don't see any issues with this so can merge once it's rebased, tests pass etc.

@yekibud yekibud force-pushed the support-lifecycle-hooks branch from 0a30d65 to cfc3355 Compare August 10, 2023 00:36
@yekibud
Copy link
Contributor Author

yekibud commented Aug 10, 2023

Thanks for the PR @yekibud. I don't see any issues with this so can merge once it's rebased, tests pass etc.

@willholley Thanks. Rebased and updated my branch. I get the same error I reported for make test on main so I don't think that has to do with my PR.

@yekibud
Copy link
Contributor Author

yekibud commented Mar 17, 2025

@willholley sorry for the delay. Resolved the latest merge

Thanks for the PR @yekibud. I don't see any issues with this so can merge once it's rebased, tests pass etc.

@willholley Thanks. Rebased and updated my branch. I get the same error I reported for make test on main so I don't think that has to do with my PR.

@willholley resolved the latest merge conflicts, but tests still don't run for me on my branch or on main, per my message in the PR summary. Thoughts?

@big-r81
Copy link

big-r81 commented Mar 18, 2025

@yekibud personally, I would squash the commits into one commit and then pass the baton to @willholley :-)

@yekibud
Copy link
Contributor Author

yekibud commented Mar 18, 2025

@yekibud personally, I would squash the commits into one commit and then pass the baton to @willholley :-)

Heh, I think the instructions for this repo say not to squash, right?

@willholley willholley merged commit 94889cc into apache:main Mar 18, 2025
2 checks passed
@big-r81
Copy link

big-r81 commented Mar 18, 2025

@yekibud personally, I would squash the commits into one commit and then pass the baton to @willholley :-)

Heh, I think the instructions for this repo say not to squash, right?

Does it? Nevermind, Will already does the job ;-)

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.

4 participants