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

add ability to specify rollback limit/amount and the target version to migrate to #256

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TECHNOFAB11
Copy link

@TECHNOFAB11 TECHNOFAB11 commented Jan 7, 2022

Used this evening to quickly implement #18

This now allows the following:

  • dbmate migrate <version> or dbmate up <version> → migrates until <version> is reached
  • dbmate rollback <version> → rollbacks until <version> is reached (<version> is NOT rolled back)
  • dbmate rollback -limit <int> → limits how many migrations are rolled back

Feel free to edit :)

@Enrico204
Copy link
Contributor

LGTM. Shall we add some tests? It would be nice (and safe!), but I don't have ideas for now

@TECHNOFAB11
Copy link
Author

TECHNOFAB11 commented Jan 27, 2022

Definetely a good idea, but I currently don't have any ideas either. Tried to make sure the existing tests don't break, at least that seemed to have worked :D

@amacneil
Copy link
Owner

Couple thoughts:

  • Definitely will need tests before we consider merging
  • How would we handle if someone wants to apply or rollback a specific migration? E.g. if I have migrations A,B,C and want to rollback B only. Ideally it's obvious whether you're triggering "rollback to this version" versus "roll back this specific migration".

@Enrico204
Copy link
Contributor

  • How would we handle if someone wants to apply or rollback a specific migration? E.g. if I have migrations A,B,C and want to rollback B only. Ideally it's obvious whether you're triggering "rollback to this version" versus "roll back this specific migration".

Uhm, I don't expect to rollback a specific migration in a normal workflow (except when it's the last applied). Given the fact that migrations are written to be executed in a given sequence, I don't think that you can safely assume that you can rollback only B without rolling back C.

The way I see this feature is the following: consider deploying to multiple environments (test and prod), you may want to apply some migrations in production (say A and B) while you're testing newer migrations in test env (say C).

In other words: in my opinion, rolling back a specific migration in a series (skipping newer migrations) is hacky and not safe. If ever happen, you may want to look at the rollback code and execute it manually.

@pawndev
Copy link

pawndev commented Jun 9, 2022

Hi ! Is this the missing tests that block this PR ?
I can add some to the @TECHNOFAB11 branch if it's ok for you

@pawndev
Copy link

pawndev commented Jun 13, 2022

Just added and edit some basic test here: TECHNOFAB11#1
If you have some feedback, it would be a pleasure !

@TECHNOFAB11
Copy link
Author

Oh wow, never gotten any notification for all this and I don't use Github actively so I didn't see it, I'm so sorry :D
@pawndev Thanks a bunch for the tests and other additions, they lgtm, will merge them into my main and then this PR can be reconsidered :)

test: add some to test up-to/down-to specific migrations
@Sajfer
Copy link

Sajfer commented Mar 21, 2023

Is there anything needed before this can be merged?
This feature would really improve our workflow.

@gregwebs
Copy link

I opened up a PR #434 that allows specifying a single migration to use. The PR is small and easy to understand and works across all commands (such as status).

It works differently though: it ignores all other migrations and assumes you are handling dependency management.
There are other migration tools like sqitch that actually track dependencies.

So both of these could be merged in without issue, but it might be confusing for the end user.

@gregwebs
Copy link

I opened #438 which allows a syntax of dbmate -m VERSION... status/rollback

@gregwebs
Copy link

Separately we still need a way to rollback/down more than just one item.

@amacneil
Copy link
Owner

I'm kinda confused about the expected behavior from this feature. I wrote up some thoughts here.

@TECHNOFAB11
Copy link
Author

I'm kinda confused about the expected behavior from this feature. I wrote up some thoughts here.

My main use care are the first two points in the OP. I use dbmate to migrate DBs in an init container in Kubernetes and when rolling back the app version it migrates up to the version and then down to the version to make sure the right schema exists.
Maybe that makes it a bit easier to decide if that's something you want in dbmate or rather not :)
I'm currently using my fork directly anyway, this would make it easier for me to update for new features and fixes though.

@dossy
Copy link
Collaborator

dossy commented Nov 14, 2023

Unfortunately, merging #441 has introduced merge conflicts for this PR. Sorry, @TECHNOFAB11.

I did a partial review of the code and there's something about it that bothers me but I can't clearly articulate it so I'm going to hold back commenting on the code, but there's one thing that I would like to share:

How do you all feel about positional arguments for subcommands like dbmate migrate <version> because of where command options need to appear in the command, dbmate migrate [command options] [arguments...]?

How do people feel about specifying the version with a command option --until like dbmate migrate --until <version>, instead?

@gregwebs
Copy link

This PR is problematic for me because it it doesn't do anything to show what is going on ahead of time. Sure, you can calculate it, but that's what computers are for.

In my PR #438 I solved this by making a filter flag rather than a bunch of commands. That way the exact same filter flag can be used with the status command. The alternative approach with this command-heavy PR would be to add a --dry-run flag to turn existing commands. There's some advantage over re-purposing status if you wanted to explicitly state that a migration would be rolled back, etc rather than inferring that from the command plus a checkbox.

@dossy
Copy link
Collaborator

dossy commented Nov 15, 2023

The alternative approach with this command-heavy PR would be to add a --dry-run flag to turn existing commands.

I'm actually a big fan of the --dry-run subcommand option for any destructive operation. I would not think to use the status subcommand, but many command-line tools have conditioned us to look for --dry-run as a non-destructive way to preview what changes would occur.

@gregwebs
Copy link

The way these commands are designed is problematic for me though. Take this migration scenario:

  • A
  • B
  • C
  • E
dbmate migrate <version>: → migrates until <version> is reached

If I want to migrate just version C there is no way to do this.

dbmate rollback <version> → rollbacks until <version> is reached (<version> is NOT rolled back)
dbmate rollback -limit <int>

What if I want to rollback just migration A? And already the fact that the version specified is not actually rolled back is confusing.

In #438 I introduce a syntax to specify a specific migration or a migration range. I would rather to that here than have more commands that imply an upward or downward range that then have to be modified to not use that range with --limit, etc. There's a possibility of using various flags like --from and --to instead of a syntax.

@TECHNOFAB11
Copy link
Author

Just a quick info, I think Greg's implementation is much better for general use. I use my implementation because of a specific use case, it's never meant to be used so generally like in the examples in the comments.
For me it works but I think this PR should not be merged and instead focus should be set on Greg's PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants