Add migrate options: --one, --all, --to, --only#632
Conversation
c1cc449 to
eaaecec
Compare
eaaecec to
d1bde2f
Compare
|
@dossy please review |
|
Code and tests look fair, although questionable whether it's better to parameterize the migrate function vs. duplicating it with minor changes, and tests are passing, which is good. However, I won't be approving this PR because I recognize that the decision to accept or reject this change needs to be made by someone who is better qualified to decide if this is a good change or not. |
Okey, who can make a decision about this feature? I agree that the option of the migrate command instead migrate-next would be better. @amacneil Hi! What do you think about that? |
|
There is lots of prior discussion about this:
Including some thoughts I posted in #438 (comment) I would like to see a proposal that solves all (or most) of these use cases:
I think we can do this within the scope of the current For example (open for comments, I think these cover all the needs people have expressed in the past): # these flags would be supported for `dbmate up` and `dbmate down` too
dbmate migrate # existing: apply all pending migrations
dbmate rollback # existing: rollback one version
dbmate migrate --only 0003 # apply only this version
dbmate rollback --only 0003 # rollback only this version
dbmate migrate --to 0003 # migrate up to this version (inclusive)
dbmate rollback --to 0003 # roll back to this version (exclusive, i.e. the specified version will become the current applied version)
dbmate migrate --one # apply one pending migration
dbmate rollback --one # this is the default, maybe included for consistency?
dbmate rollback --all # special case to rollback all migrations
dbmate migrate --all # this is the default, maybe included for consistency? |
So good, I'll implement this in current PR this week. |
MigrateNext option--one, --all, --to
8529880 to
15283a0
Compare
af5d13c to
a47c298
Compare
a47c298 to
f5fb235
Compare
|
epic! let us know when ready for a review |
061647a to
b9aeb2a
Compare
b847b0e to
3a60709
Compare
--one, --all, --to--one, --all, --to, --only
405dfa6 to
ef2ca13
Compare
There was a problem hiding this comment.
Hi, first of all thank you very much for the PR! These features are really missing IMO and are making dbmate a perfect tool for our use-cases.
In order to make my contribution to the project, I had a closer look on the code changes and I have two comments/questions. Hope this can bring the PR a bit closer to get merged in the future!
| if m.Applied || m.Version > target { | ||
| continue | ||
| } | ||
| if err := db.MigrateOnly(migrations, m.Version); err != nil { |
There was a problem hiding this comment.
Compared to the RollbackTo func, the MigrateTo func applies changes even if the intended target does not exists. I would suggest to align the behaviour here and fail with an error msg before any changes are applied if the defined target does not exist.
There was a problem hiding this comment.
Correct! Thanks for feedback. I fixed this case: added step that checks if target exists in window to apply.
| ) | ||
| } | ||
|
|
||
| sqlDB, err := db.openDatabaseForMigration(drv) |
There was a problem hiding this comment.
Any specific reason, why you are reimplementing the "apply migrations steps" (as far as I can see, this is just duplicated code) and not reusing the MigrateOnly function?
Same question of course applies to the Rollback* equivalent functions of course.
89a3ff6 to
d698b76
Compare
|
@tjarbo Ready for re-review =) |
d698b76 to
35ba1c4
Compare
35ba1c4 to
1964c9e
Compare
tjarbo
left a comment
There was a problem hiding this comment.
The code looks good to me. Thanks a lot!
A disclaimer for the the maintainers: I have not tested the code functionally speaking.
I just have another two small comments, but they are not worth to block the PR. I leave it up to you whether to implement them or not.
|
Hi @amacneil Best regards |
|
Thanks for your work - really looking forward to this! It would help immensely in our CI pipelines / tests. Any news on when this will make it in? 🚀 |
Call for @amacneil =) I will resolve conflict if this pr will be merged after that. |
Implements #631 according to #633