Skip to content

Switch to timestamp based migration IDs instead of sequential IDs #1969

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

Closed
deansheather opened this issue Jun 1, 2022 · 4 comments
Closed
Labels
api Area: HTTP API site Area: frontend dashboard stale This issue is like stale bread.

Comments

@deansheather
Copy link
Member

What is your suggestion?

Instead of using migration IDs like 1_add_table we should use timestamp-based IDs like 1654116390_add_table.

Why do you want this feature?

Using sequential IDs and enforcing uniqueness is difficult to do. We have a test to enforce uniqueness, but if you have two PRs at the same time that use the same migration ID, the tests will pass as the migrations are only checked for uniqueness against main and not each other. So theoretically you could merge two PRs with the same migration ID and cause fatal errors in prod.

See #1968 for an example where this happened and got past the linter.

Switching to Unix timestamp-based IDs avoids this problem as it will only happen if the two devs made the two different migrations at the exact same second, which given how many migrations we have right now, seems very unlikely. The same test we have for uniqueness can still be employed as well even though it's imperfect.

In Coder v1 we use timestamp migration numbers.

Alternatively we could use YYYY_MM_DD__HH_mm_add_table but that may be more work to support for little gain

Are there any workarounds to get this functionality today?

n/a

Are you interested in submitting a PR for this?

Maybe, but I think it would be better if someone more familiar with the migration code handled this

cc @kylecarbs @coadler @johnstcn

@kylecarbs
Copy link
Member

Very wise

@coadler
Copy link
Contributor

coadler commented Jun 1, 2022

TS based migrations have a lot of subtle bugs related to ordering. Since it's possible to merge a migration with an earlier timestamp, migrations can run out of order depending on how users upgrade to a specific version.

@ketang
Copy link
Contributor

ketang commented Jun 2, 2022

Why not go all the way to a UUID? We're not going to be at the relevant scale for a while, but I have seen collisions when using time as a unique ID, and that was millisecond level time.

@misskniss misskniss added api Area: HTTP API site Area: frontend dashboard labels Jun 14, 2022
@github-actions
Copy link

This issue is becoming stale. In order to keep the tracker readable and actionable, I'm going close to this issue in 7 days if there isn't more activity.

@github-actions github-actions bot added the stale This issue is like stale bread. label Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Area: HTTP API site Area: frontend dashboard stale This issue is like stale bread.
Projects
None yet
Development

No branches or pull requests

5 participants