Added clarification for why the migrations are done the way they are … #508

Patrick-Pimentel-Bitwarden · 2025-01-02T21:33:14Z

🎟️ Tracking

📔 Objective

Added clarification explaining why the migrations are done the way they are done.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
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 confirmed
issue 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

…done.

github-actions · 2025-01-02T21:36:40Z

Checkmarx One – Scan Summary & Details – 079bd54e-32f1-4e91-abc2-35df7b2d04df

No New Or Fixed Issues Found

withinfocus

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.

withinfocus · 2025-01-02T23:40:19Z

docs/contributing/database-migrations/index.md

+
+   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


Suggested change

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

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?

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!

"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.

Just to wordsmith it a bit:

The separate database definitions in src/Sql/.../dbo serve 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.

Works for me!

cloudflare-workers-and-pages · 2025-01-03T20:11:56Z

Deploying contributing-docs with Cloudflare Pages

Latest commit:	`eb0c328`
Status:	✅ Deploy successful!
Preview URL:	https://c5013276.contributing-docs.pages.dev
Branch Preview URL:	https://migration-clarification.contributing-docs.pages.dev

View logs

withinfocus · 2025-01-03T21:14:07Z

docs/contributing/database-migrations/index.md

     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


🎨 Pop this into a note or info box instead, higher up, probably right under the tip.

Fixed with 6b18eead

withinfocus

⛏️y but really close.

withinfocus · 2025-01-06T23:57:56Z

docs/contributing/database-migrations/index.md


+:::info
+
+Note on creating migrations: The separate database definitions in src/Sql/.../dbo serve as a


Suggested change

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

Fixed with c5151e7

Hinton · 2025-01-07T10:26:52Z

Some useful context.

src/Server is a regular sqlproj, https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects. But rather than using the auto generated migrations from DAC https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16 we need to manually write them to be performant and prevent accidental data loss.

This is why there are both a sqlproj and standalone migrations.

withinfocus · 2025-01-07T15:31:40Z

CF didn't deploy the latest so we might still have a trigger issue.

Patrick-Pimentel-Bitwarden · 2025-01-08T22:15:27Z

Some useful context.

src/Server is a regular sqlproj, https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects. But rather than using the auto generated migrations from DAC https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16 we need to manually write them to be performant and prevent accidental data loss.

This is why there are both a sqlproj and standalone migrations.

Thank you so much for linking this information. I went and added this additional context.

withinfocus

Hopefully the edits make sense. The pasted comment from Oscar needed adjustment.

withinfocus · 2025-01-09T16:10:34Z

docs/contributing/database-migrations/index.md

+:::info
+
+The separate database definitions in `src/Sql/.../dbo` serve as a "master" reference for the
+intended and final state of the database at that time. This is crucial because this state of files


Suggested change

intended and final state of the database at that time. This is crucial because this state of files

intended and final state of the database at that time. This is crucial because the state of database definitions

withinfocus · 2025-01-09T16:19:34Z

docs/contributing/database-migrations/index.md

+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.
+
+Additionally, `src/Server` is a regular `sqlproj` as described in the


Suggested change

Additionally, `src/Server` is a regular `sqlproj` as described in the

Additionally, a [SQL database project](https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects) is in place

withinfocus · 2025-01-09T16:19:54Z

docs/contributing/database-migrations/index.md

+maintain clarity and accuracy in the database schema management and synchronization processes.
+
+Additionally, `src/Server` is a regular `sqlproj` as described in the
+[SQL Database Project Extension SDK Style Projects](https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects).


Suggested change

[SQL Database Project Extension SDK Style Projects](https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects).

withinfocus · 2025-01-09T16:20:30Z

docs/contributing/database-migrations/index.md

+
+Additionally, `src/Server` is a regular `sqlproj` as described in the
+[SQL Database Project Extension SDK Style Projects](https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects).
+However, instead of using the auto-generated migrations from DAC, as detailed in


Suggested change

However, instead of using the auto-generated migrations from DAC, as detailed in

however instead of using the auto-generated migrations from [DAC](https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16)

withinfocus · 2025-01-09T16:21:41Z

docs/contributing/database-migrations/index.md

+Additionally, `src/Server` is a regular `sqlproj` as described in the
+[SQL Database Project Extension SDK Style Projects](https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects).
+However, instead of using the auto-generated migrations from DAC, as detailed in
+[Data-tier Applications](https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16),


Suggested change

[Data-tier Applications](https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16),

withinfocus · 2025-01-09T16:23:02Z

docs/contributing/database-migrations/index.md

