Skip to content

Better make test-schema - #428

Merged
rickyrombo merged 3 commits into
mainfrom
mjp-test-schema-migrate
Sep 30, 2025
Merged

Better make test-schema#428
rickyrombo merged 3 commits into
mainfrom
mjp-test-schema-migrate

Conversation

@rickyrombo

Copy link
Copy Markdown
Contributor
  • Default to test db for make test-schema
  • Run migrations regardless of env file config when running make migrate

Comment thread main.go
command = os.Args[1]
}

if !config.Cfg.RunMigrations && command != "migrate" {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i'd prefer this be wet and still called inside each switch. someone adding a switch in the future may accidentally wreck a db not knowing this auto runs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

after thinking about it some more, i feel like this might be a non-issue or at least quite a rare occurrence. for it to be a problem, you'd have to:

  1. Be connected to an existing db, like prod/stage
  2. Have runMigrations=true
  3. Have a new command you're adding
  4. Have a bad new migration that does something you don't want to do on those dbs

I think it's probably easier to reason around having any command will run migrations if runMigrations=true vs all the special casing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fair enough

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we actually want every command to run migrations though? It means when we deploy, every container in every replica tries to grab the lock to run the migrations, when we only need one of them to do it. Just seems like a lot more thrash for the benefit of only tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is equivalent to what it was before, besides elasticsearch. It makes a lot of sense for the dev flow, but I agree is a bit overkill for deploys. We could pick a single command to champion migrations but I think it's alright for now

Comment thread main.go
command = os.Args[1]
}

if !config.Cfg.RunMigrations && command != "migrate" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we actually want every command to run migrations though? It means when we deploy, every container in every replica tries to grab the lock to run the migrations, when we only need one of them to do it. Just seems like a lot more thrash for the benefit of only tests.

@rickyrombo
rickyrombo merged commit e0f393a into main Sep 30, 2025
5 checks passed
@rickyrombo
rickyrombo deleted the mjp-test-schema-migrate branch September 30, 2025 19:42
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.

3 participants