Skip to content
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

Add drivers for Xata #1902

Merged
merged 19 commits into from
Mar 19, 2024
Merged

Add drivers for Xata #1902

merged 19 commits into from
Mar 19, 2024

Conversation

SferaDev
Copy link
Contributor

@SferaDev SferaDev commented Feb 19, 2024

No description provided.

Signed-off-by: Alexis Rico <sferadev@gmail.com>
@SferaDev SferaDev force-pushed the xata branch 4 times, most recently from 502bbf9 to 03227aa Compare March 7, 2024 07:42
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
@SferaDev SferaDev marked this pull request as ready for review March 7, 2024 09:09
await db.session.execute(sql.raw(stmt));
}

rowsToInsert.push(sql`insert into ${sql.identifier(migrationsSchema)}.${sql.identifier(migrationsTable)} ("hash", "created_at") values(${migration.hash}, ${migration.folderMillis})`);
Copy link
Member

Choose a reason for hiding this comment

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

Depending on the results you want to see here, it may be either a good or a bad decision.

So, currently, here is what will happen:

You have 2 files with migrations (migration_1.sql, migration_2.sql):

  • If the migration process fails after migration_1.sql is applied, the row in the __drizzle_migrations table won't be created, and you will be in a situation where you have all statements from the first file applied, but nothing is stored about that in the database.

As a user, I would expect the migrator to store information about the applied migration file in the database. Therefore, perhaps you need to run this insert query here and not on lines 46-48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the example set by Neon's HTTP migrator: https://github.com/drizzle-team/drizzle-orm/blob/main/drizzle-orm/src/neon-http/migrator.ts#L37-L50

Maybe theirs should also be updated?

Copy link
Member

Choose a reason for hiding this comment

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

good point, will need to change it as well, I guess I missed it on a review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndriiSherman AndriiSherman changed the base branch from main to beta March 14, 2024 18:20
AndriiSherman and others added 4 commits March 14, 2024 20:20
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
@SferaDev
Copy link
Contributor Author

@AndriiSherman Updated with the changes from #2033

@AndriiSherman AndriiSherman merged commit c9a0caa into drizzle-team:main Mar 19, 2024
7 checks passed
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.

2 participants