Rollback command

[krzykamil]

1. Focused on doing things 1 database per rollback, no "assuming" behavior user would want with multiple databases
2. Enforce specific usage of the command, unless the setup is very basic or very obvious (single app, single database etc.)
3. Use err.puts to communicate what user needs to do (which I based on it being used in lib/hanami/cli/commands/app/db/command.rb)
4. Utilize command_exit more (looked more into the migrate command and based it on that)
5. Hope the messages to the user are clear enough, lemme know WDYT
6. Provided a little comment about calculating target_index

If this is accepted I will provide relevant documentation changes

[kyleplump]

awesome work @krzykamil!! 💪 i left a few comments

more broadly: do you think its worth adding a flag for the number of steps? e.g.: rollback -s 3 or rollback --steps=2 or something similar. i like the simplicity of just passing a number, but i feel the explicitness could both prevent mistypes, help simplify the implementation (since its currently handling the ambiguity), and reduce overall 'magic'

maybe we could keep just passing a single number (like we currently have), as a shortcut. and then the steps flag would be 'in-addition'? i'm curious what you and the rest of the team think about this!

looking forward to seeing where this goes! this is a great feature to have

[krzykamil]

awesome work @krzykamil!! 💪 i left a few comments

more broadly: do you think its worth adding a flag for the number of steps? e.g.: rollback -s 3 or rollback --steps=2 or something similar. i like the simplicity of just passing a number, but i feel the explicitness could both prevent mistypes, help simplify the implementation (since its currently handling the ambiguity), and reduce overall 'magic'

maybe we could keep just passing a single number (like we currently have), as a shortcut. and then the steps flag would be 'in-addition'? i'm curious what you and the rest of the team think about this!

looking forward to seeing where this goes! this is a great feature to have

thanks, I applied everything you caught

I like passing steps as a number by default and having it explicit as -s or --steps both. Basically it is either argument or option in the DSL so not much work to change it

[kyleplump]

great, thanks @krzykamil!

everything else looks good to me, curious what everyone else thinks 🙂

[krzykamil]

great, thanks @krzykamil!

everything else looks good to me, curious what everyone else thinks 🙂

Thanks, thanks

I am also wondering how to do specs in a smart way. Not sure if just copying every spec I have into context for postgres and mysql is the best way, or should I DRY it up (personally not a big fan of RSpec shared_example as it makes debugging nasty sometimes).

If none has better ideas I would just copy the specs for each of the database adapters

[alassek]

@krzykamil

But I kinda did not understood what the app flag is for except if we would want to command to infer the slice from the place the command is run

The default behavior, if --app or --slice are absent, is to execute against all gateways. The --app flag limits the migrator to just the app-level db.

See https://github.com/hanami/cli/blob/fa4aeed23868a8f9dad4e68e59999c09198f391f/lib/hanami/cli/commands/app/db/command.rb#L62-L69

[krzykamil]

@krzykamil

But I kinda did not understood what the app flag is for except if we would want to command to infer the slice from the place the command is run

The default behavior, if --app or --slice are absent, is to execute against all gateways. The --app flag limits the migrator to just the app-level db.

See

https://github.com/hanami/cli/blob/fa4aeed23868a8f9dad4e68e59999c09198f391f/lib/hanami/cli/commands/app/db/command.rb#L62-L69

Okey, thanks, I somehow missed that code, that makes a lot more sense now :) Will have to adapt my code a bit then

[timriley]

@krzykamil Thanks so much for putting in the work on this command. I recognise that it's pretty gnarly, and I appreciate that you're willing to dive in!

Based on your last comment, it sounds like you need to make a couple of adjustments before this is ready for review again?

Before that happens, I wonder if we can simplify your job a little bit.

Thinking about the “which database(s) to use” heuristic that @alassek shared above, I wonder if it even makes sense for a command like db rollback to apply to "all databases" — it feels like that will make it harder for a user to understand. Your “if the last 3 migrations were spread across 2 slices and the app, and X is 3, those 3 will be rolled back” logic is very clever, but when it comes to database migrations, I don't think the user will be keeping the union of all database migrations in their mind when they ask to rollback. Instead, I think they'll probably be thinking about a single database at a time, since that is much easier to reason about, given you can look at a single db/migrate/ dir and understand the entire state.

So: what if we made db rollback so that it works only for a single database at a time?

If you have an app with just one database, then you can do db rollback and it'll automatically operate on the app's single database, no worries. But if you have an app with multiple databases, then running db rollback on its own will print a message asking you to provide --app or --slice. This way, the user can be confident at all times which migrations are being reversed.

(I think we could apply the same “single database only” logic to the db migrate command when the -t option is being given).

If we took this approach, I think this would make the db rollback implementation much simpler, and still give our users a command that is convenient and easy to understand. My feeling is that no one will gripe about rollback working on a single database at a time. (And if they do, that's useful feedback for the future).

What do you reckon?

[krzykamil]

What do you reckon?

You make valid points ofc, I did work out some scenarios without thinking how useful they are (was thinking too much about if I could, rather than if I should :d .
Rolling back more than 1 database at a time seems very edge-casey/uncommon and counter-intuitive. Keeping in mind consequences of 1 rollback is enough, doubt someone will bother doing that in bigger scale

I will refocus on this PR now with those things in mind:

1. --app option means "run this only for main app context" (this will be the new default behavior if there are no multiple databases)
   a. If there are multiple databases (across app or slices or gateways) and neither app or slice is provided, user will be prompted for one or the other
   b. If there are multiple gateways in a given context (app has multiple gateways or slice), user will be prompted to specify which one is to be chosen
2. Only allow the command to work on one database at a time, for both user and implementation simplicity

[krzykamil]

This has been updated, I am still working on making specs for other databases but the new approach should be clearly visible now

[timriley]

@krzykamil This is looking beautiful! Definitely the right direction. The command class is much clearer and easier to follow. Thank you! Left a couple of very minor thoughts for you while I was here. Let me know again when this is ready for merge! ❤️

[timriley]

Thanks for doing this one. I like that we can keep hanami db rollback simple to call for Hanami apps that have just one database.

[timriley]

These feel like they could do with some comments explaining what's happening. It's a lot of logic in two lines!

[timriley]

Explaining this here is very helpful! Thank you!

[krzykamil]

@timriley did some cleanup, added comment about specs, I think its ready

[timriley]

@krzykamil Beautiful! We made it! What a journey this has been, thank you so much for your persistence 🥰

I'll go ahead and merge this now!