Update migrate.ps1 to support test database used by integration tests
#4912
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4912 +/- ##
=======================================
Coverage 41.75% 41.75%
=======================================
Files 1363 1363
Lines 63915 63915
Branches 5853 5853
=======================================
Hits 26686 26686
Misses 36026 36026
Partials 1203 1203 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
There was a problem hiding this comment.
This makes sense to me but requesting a review from @rkac-bw to confirm this retroactive change is OK.
There was a problem hiding this comment.
Yes, the change for MySQL makes sense. The existence check for the Id column in the Grant table is scoped to the current database schema. This prevents false positives when multiple databases are present on the same server, avoiding unintended changes to other databases.
| @($mysql, "MySQL", "MySqlMigrations", "mySql", 2), | ||
| @($postgres, "PostgreSQL", "PostgresMigrations", "postgreSql", 0), | ||
| @($sqlite, "SQLite", "SqliteMigrations", "sqlite", 1) |
There was a problem hiding this comment.
The test attributes (DatabaseData, DatabaseTheory) allow these providers to be defined in any order, but this script assumes that postgres is 0, sqlite is 1, mySql is 2 and mssql is 3. That won't be the case for everyone, depending on how they've set up their user secrets.
You could add logic here iterating over the user secrets to find the index for each database type, but it might make more sense to update the structure of the user secrets themselves to adopt a more conventional key-value format, like test:database:mysql:connectionString. That would also require updating the test attributes, so it's a little in the weeds, but it would make sure this Just Works for everyone.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-10863
📔 Objective
The integration tests for databases use different
secrets.jsonproperties than the ones used by locally running servers. This is to support not corrupting folk's local test data.Currently these test databases are not supported by
migrate.ps1and other tooling. This PR updatesmigrate.ps1with a--testswitch, and adds logic for connecting to and migrating these databases.The
--allswitch was modified to include running migrations against test databases as well.🧮 Side Effects
As part of this work I had to modify an old MySql migration because of an error in its manually written logic. The script in question checks for the existence of a column from system tables before trying to drop it. This doesn't check for a specific databases, and so can cause false positives if you try to have multiple databases on the same server.
📸 Screenshots
In this demo I build fresh databases from scratch and run some migrations using
migrate.ps1. It:pwsh migrate.ps1 --allcommand that skips test databases because no connection strings are found.pwsh migrate.ps1 --allagain, this time with connection strings setpwsh migrate.ps1 --test --{DATABASE}for targeting specific test databases.migrate.mov
⏰ Reminders before review
🦮 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