Add: Dashboard db migrations & tooling #6
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
In readme you are missing bun as a prerequisite, also it needs to be 1.2 + because of SQL. I guess npm is not supported anymore.
Not sure if we want to replicate migration logic here, like splitting sql statements etc, why are we solving already solved problem.
AI is really good, but not checking the code afterwards or testing it ngmi..
scripts/apply-migrations.ts
Outdated
| async function ensureMigrationsTable() { | ||
| try { | ||
| await db(` | ||
| CREATE TABLE IF NOT EXISTS public._migrations ( |
There was a problem hiding this comment.
| CREATE TABLE IF NOT EXISTS public._migrations ( | |
| CREATE TABLE IF NOT EXISTS public._dahboard_migrations ( |
scripts/apply-migrations.ts
Outdated
| await db(` | ||
| GRANT ALL ON public._migrations TO supabase_admin; | ||
| `) |
There was a problem hiding this comment.
could you explain why you added?
scripts/apply-migrations.ts
Outdated
| */ | ||
| async function ensureMigrationsTable() { | ||
| try { | ||
| await db(` |
There was a problem hiding this comment.
this isn't working because correct syntax is
| await db(` | |
| await db` |
scripts/apply-migrations.ts
Outdated
| content: string | ||
| ): Promise<{ applied: boolean; checksumMatch: boolean }> { | ||
| // Query to get the existing migration record | ||
| const result = await db.unsafe( |
There was a problem hiding this comment.
there's no need to this to be unsafe
scripts/apply-migrations.ts
Outdated
| ): Promise<void> { | ||
| // Convert checksum to string to avoid bigint overflow | ||
| const checksum = String(Bun.hash(content)) | ||
| await db.unsafe( |
README.md
Outdated
| ### Migration Safety | ||
|
|
||
| The system includes several safety features: | ||
| - Migrations are applied in a transaction (all-or-nothing) |
i knew all of this. wanted to try the bun sql package. not the best decision for public use indeed.
fair.
code was of course checked & tested multiple times. i did bad decisions while doing so is a better way to put it. |
…f manual statement splitting
scripts/apply-migrations.ts
Outdated
There was a problem hiding this comment.
can we just delete it for now? We can add it later in separate pr and update the table manually
Co-authored-by: Jakub Novák <kubus.novak@gmail.com>
E.g. migrations weren't tested, at least not without some already existing tables, because it was failing as I mentioned in PR |
i tested it multiple times with multiple scenarios. maybe there was some edge case i did not caught. just want to mention that i of course test the code i write and not just push blindly on purpose. |
|
I applied it to my database, I think going from clean state isn't an edge case |
this pr moves dashboard specific migrations inside this repo. also it adds tooling to work with db migrations.