Added clarification for why the migrations are done the way they are … #508
Conversation
|
No New Or Fixed Issues Found |
There was a problem hiding this comment.
There's coverage of this at https://contributing.bitwarden.com/contributing/database-migrations/edd as well and I see it's linked right above this -- is this perhaps duplicative?
Also, not sure why CF Pages didn't deploy this for previewing, may need to check with BRE.
|
|
||
| This directory is used for storing migration scripts that will be executed to transition the | ||
| database from its current state to the new state as defined in `src/Sql/dbo`. Each script must be | ||
| prefixed with the current date to ensure they are run in the correct order. These scripts are |
There was a problem hiding this comment.
| prefixed with the current date to ensure they are run in the correct order. These scripts are | |
| prefixed with the current date to ensure they are executed in the correct order. These scripts are |
There was a problem hiding this comment.
I reread through edd link you sent again and I think I still feel like if someone asked: "Why do we create a separate stored procedure file in the src/Sql/.../dbo/Stored Procedures from the stored procedure in the migration file" I'm not sure what line/paragraph of that documentation I would quote or reference. I was aiming to add a line explaining/justifying why we do that explicitly. If I'm missing something would you mind pointing to what explains that?
There was a problem hiding this comment.
When I read the "Migrations" section on the EDD page it's covering what you briefly summarized in your expansion of step 2 here. Step 1's expanded content could probably be mentioned there as well, with the mention of "current state".
I think I reacted to this being related to EDD and the content there in part because of your language around "current" and "intended" -- those terms are true at the moment in time of the change. This is subtle but important, because what's in that (what I call "master") file today isn't what was in it when a migration a year ago was added. Additionally, we keep the master files to serve as a lint / validation step more than anything, to ensure migrations work as expected.
I might not be the best person to comment on this as I've had a lot to do with the migrations and EDD processes here though 😜 ... so maybe with just some wording tweaks and section-level links to the EDD content this is just fine; others should weigh in!
There was a problem hiding this comment.
"This is subtle but important, because what's in that (what I call "master") file today isn't what was in it when a migration a year ago was added. Additionally, we keep the master files to serve as a lint / validation step more than anything, to ensure migrations work as expected."
I think this would serve as a great addition to explicitly put somewhere. I did ping on the code channel and got some feedback as well. Between asking around the auth team/public channels it seems like there is a noticeable sentiment of ambiguity around the full reason why we do this so I was hoping to clarify that in the docs here. You are the third or forth person who has said "I might not be the best person to comment on this" or "This could also be being used in ways I don't know about though." or "This has also caused me pause... I do not know why though. I assume something to do with EDD..."
How does something like this sound:
Why separate stored procedure files?
The separate stored procedure files in src/Sql/.../dbo/Stored Procedures serve as a "master" reference for the current intended state of the database. This is crucial because the state of these files today may differ from when a migration was added a year ago. They act as a lint/validation step to ensure that migrations work as expected. This separation helps maintain clarity and accuracy in the database schema management process as well as there are build scripts to ensure they stay synced.
There was a problem hiding this comment.
Just to wordsmith it a bit:
The separate database definitions in
src/Sql/.../dboserve as a "master" reference for the intended and final state of the database at that time. This is crucial because this state of files at the current moment may differ from when a migration was added in the past. These definitions act as a lint and validation step to ensure that migrations work as expected, and the separation helps maintain clarity and accuracy in the database schema management and synchronization processes.
There was a problem hiding this comment.
Works for me!
Deploying contributing-docs with
|
| Latest commit: |
6b18eea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ebc2e651.contributing-docs.pages.dev |
| Branch Preview URL: | https://migration-clarification.contributing-docs.pages.dev |
| @@ -54,6 +54,13 @@ required. | |||
| compatible with the changes to DbScripts. In order to achieve this we only keep a single | |||
| migration, which executes all backwards incompatible schema changes. | |||
|
|
|||
| > **Note on creating migrations:** The separate database definitions in src/Sql/.../dbo serve as a | |||
There was a problem hiding this comment.
🎨 Pop this into a note or info box instead, higher up, probably right under the tip.
There was a problem hiding this comment.
Fixed with 6b18eead
| @@ -27,6 +27,17 @@ Style][code-style-sql] first, since they have a major impact in how we write mig | |||
|
|
|||
| ::: | |||
|
|
|||
| :::info | |||
|
|
|||
| Note on creating migrations: The separate database definitions in src/Sql/.../dbo serve as a | |||
There was a problem hiding this comment.
| Note on creating migrations: The separate database definitions in src/Sql/.../dbo serve as a | |
| The separate database definitions in src/Sql/.../dbo serve as a |
|
Some useful context.
This is why there are both a |
🎟️ Tracking
📔 Objective
Added clarification explaining why the migrations are done the way they are done.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes