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

Create new pre/post migrate jobs #163

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

timetinytim
Copy link
Contributor

This is a replacement PR for #37; it does essentially the same thing, but with the changes discussed in that PR, plus some bugfixes for things I discovered while testing this on our staging environment.
Also addresses the concern from #74 as relevant env variables are now included in the jobs.

@renchap: I tested this directly in our staging environment to make sure jobs spun up & down as necessary, and it works great. This doesn't address the issue with connection poolers, but I'll do that in the next PR. This is just prep work for that.

CC'ing @angdraug @dunn @paolomainardi as they created/contributed to the discussion in the previous PR.

@timetinytim timetinytim requested a review from renchap February 12, 2025 12:50
@timetinytim timetinytim self-assigned this Feb 12, 2025
{{- if .preDeploy }}
"helm.sh/hook": pre-upgrade
{{- else }}
"helm.sh/hook": post-install,post-upgrade
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be post-install? If we do it this way, then on the first install of the chart the sidekiq/web pods will start, then the migration will run, but there will be a time where the schema is empty. pre-install should be better here?

Also on initial install, we do not recommend to run the migrations (it will run every DDL one by one), but to use db:setup which will load it all at once. So doing that is probably better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it's been a while since I did this from scratch, and forgot about db:setup. So that should be run before anything is deployed for the schema, then the pods can be created. I will make that change.

Then I have two questions:

  • Is db:setup idempotent? If it is, we can just blindly run it every time and not worry about it. Otherwise, we will have to create some kind of logic to only have it run if we can detect that the database schema is already present.
  • If db:setup is run first on an initial install, is there a problem with running db:migrate as a post-install hook? I figure it will simply do nothing.

Copy link
Member

Choose a reason for hiding this comment

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

Is db:setup idempotent?

It should, but I just wiped by dev database running it again, so it looks like it does not? Need more tests on this

If db:setup is run first on an initial install, is there a problem with running db:migrate as a post-install hook? I figure it will simply do nothing.

Yes, you can run the migrations and nothing will happen

Copy link
Contributor

@SISheogorath SISheogorath Feb 28, 2025

Choose a reason for hiding this comment

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

I don't think you can run migration pre-install. Because you won't have a database at that point (at least when you use the one from the chart dependencies) 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true definitely. That's why I've since updated this PR to include an additional job that will run db:prepare as a pre-install hook to set up the the database schema (if it's not already there) before anything is installed.

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