[DEVOPS-1537 & DEVOPS-1519] Update dbo.Migrations table to support own migration schema #3212

michalchecinski · 2023-08-18T11:42:29Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

In the Migrator project create our own custom journaling, so that we can add migration scripts as repeatable in the migrations table.

Code changes

util/Migrator/RerunableSqlTableJournal.cs: Sql MIgrations Journal implementation. Our own implementation for Journal to support re-runnable SQL migrations as described in DEVOPS-1519 Jira issue.
util/Migrator/RerunableSqlTableJournalExtensions.cs Add extension for RerunableSqlTableJournal to use in fluent way in util/Migrator/DbMigrator.cs
util/Migrator/DbMigrator.cs Change DbUp JournalToSqlTable to RerunableSqlTableJournal using RerunableSqlTableJournalExtensions.

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

bitwarden-bot · 2023-08-18T12:02:59Z

Checkmarx One – Scan Summary & Details – d423a448-8c1c-494c-b6ab-ad564e2bc87a

New Issues

Issue	Source File / Package	Checkmarx Insight
Passwords And Secrets - Generic Password	/database.yml: 69	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/infrastructure-tests.yml: 78	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/database.yml: 65	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/infrastructure-tests.yml: 91	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/infrastructure-tests.yml: 97	Query to find passwords and secrets in infrastructure code.

withinfocus

Is this still a work in progress? I added a few comments as I am interested in this change but ... I don't see any differences from what's already available.

withinfocus · 2023-08-18T14:30:44Z

util/Migrator/RerunableSqlTableJournalExtensions.cs

+
+namespace Bit.Migrator;
+
+public static class RerunableSqlTableJournalExtensions


⛏️ I'd call this just SqlTableJournalExtensions.

withinfocus · 2023-08-18T17:01:56Z

util/Migrator/RerunableSqlTableJournal.cs

+
+namespace Bit.Migrator;
+
+public class RerunableSqlTableJournal : TableJournal


🎨 I think it's fine to have this class in the extensions class, as it's only used there.

Sure. Done.

withinfocus · 2023-08-18T17:57:22Z

util/Migrator/RerunableSqlTableJournal.cs

+
+namespace Bit.Migrator;
+
+public class RerunableSqlTableJournal : TableJournal


💭 I see https://github.com/DbUp/DbUp/blob/master/src/dbup-sqlserver/SqlTableJournal.cs -- should that be used instead?

Let me give a little background on this. We want to support transition migrations as rerunnable and mark this fact in the migrations database. We split this implementation in two steps:

Reflect the current functionality in our own code (and passing it to the DbUp), to then :

Extend "RerunableSqlTableJournal" class to support marking a script as rerunnable - adding a column in the Migrations table, fill and check value when adding and getting the values from this table.

This PR is about the first point. This class is basically the same implementation as you posted, but we want to test the existing migration process with our own code, to then extended it.

OK, so it was split up. I realize that's not how the tasks are written, but I don't feel it's needed to review this functionality clone unless some value is being created; I think the two items above should be one change. I would think you could still use SqlTableJournal and override potentially just one method to add what you're describing.

If you want to be able to run something many times over, DbUp provides NullJournal as well -- could that be used instead?

We had this conversation with the DevOps team if we want/need to journal those migrations that must be run multiple times. We concluded that we do want to track which migrations have been run, but at the same time mark them as rerunnable to run multiple times.

withinfocus

Per my inline comment.

This reverts commit df15fa9.

…dbo.Migrations-table-to-support-repeatable-migrations

michalchecinski · 2023-09-05T15:12:45Z

Hey @withinfocus 👋 I just finished testing the whole utility. It's working as intended, so the PR is ready for review 😊

withinfocus · 2023-09-05T20:00:06Z

util/Migrator/DbMigrator.cs

