Skip to content

Better make test-schema#428

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.

Sign up for GitHub

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

Merged
rickyrombo merged 3 commits into from
Sep 30, 2025
Merged

Better make test-schema#428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47ff33a
Run migrations by default when using the 'migrate' command, default t…
rickyrombo Sep 29, 2025
398a039
only run migrations for test db
rickyrombo Sep 29, 2025
db8b965
Better migrations runner
rickyrombo Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions Makefile
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,8 +41,10 @@ test-schema::
@set -a; \
. .env; \
if [ -z "$$writeDbUrl" ]; then \
echo "writeDbUrl is not set in .env"; \
exit 1; \
echo "writeDbUrl is not set in .env - using test db and running migrations"; \
writeDbUrl=postgresql://postgres:example@localhost:21300/postgres; \
make migrate; \
fi; \
adjustedUrl=$$(echo "$$writeDbUrl" | sed 's/localhost/host.docker.internal/g'); \
docker compose exec db bash -c "pg_dump '$$adjustedUrl' --schema-only --no-owner --no-acl > ./sql/01_schema.sql"
docker compose exec db bash -c "pg_dump '$$adjustedUrl' --schema-only --no-owner --no-acl > ./sql/01_schema.sql"; \
echo "schema dumped to ./sql/01_schema.sql"
6 changes: 0 additions & 6 deletions ddl/run_migrations.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,12 +9,6 @@ import (
)

func RunMigrations() error {
fmt.Println("Running migrations...")
if !config.Cfg.RunMigrations {
fmt.Println("Skipping migrations. Set env runMigrations=true to run.")
return nil
}

cmd := exec.Command("bash", "pg_migrate.sh")
cmd.Dir = "ddl"

Expand Down
13 changes: 8 additions & 5 deletions main.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -23,18 +23,22 @@ func main() {
command = os.Args[1]
}

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

@raymondjacobsonraymondjacobsonSep 29, 2025

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

@rickyromborickyromboSep 30, 2025

Copy link
Copy Markdown
ContributorAuthor

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

@raymondjacobsonraymondjacobsonSep 30, 2025

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

@schottraschottraSep 30, 2025

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.

@rickyromborickyromboSep 30, 2025

Copy link
Copy Markdown
ContributorAuthor

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

fmt.Println("Skipping migrations. Set env runMigrations=true to run.")
} else {
fmt.Println("Running migrations...")
ddl.RunMigrations()
}

switch command {
case "server":
{
ddl.RunMigrations()

fmt.Println("Running server...")
as := api.NewApiServer(config.Cfg)
as.Serve()
}
case "indexer":
{
ddl.RunMigrations()
fmt.Println("Running indexer...")
_, err := indexer.NewIndexer(indexer.CoreIndexerConfig{
DbUrl: config.Cfg.WriteDbUrl,
Expand All@@ -53,7 +57,6 @@ func main() {
}
case "solana-indexer":
{
ddl.RunMigrations()
fmt.Println("Running solana-indexer...")
solanaIndexer := solana_indexer.New(config.Cfg)
defer solanaIndexer.Close()
Expand All@@ -70,7 +73,7 @@ func main() {
}
case "migrate":
{
ddl.RunMigrations()
// no-op, handled prior to switch/case
os.Exit(0)
}
default:
Expand Down
Loading