-
Notifications
You must be signed in to change notification settings - Fork 334
Add migrate options: --one, --all, --to, --only
#632
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct! Thanks for feedback. I fixed this case: added step that checks if target exists in window to apply.
pkg/dbmate/db.go
Outdated
| ) | ||
| } | ||
|
|
||
| sqlDB, err := db.openDatabaseForMigration(drv) |
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.
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.
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.
No reason :)
Fixed!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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