add rollback_to feature #133
Conversation
| @@ -26,7 +29,7 @@ if [ $# -eq 0 ] | |||
| then | |||
| printf "\nChecking that tasks build correctly\n\n" | |||
| $COMPOSE shards build | |||
|
|
|||
| printf "\nChecking code formatting\n\n" | |||
There was a problem hiding this comment.
Whoops, didn't notice this whitespace change. I guess my editor automatically kills whitespace at the end of lines. I can revert this if desired.
There was a problem hiding this comment.
No this is great! I'm glad your editor caught it :D
| @@ -26,7 +29,7 @@ if [ $# -eq 0 ] | |||
| then | |||
| printf "\nChecking that tasks build correctly\n\n" | |||
| $COMPOSE shards build | |||
|
|
|||
| printf "\nChecking code formatting\n\n" | |||
There was a problem hiding this comment.
No this is great! I'm glad your editor caught it :D
| @@ -14,6 +14,9 @@ then | |||
| $COMPOSE lucky db.drop | |||
| $COMPOSE lucky db.create | |||
| $COMPOSE lucky db.migrate | |||
| printf "\nRolling back to 20180802180356\n" | |||
| $COMPOSE lucky db.rollback_to 20180802180356 | |||
| printf "\nRolling back remainder\n" | |||
| if version && version.to_i64? | ||
| Avram::Migrator::Runner.new.rollback_to(version.to_i64) | ||
| else | ||
| raise "Migration version is required. Example: lucky db.rollback_to 20180802180356".colorize(:red).to_s |
| subset = migrated_migrations.select do |mm| | ||
| mm.new.version.to_i64 > last_version | ||
| end | ||
| subset.reverse.each &.new.down |
There was a problem hiding this comment.
What do you think about printing a message after this like "Done rolling back to #{last_ version}"?
src/avram/tasks/db/rollback_to.cr
Outdated
| class Db::RollbackTo < LuckyCli::Task | ||
| summary "Rollback to a specific migration" | ||
|
|
||
| def call(version : String? = nil) |
There was a problem hiding this comment.
It looks like this argument is not being used so I think it can be removed
There was a problem hiding this comment.
I copied the pattern from gen.migration. I was assuming there was some reason to provide both a method argument and from ARGV in general. I'll remove it.
There was a problem hiding this comment.
Probably left on accident :S I think we can use something like this to test with in the future though. I need to go through and cleanup specs and do some refactoring at some point :)
There was a problem hiding this comment.
So should I leave it in in case of future test or leave it out for now? I'm happy both ways :)
There was a problem hiding this comment.
Sorry I wasn't clear there at all 🤣 you can go ahead and remove it for now. I'll add it back in when I spruce up the tests
|
Travis is having an incident so I ran this locally and specs pass. Merging now. Thanks @KCErb :D |
|
🎉 sweet |
Wow, that was easier than I thought!
But I don't understand the specs well enough to be sure that these tasks don't have specs. Is there somewhere I'm missing that a task spec belongs?
Also you'll notice that I changed the messaging a bit in the test script. Since I put this before
rollback_allI wanted to make sure I could visually compare (assuming that's our test method) what the message says we are rolling back to vs what actually got rolled back.If this goes in, I'll be happy to update the docs (over on the lucky webpage right?). The doc should say something like "rollback_to " and make it clear that we are not rolling back the migration named in the time stamp, we are rolling back to the state that that migration provided.