-
-
Notifications
You must be signed in to change notification settings - Fork 495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): migration:rollup command #3633
base: master
Are you sure you want to change the base?
Conversation
// for snapshots, we always want to use the path based on `emit` option, regardless of whether we run in ts-node context | ||
/* istanbul ignore next */ | ||
const snapshotDir = options.emit === 'ts' && options.pathTs ? options.pathTs : options.path!; | ||
const absoluteSnapshotPath = Utils.absolutePath(snapshotDir, orm.config.get('baseDir')); | ||
const dbName = basename(orm.config.get('dbName')); | ||
const snapshotName = options.snapshotName ?? `.snapshot-${dbName}`; | ||
const snapshotPath = Utils.normalizePath(absoluteSnapshotPath, `${snapshotName}.json`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original intent was to use migrator.snapshotPath
. However it seems to be a private property right now and is not a part of the IMigrator
interface. Some refactoring may be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, is there a way to bypass having to delete the snapshot in order to roll up multiple migrations that are reflected in the snapshot?
// Create the rollup | ||
await this.handleCreateCommand(migrator, args, orm.config); | ||
|
||
// Execute the rollup | ||
await this.handleUpDownCommand(args, migrator, 'up'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm questioning if it's a good idea to use the other command handlers directly or if I should try to implement all logic directly at a lower level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, I'm questioning whether or not the rollup should be automatically executed. In order to stay consistent with the create command, it most likely should not.
await expect(cmd.handler({} as any)).resolves.toBeUndefined(); | ||
expect(down.mock.calls.length).toBe(1); | ||
expect(down.mock.calls[0][0]).toEqual({}); | ||
expect(closeSpy).toBeCalledTimes(1); | ||
await expect(cmd.handler({ to: 'a' } as any)).resolves.toBeUndefined(); | ||
expect(down.mock.calls.length).toBe(2); | ||
expect(down.mock.calls[1][0]).toEqual({ to: 'a' }); | ||
expect(closeSpy).toBeCalledTimes(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test needs more work, it's mostly just a stub for now. How do you mock filesystem commands (e.g. remove and pathExists)?
Codecov ReportBase: 99.89% // Head: 99.88% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3633 +/- ##
==========================================
- Coverage 99.89% 99.88% -0.01%
==========================================
Files 211 211
Lines 13192 13213 +21
Branches 3059 3063 +4
==========================================
+ Hits 13178 13198 +20
- Misses 14 15 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
} | ||
|
||
// Delete migrations | ||
// TODO(marek) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we need to delete migration files, it may make sense to not run the down command above but rather re-implement it so we get access to the list of migrations here.
This is actually a very nice feature to have. "collapsing" 10 migrations into 1 is something really useful down the line. |
The idea is good, I am not entirely sure about the implementation. I don't like that it first migrates all the way down and up again, it could mess up with the data easily, e.g. every migration that created a column will first drop it and recreate again - but without data. This could even potentically fail if its non null without a default, as that works only on tables without any rows. Why don't we just merge the contents of the migrations, instead of messing up with the database? |
Totally. This was the only way I was able to get the rollup to work without writing a lot of custom logic. Also, you technically need to roll back to the first migration included in the rollup so you can do the diff and generate the rolled-up migration that contains all changes. Obviously, this wouldn't work if you wanted to run it in a non-local environment. This particular implementation is only good for rolling up migrations in feature branches as part of the final cleanup. I was inspired by a similar feature in Doctrine (PHP world): I'd be happy to revisit this but was holding to get some feedback from the community and I also outlined some questions as I wasn't sure what's the best way to do certain things (SDK-wise) |
But why? You have the migrations, they contain the queries, why recompute everything instead of just concatenating? |
You're not wrong. But consider you adding a new table, then adding a few more columns in the following migrations. If we just combine all migrations, there will be extraneous statements whereas it could be optimized to become a single statement. I know it's an optimization but there are other cases where you're experimenting with different states and so some migrations become unnecessary - which again - you could edit manually after the rollup. I'm impartial here and would be happy to explore the other approach which I did consider but again - it requires writing more custom logic - the solution contained in this PR only relies on existing commands. |
I'd go with concatenation, as it will be safer (no chance of data loss) as well as faster (no queries involved), and you don't need to touch the snapshot file or the schema state at all. I don't mind supporting the down/up way too, but rather as opt-in than default, could be a Btw don't be afraid of doing changes around the codebase, this does not have to be just a self-contained CLI command - if you need to modify the snapshot, you can add some methods to the migrator, we can mark them as internal via jsdoc if they need to be public. Maybe better target the v6 branch nowadays, I would be up to merging something not fully complete in there too and polish it a bit myself before the final release (which will take a few more months for sure). |
@B4nan thank you for your feedback, Martin. I'll take a stab at this. It seems that I should be able to rely on the methods available in the
How does that sound? |
Migrations can contain code that directly works with the database (e.g. |
That's actually a great callout. I was trying to avoid having to use a JS/TS parser but I'm now not sure it's avoidable. I'd rather not rely on regex matching. If not, I'll have to use one of the available parsers (see the list below) in order to extract |
As we iterate on database schemas in feature branches, the number of database migrations tends to grows fairly quickly. With an ever-increasing number of database migrations, it takes increasingly more time to run these migrations in the CI/CD environment and locally too. This PR introduces a convenient way to roll up (or combine) a list of migrations into a single migration. This is somewhat inspired by a similar Doctrine command. This command could eventually be used to roll up DB migrations in non-local environments too.
TODO: