Skip to content

tapd+sqlc: separate SQL migrations from code migrations #1881

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

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

tapd+sqlc: separate SQL migrations from code migrations #1881

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18e9478
tapd+sqlc: separate SQL migrations from code migs
ViktorT-11 Nov 20, 2025
8e4edf6
scripts: ignore missing .down.sql for code migs
ViktorT-11 Nov 24, 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
25 changes: 24 additions & 1 deletion scripts/check-migration-numbering.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,25 @@ if [ ! -d "$migrations_path" ]; then
exit 1
fi

# is_sql_migration checks if a .up.sql file contains executable SQL
# statements and not just comments/whitespace/semicolons. The logic mirrors the
# Go helper the `migrate` package dependency uses in the `migrate.go` file.
is_sql_migration() {
local file="$1"
local cleaned

cleaned=$(
perl -0777 -pe '
s/^\x{FEFF}//;
s{/\*.*?\*/}{}gs;
s{--[^\r\n]*}{}g;
' "$file" 2>/dev/null | \
sed -E 's/[[:space:];]+//g'
)

[ -n "$cleaned" ]
}

# Get all unique prefixes (e.g., 000001, always 6 digits) from .up.sql files.
prefixes=($(ls "$migrations_path"/*.up.sql 2>/dev/null | \
sed -E 's/.*\/([0-9]{6})_.*\.up\.sql/\1/' | sort))
Expand All @@ -34,7 +53,11 @@ for i in "${!prefixes[@]}"; do
base_filename=$(ls "$migrations_path/${prefixes[$i]}"_*.up.sql | \
sed -E 's/\.up\.sql//')

if [ ! -f "$base_filename.down.sql" ]; then
# Error if the .up.sql is an SQL migration, but we're missing the
# corresponding .down.sql. This doesn't apply if the .up.sql file is a code
# migration.
if is_sql_migration "$base_filename.up.sql" && \
[ ! -f "$base_filename.down.sql" ]; then
Comment on lines +56 to +60
Copy link
Contributor

@ffranr ffranr Nov 25, 2025

Choose a reason for hiding this comment

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

Updated:

Do we need to include this commit? Is it required for your end goal?

We don’t often have no-op down migrations, but IMO it’s fine to include them. It keeps the migration sequence contiguous, and IMO it’s cleaner to always provide a down file for every up, even if the down is empty.

Copy link
Contributor Author

@ViktorT-11 ViktorT-11 Nov 25, 2025

Choose a reason for hiding this comment

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

Do we need to include this commit? Is it required for your end goal?

It’s not required, but I want to explain why I think this change is beneficial.

This change specifically targets code migrations, allowing them to omit a down.sql file when appropriate.

To illustrate why SQL and code migrations should behave differently, consider the following example of migrations:

  • 0001_example.up.sql
    CREATE TABLE example (id INTEGER PRIMARY KEY);

  • 0001_example.down.sql
    DROP TABLE example;

  • Followed by a code migration:
    0002_example_code_migration.up.sql
    (This migration inserts data into the example table.)

If we also add a down.sql for the code migration:

  • 0002_example_code_migration.down.sql
    (This would simply re-run the code migration and insert the data again.)

This highlights the mismatch:

  • For SQL migrations, the down.sql undoes the up.sql.
  • For code migrations, a down.sql would unintentionally re-execute the migration instead of undoing the changes.
    In this example, the data inserted by the code migration would actually be removed only when the table itself is dropped by 0001_example.down.sql.

Currently, the code migrations in tapd are structured as no-ops when re-executed, but that assumption can't be true for all code migrations. For example, it'd be much more complex for migrations like the kvdb → SQL migration in litd.

For that reason, I think the code migration itself should indicate whether a down.sql should be present or not, rather than having CI enforce for all types of migrations. The migrate library explicitly supports migrations without a down.sql file, and I think this is a case where that functionality makes sense to use.

Copy link
Contributor Author

@ViktorT-11 ViktorT-11 Nov 25, 2025

Choose a reason for hiding this comment

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

We don’t often have no-op down migrations, but IMO it’s fine to include them. It keeps the migration sequence contiguous, and IMO it’s cleaner to always provide a down file for every up, even if the down is empty.

And just to clarify, this if clause here will only trigger when the up.sql case contains any executable content, i.e. when the up.sql is an SQL migration. It's not checking if the down.sql file is empty.

echo "Error: Missing .down.sql file for migration $expected_prefix."
exit 1
fi
Expand Down
Loading