Feature/add rollback all to cli #3187
Conversation
|
@leeallen337 Well yeah, it obviously runs all migrations that it can find. Probably the best approach to address that would be creating migrations programmatically and then deleting them after test. Then you could incrementally add up new migrations. |
test/jake/jakelib/migrate-test.js
Outdated
| test('migrate:rollback --all rolls back all completed migrations', (temp) => { | ||
| return ( | ||
| new Promise((resolve, reject) => { | ||
| fs.writeFile( |
There was a problem hiding this comment.
I would recommend using writeFileSync for simplicity, we don't care about performance in tests much.
test/jake/jakelib/migrate-test.js
Outdated
|
|
||
| .then(() => { | ||
| return new Promise((resolve, reject) => { | ||
| fs.writeFile( |
There was a problem hiding this comment.
if after using writeFileSync and ensuring that files indeed exist at the moment you are running migrations, you still are only running the very first migration, I would recommend checking if knexfile.js has correct path to migrations directory, could be that it's looking in a different place.
There was a problem hiding this comment.
Maybe I'm way off base here but why would it only run the very first migration even if there are more migrations in that directory? Shouldn't migration:latest run all migrations that haven't been run yet?
There was a problem hiding this comment.
It should run all of them, which is why I'm slightly suspicious about async write not completing when expected or knex reading from wrong directory.
There was a problem hiding this comment.
That makes sense. I'll investigate
|
@leeallen337 I'm planning to cut a new version on this weekend. Should I wait for this PR to be ready first, or you don't expect it completed until next week? |
|
@kibertoad Don't wait up for this PR. I don't want to hold up a new version release. I don't know if I'll be able to get it completed until next week |
|
@leeallen337 OK! I guess we could do a follow-up one later on. |
|
@kibertoad I actually think I fixed the tests. It was a mix of me thinking that I needed to use the So 100% human error 😂 Let me know what you think of the changes. I need to step away to get some food but if you want me to make any changes I should be able to make them tonight or tomorrow |
|
@leeallen337 Thank you, looks great! |
leeallen337
changed the title
This pull request adds support for rolling back all completed migrations to the CLI.
This would, in part, address the feature requests in the following issues: #783 and #2907
I've labeled this still a work in progress because I'm having difficulty getting the test to work the way I want. Perhaps that's me just not understanding how the testing is set up but here's what I'm trying to accomplish:
migration:latestmigration:latestmigrate:rollback --allto ensure that migrations 002 and 001 were run in that order and it did not run 003.The problem is that it only runs the first migration (i.e. migration 001) and no additional migrations.
The reason behind having to create and run them incrementally like that is because running
migrate:latestwould just create one batch and rolling back all wouldn't really be testing it correctly since we want to make sure it rolls back all batches. I've left some commented out code (e.g. console.logs) in there in case it might help troubleshoot.@kibertoad any instruction you might be able to provide around the testing would be appreciated!