chore: add document-id migration #19514
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexandrebodin
added
source: core:database
|
Size Change: 0 B Total Size: 2.38 MB ℹ️ View Unchanged
|
alexandrebodin
force-pushed
the
v5/document-id-migration
branch
3 times, most recently
from
79d39f9
to
397e04a
Compare
alexandrebodin
force-pushed
the
v5/document-id-migration
branch
from
397e04a
to
b801c7d
Compare
Marc-Roig
approved these changes
Feb 17, 2024
There was a problem hiding this comment.
clean! 🚀
I created another ticket to migrate and generate the draft versions of documents when D&P is enabled
Comment on lines
12
to
15
| database: 'postgres', | ||
| user: 'postgres', | ||
| password: 'test', | ||
| port: 54321, |
There was a problem hiding this comment.
I would prefer to keep the old credentials, as all databases sahre the same ones! is there any reason to change these ones?
Comment on lines
134
to
136
| const hasLocalizationsJoinTable = async (knex: Knex, tableName: string) => { | ||
| return knex.schema.hasTable(`${tableName}_localizations_links`); | ||
| }; |
There was a problem hiding this comment.
I wonder how @innerdvations, about shortening table names, is going to affect this bit.
After the first migration to v5, I imagine the table names will have not been shortened in knex schema right?
There was a problem hiding this comment.
I will have to update it later, but we can merge this first and I'll take it into account for my feature and fix it.
I actually suspect we will find a lot of little places throughout the codebase with hardcoded names that I'm not currently aware of and can't easily discover until I start testing the shortening and see what breaks.
There was a problem hiding this comment.
indeed, I wanted to take a look when reviewing your PR!
packages/core/database/src/migrations/internal-migrations/document-id.ts
Outdated
Show resolved
Hide resolved
alexandrebodin
commented
Feb 19, 2024
packages/core/database/src/migrations/internal-migrations/document-id.ts
Outdated
Show resolved
Hide resolved
joshuaellis
reviewed
Feb 19, 2024
There was a problem hiding this comment.
What do you think about naming it something like 5.0.0-01-document-id.ts so that we have an easy way to see the version it was introduced in and the order in which the migrations are run? It's not (currently) enforced by the internal migrations loader because we're just using an array, but it could be added later, and in the meantime it would still be helpful to keep things organized as we add more migrations
There was a problem hiding this comment.
Not directly a concern of this PR, but since this is our first internal migration:
Do we have a plan or documentation for when a migration fails? It looks like in the case of a catastrophic failure, knex will leave the db lock in place and users will have to manually remove it: https://knexjs.org/guide/migrations.html#notes-about-locks
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Looking forward to this <3 |
alexandrebodin
commented
Mar 28, 2024
packages/core/database/src/migrations/internal-migrations/5.0.0-02-document-id.ts
Outdated
Show resolved
Hide resolved
alexandrebodin
commented
Apr 2, 2024
There was a problem hiding this comment.
Let's merge and start getting community members to test the migration cc @derrickmehaffy if you have some candidates for data migration tests soon :)
|
I would personally wait for #19813 too before testing the v4 data migration |
|
@innerdvations about the lock, I have only been testing this with sqlite, but at least I did not need to remove any lock if the migration failed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does it do?
Why is it needed?
to migrate from v4 to v5
How to test it?
test moving from v4 to v5 (main to v5/main) and restarting an app will add the document_id column and populate respecting localizations.
Tested
Related issue(s)/PR(s)
Let us know if this is related to any issue/pull request