feat: Add migration loader strategies with SQL grouping and docs#1582
Conversation
Adding feature tests for custom loaders, regression tests and documentation.
peturv
changed the title
peturv
changed the title
|
I did this on kind of short notice. Have some nodejs services I'm working on that I need to get to production state and db-versioning is part of that. So when I was looking around for good tools yesterday I stumbled upon this project that looks quite solid. But since a lot of my workflow is based on DDL in sql-files, and I saw there were a couple of other questions around this, I figured I might as well suggest a simple solution that enables users to manage this by themselves if they want to. Cheers and kudos for a nice tool! |
|
Hey @peturv 🚀 You should run It looks like cockroach db tests are running into errors 🤔 when the pipeline runs green, I will do a deep review 🙂 |
…e-pg-migrate.ts to avoid ambiguity, harmonizing the import with that of runner.
Thanks for the quick look. Have addressed the issues that were reported, a build should be on the way. |
|
I'll see what I can do about the failing the cockroach integration test. Didn't think I was close to touching that. |
|
the following comment is written by claude CockroachDB Test Failure AnalysisFailing StepMigration Root Cause
const sortedMigrationUnits = migrationUnits.toSorted((a, b) =>
a.filePaths[0].localeCompare(b.filePaths[0])
);The original sorting in This difference changes the sort order for files that share the same numeric prefix:
With plain Suggested FixReuse the existing sort helpers from const sortedMigrationUnits = migrationUnits.toSorted((a, b) =>
a.filePaths[0].localeCompare(b.filePaths[0], undefined, {
numeric: true,
sensitivity: 'variant',
ignorePunctuation: true,
})
);Ideally, extract |
Yes sir, thank you! Had to fix dinner here at home :) Extracted the localeCompareStringsNumerically function and added a very specific test. Did not touch compareFileNamesByTimestamp for now. Have run against a local dockerized cockroach to validate integration as well. I do hope that's the last of my mistakes. I'm going to blame my rush, and be thankful that you have a good test suite :) |
|
the following comment is written by claude PR Review: feat: Add migration loader strategies with SQL grouping and docsOverallGood feature addition that addresses two long-standing issues (#786, #937). The strategy pattern is clean, backward-compatible by default, and well-documented. The fix for the CockroachDB sort regression in the latest commit shows responsiveness. Test coverage is solid. A few items worth addressing before merge: Architecture & Design
The original Consider reusing the full CodeDuplicate
Both
This means that if a user switches from a single TestsGood coverage overall. A few gaps:
Docs
Nits
SummaryThis is a solid, well-scoped feature. The main structural concern is the sort divergence between |
|
Solid feedback. Will have a look either later tonight or tomorrow and get back with changes addressing these points. |
Corrections of how jiti is exported to node-pg-migrate for legacy build.
Documentation updates. Clarification of SQL migration loader behaviour. Added more tests for utilities and loader. Moved tests to separate files. Renamed testfiles to follow test target file name. "Nits" addressed.
Shinigami92
left a comment
Shinigami92
left a comment
There was a problem hiding this comment.
First real human-read iteration done.
If I would ever get such clean contributions, my life would be so much easier 💚
Only some small minor things/chors left, after these I can merge and release
Thank you for the kind words. I've added a commit that addresses your feedback, but not clicking resolve on anything until the pipeline is finished. |
2 participants
Summary
Since database migrations are a sensitive area that should be treated with care, I've tried to make my changes to existing code as small as possible, while also putting effort into ensuring that existing behavior is not changed unless loading strategy configuration is applied.
Behavior and compatibility
Test coverage added
Docs
Validation
pnpm run test:unitpnpm run docs:build