Run database migrations in pre-install,pre-upgrade hook - #121
Conversation
45c19e0 to
106a60b
Compare
ae33a8c to
63b0a51
Compare
almeidabbm
left a comment
There was a problem hiding this comment.
Hey hey!! Thank you for you contribution 🚀
This looks mostly good although I do have a couple suggestions
Also, in the PR description you mention changing the entrypoint to node dist/index.js, this change isn't here but I do think it should be kept out so that pods still run migrations if they just get their new version via a kubectl command (not upgraded via helm)
a2fd74b to
3888edb
Compare
Yes, that was my initial idea but we can leave it like this. It shouldn't matter as long as the migration runs before the backend pods starts... |
c976ed1 to
2da6b40
Compare
There was a problem hiding this comment.
Can we add a migrationJob.enabled value and conditionally render all the templates:
- configmap
- secrets
- serviceAccount
- migration job
This way someone could opt out of running this job and render the resources
Also, we should bump the version in charts/lightdash/Chart.yaml and charts/lightdash/README.md like - here's a PR with the example https://github.com/lightdash/helm-charts/pull/120/changes
cec113e to
76fda3f
Compare
76fda3f to
c101349
Compare
almeidabbm
left a comment
There was a problem hiding this comment.
LGTM 🚀
Rendering test results:
Results:
┌─────────────────────────────────┬─────────────────────────────────────────────────────┐
│ Scenario │ Outcome │
├─────────────────────────────────┼─────────────────────────────────────────────────────┤
│ Default values │ All 4 migration resources render correctly │
├─────────────────────────────────┼─────────────────────────────────────────────────────┤
│ migrationJob.enabled: false │ Nothing renders — clean │
├─────────────────────────────────┼─────────────────────────────────────────────────────┤
│ migrationJob: {} (empty object) │ Renders fine — Helm deep-merges with chart defaults │
├─────────────────────────────────┼─────────────────────────────────────────────────────┤
│ migrationJob: null │ Nil pointer error on migrationServiceAccount.yaml:1 - same pattern as scheduler │
└─────────────────────────────────┴─────────────────────────────────────────────────────┘
Description
Link to issue.
This solves issue #98 by adding a
pre-upgradehook which runspnpm -F backend migrate-production, without being constrained bystartupProbeorlivenessProbe.Also, this job runs on
pre-installwhen using an external database. To make sure all necessary environment variables are available to the migrationJob, the chart creates ephemeralpre-installConfigMap and Secret objects that are mounted into the migration container.