@@ -24,6 +24,8 @@ public DbMigrator(string connectionString, ILogger<DbMigrator> logger)
    }

    public bool MigrateMsSqlDatabaseWithRetries(bool enableLogging = true,
+        bool rerunable = false,


🎨 Hate to do this since it's in so many places, but two Ns for "rerunnable". I suggest we go with a less obscure English word and use "repeatable".

withinfocus · 2023-09-05T20:00:21Z

util/Migrator/DbScripts/2023-08-24_00_AddRerunableColumnIndboMigrationsTable.sql

@@ -0,0 +1,3 @@
+ALTER TABLE dbo.Migration
+ADD Rerunable bit NOT NULL DEFAULT 0
+GO


⛏️ Newline.

withinfocus · 2023-09-05T20:01:44Z

util/Migrator/Migrator.csproj

@@ -2,6 +2,7 @@

  <ItemGroup>
    <EmbeddedResource Include="DbScripts\**\*.sql" />
+    <EmbeddedResource Include="DbScripts_data_migration\**\*.sql" />


❓ Is the _data_migration in docs somewhere, or a recommendation? DbDataMigrationScripts or DbDataMigrations is cleaner to me.

We've got another card to rename this folder: https://bitwarden.atlassian.net/browse/DEVOPS-1520

withinfocus · 2023-09-05T20:09:36Z

util/Migrator/SqlTableJournalExtensions.cs

+
+namespace Bit.Migrator;
+
+public static class SqlTableJournalExtensions


❓ Did you ever look into my comment about using NullJournal? Now that the implementation is here, what is the purpose of tracking scripts that you always want to run? If you have a folder for some that can always run, that's a use case for a different journal otherwise you'd keep using the one already in place. What am I missing in the requirements?

withinfocus · 2023-09-05T20:15:36Z

I started top down and left a few comments but the last one is the key and worth stepping back for a moment.

michalchecinski · 2023-09-11T10:18:28Z

After talking about it, we decided to not track the Transition migrations that have been applied to the database until they are applied from the migrations directory.

I'm closing the PR but not deleting the branch for further reference.

michalchecinski added 7 commits August 16, 2023 17:31

Add RerunableSqlTableJournal

d83554b

Add extension to use rerunable sql table journal

0d67ee7

Use rerunable sql journal

4949c62

format

e47d544

Enable logging

7a7585e

FIx

068c73b

Disable logging

1968d11

michalchecinski requested a review from a team August 18, 2023 11:43

withinfocus reviewed Aug 18, 2023

View reviewed changes

Rename to SqlTableJournalExtensions

2c3c869

michalchecinski enabled auto-merge (squash) August 21, 2023 16:18

Move RerunableSqlTableJournal to Extension class

683fd71

michalchecinski requested a review from withinfocus August 21, 2023 16:20

Fix usings

ee26db7

withinfocus requested changes Aug 21, 2023

View reviewed changes

michalchecinski requested a review from withinfocus August 22, 2023 17:01

michalchecinski added 13 commits August 23, 2023 17:40

Add rerunable schema

ff021b3

Format

47f8555

Fix typo

c606efe

Enable logging in db migrator

edc8139

add rerunable column in dbo migrations table migration

d413b4b

Trying

98168fe

Fix journal table name

d73dbfb

Trying to migrate first

a742316

After migration

d95c766

Testing

b5420e1

Add update from rerunable to not rerunable script

731be1f

Change name

3d65219

Add rerunable option and script folder name

d82025b

michalchecinski added 15 commits August 28, 2023 16:41

Add rerunable options and folder

5ecffda

Fix

5d7da92

Add transition (aka rerunable) migrations to Setup

d242e6e

Parse parameters on migrator utility

c6df753

Fix sql scripts

82c1742

Remove CreateSchemaTableSql as it'll be migrated using migration

928179e

Embed dbScripts_data_migration folder

8dab294

Remove testing sql script

50145fb

Add optins parsing nuget for msSqlMigratorUtility

3bbf36e

Fix sql journal

953e645

Ran dotnet format

c3f58e6

Comment out index

df15fa9

▫️Revert "Comment out index"

00056cb

This reverts commit df15fa9.

Merge remote-tracking branch 'origin/master' into DEVOPS-1537-Update-…

9ba7b94

…dbo.Migrations-table-to-support-repeatable-migrations

Disable logging

54e92b5

withinfocus requested changes Sep 5, 2023

View reviewed changes

michalchecinski changed the title ~~[DEVOPS-1537] Update dbo.Migrations table to support own migration schema~~ [DEVOPS-1537 & DEVOPS-1519] Update dbo.Migrations table to support own migration schema Sep 6, 2023

michalchecinski added 2 commits September 6, 2023 17:09

Add newline

3682633

Rename rerunable to repeatable

95c14e4

michalchecinski closed this Sep 11, 2023

auto-merge was automatically disabled September 11, 2023 10:18
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DEVOPS-1537 & DEVOPS-1519] Update dbo.Migrations table to support own migration schema #3212

[DEVOPS-1537 & DEVOPS-1519] Update dbo.Migrations table to support own migration schema #3212

michalchecinski commented Aug 18, 2023 •

edited

Loading

bitwarden-bot commented Aug 18, 2023 •

edited

Loading

withinfocus left a comment

withinfocus Aug 18, 2023

michalchecinski Aug 21, 2023

withinfocus Aug 18, 2023

michalchecinski Aug 21, 2023

withinfocus Aug 18, 2023

michalchecinski Aug 21, 2023

withinfocus Aug 21, 2023

michalchecinski Aug 22, 2023 •

edited

Loading

withinfocus left a comment

michalchecinski commented Sep 5, 2023

withinfocus Sep 5, 2023

withinfocus Sep 5, 2023

withinfocus Sep 5, 2023

michalchecinski Sep 6, 2023

withinfocus Sep 5, 2023

withinfocus commented Sep 5, 2023

michalchecinski commented Sep 11, 2023


		namespace Bit.Migrator;

		public static class RerunableSqlTableJournalExtensions


		namespace Bit.Migrator;

		public class RerunableSqlTableJournal : TableJournal


		namespace Bit.Migrator;

		public static class SqlTableJournalExtensions

[DEVOPS-1537 & DEVOPS-1519] Update dbo.Migrations table to support own migration schema #3212

[DEVOPS-1537 & DEVOPS-1519] Update dbo.Migrations table to support own migration schema #3212

Conversation

michalchecinski commented Aug 18, 2023 • edited Loading

Type of change

Objective

Code changes

Before you submit

bitwarden-bot commented Aug 18, 2023 • edited Loading

New Issues

withinfocus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalchecinski Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

withinfocus left a comment

Choose a reason for hiding this comment

michalchecinski commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

withinfocus commented Sep 5, 2023

michalchecinski commented Sep 11, 2023

michalchecinski commented Aug 18, 2023 •

edited

Loading

bitwarden-bot commented Aug 18, 2023 •

edited

Loading

michalchecinski Aug 22, 2023 •

edited

Loading