Skip to content
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

Feature/add rollback all to cli #3187

Merged

Conversation

Projects
None yet
2 participants
@leeallen337
Copy link
Contributor

commented May 11, 2019

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:

  1. Create migration 001
  2. Run migration:latest
  3. Create migration 002
  4. Run migration:latest
  5. Create migration 003
  6. Run the cli for migrate:rollback --all to 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:latest would 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!

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

@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('migrate:rollback --all rolls back all completed migrations', (temp) => {
return (
new Promise((resolve, reject) => {
fs.writeFile(

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 11, 2019

Collaborator

I would recommend using writeFileSync for simplicity, we don't care about performance in tests much.


.then(() => {
return new Promise((resolve, reject) => {
fs.writeFile(

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 11, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@leeallen337

leeallen337 May 11, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 11, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@leeallen337

leeallen337 May 11, 2019

Author Contributor

That makes sense. I'll investigate

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

@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?

@leeallen337

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

@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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

@leeallen337 OK! I guess we could do a follow-up one later on.

@leeallen337

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

@kibertoad I actually think I fixed the tests. It was a mix of me thinking that I needed to use the sqlite3 get rather than sqlite3 all so I think all the migrations were actually running correctly. However, when I was querying the database I wasn't getting all the rows since I was using sqlite3#get so I thought not all of them were running. However, I think my rollback --all at the end was initially incorrect too so I fixed that as well.

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

@kibertoad kibertoad merged commit 75df3b6 into tgriesser:master May 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 85.325%
Details

@leeallen337 leeallen337 deleted the leeallen337:feature/add-rollback-all-to-cli branch May 11, 2019

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

@leeallen337 Thank you, looks great!
One thing left - could you please create a PR for https://github.com/knex/documentation to update documentation for CLI rollback command?

@leeallen337 leeallen337 changed the title WIP: Feature/add rollback all to cli Feature/add rollback all to cli May 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.