+However, instead of using the auto-generated migrations from DAC, as detailed in
+[Data-tier Applications](https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16),
+we manually write migrations. This approach is chosen to enhance performance and prevent accidental
+data loss. This is why there are both a `sqlproj` and standalone migrations.


Suggested change

data loss. This is why there are both a `sqlproj` and standalone migrations.

data loss, and is why there are both a `sqlproj` and standalone migrations.

withinfocus

⛏️ on grammar.

withinfocus · 2025-01-09T18:08:43Z

docs/contributing/database-migrations/index.md

+separation helps maintain clarity and accuracy in database schema management and synchronization
+processes.
+
+Additionally, an


Suggested change

Additionally, an

Additionally, a

withinfocus · 2025-01-09T18:09:07Z

docs/contributing/database-migrations/index.md

+is in place; however, instead of using the auto-generated migrations from
+[DAC](https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16),
+we manually write migrations. This approach is chosen to enhance performance and prevent accidental
+data loss, which is why we have both an `sqlproj` and standalone migrations.


Suggested change

data loss, which is why we have both an `sqlproj` and standalone migrations.

data loss, which is why we have both a `sqlproj` and standalone migrations.

I suppose I'm an outlier how I pronounce it 😅

Added clarification for why the migrations are done the way they are …

b9a3ce5

…done.

Patrick-Pimentel-Bitwarden marked this pull request as ready for review January 2, 2025 22:13

Patrick-Pimentel-Bitwarden requested a review from a team as a code owner January 2, 2025 22:13

withinfocus reviewed Jan 2, 2025

View reviewed changes

Changed how the note was made from the feedback that was given.

37e585e

withinfocus reviewed Jan 3, 2025

View reviewed changes

Moved where the note is.

6b18eea

withinfocus reviewed Jan 6, 2025

View reviewed changes

Removed leading sentence.

c5151e7

withinfocus previously approved these changes Jan 7, 2025

View reviewed changes

Added additional context provided by Oscar

060499b

Patrick-Pimentel-Bitwarden dismissed withinfocus’s stale review via 060499b January 8, 2025 22:15

Patrick-Pimentel-Bitwarden requested a review from withinfocus January 9, 2025 14:28

withinfocus requested changes Jan 9, 2025

View reviewed changes

Fixed grammar and phraseololgy.

2769117

withinfocus reviewed Jan 9, 2025

View reviewed changes

Fixed grammar for sql.

eb0c328

withinfocus approved these changes Jan 9, 2025

View reviewed changes

Patrick-Pimentel-Bitwarden merged commit 05ab1d9 into main Jan 9, 2025
9 checks passed

Patrick-Pimentel-Bitwarden deleted the migration-clarification branch January 9, 2025 18:35

	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


		:::info

		Note on creating migrations: The separate database definitions in src/Sql/.../dbo serve as a

	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

	intended and final state of the database at that time. This is crucial because this state of files
	intended and final state of the database at that time. This is crucial because the state of database definitions

	Additionally, `src/Server` is a regular `sqlproj` as described in the
	Additionally, a [SQL database project](https://learn.microsoft.com/en-us/azure-data-studio/extensions/sql-database-project-extension-sdk-style-projects) is in place

	However, instead of using the auto-generated migrations from DAC, as detailed in
	however instead of using the auto-generated migrations from [DAC](https://learn.microsoft.com/en-us/sql/relational-databases/data-tier-applications/data-tier-applications?view=sql-server-ver16)

	data loss. This is why there are both a `sqlproj` and standalone migrations.
	data loss, and is why there are both a `sqlproj` and standalone migrations.

	data loss, which is why we have both an `sqlproj` and standalone migrations.
	data loss, which is why we have both a `sqlproj` and standalone migrations.

Added clarification for why the migrations are done the way they are … #508

Added clarification for why the migrations are done the way they are … #508

Uh oh!

Conversation

Patrick-Pimentel-Bitwarden commented Jan 2, 2025

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No New Or Fixed Issues Found

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patrick-Pimentel-Bitwarden Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Why separate stored procedure files?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudflare-workers-and-pages bot commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying contributing-docs with Cloudflare Pages

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hinton commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

withinfocus commented Jan 7, 2025

Uh oh!

Patrick-Pimentel-Bitwarden commented Jan 8, 2025

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

github-actions bot commented Jan 2, 2025 •

edited

Loading

Patrick-Pimentel-Bitwarden Jan 3, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 3, 2025 •

edited

Loading

Hinton commented Jan 7, 2025 •

edited

Loading