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 code lens for running migrations #239

Merged
merged 1 commit into from
Jul 2, 2024
Merged

Add code lens for running migrations #239

merged 1 commit into from
Jul 2, 2024

Conversation

gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Feb 1, 2024

Closes #51

Adds code lens to run migrations to specific versions in the terminal. This is convenient because it allows developers to quickly rollback or fast-forward to specific schema versions with a click.

Screenshot 2024-01-31 at 9 09 05 PM Screenshot 2024-01-31 at 9 10 45 PM

@gmcgibbon gmcgibbon requested a review from a team as a code owner February 1, 2024 03:11
@gmcgibbon gmcgibbon requested review from andyw8 and Morriar February 1, 2024 03:11
@gmcgibbon gmcgibbon force-pushed the run_migration branch 9 times, most recently from 096cb2b to 390d6d5 Compare February 3, 2024 04:16
@vinistock
Copy link
Member

This is great! Thanks for contributing it.

Since the LSP specification doesn't allow us to add more details to a code lens (all we have is the button label), I'm trying to think about how we can communicate the action that will occur as clearly as possible.

For example, I can see someone clicking on the Run in terminal button in an old migration and being confused as to why it didn't run their latest migrations (just bin/rails db:migrate).

I have a couple of questions so that we can converge on the DX:

  • Is there a difference between migrating using the latest migration version and simply running db:migrate to run all of them? I mean the end result of doing each action
  • Is there a different between migrating to a specific version vs rolling back to that version? For example, is migrating to the second to last version exactly the same as db:rollback?

What I'm trying to figure out is:

  • Do we need to change the button to Migrate to this version instead of Run in terminal?
  • Do we need to add Run all migrations and Rollback latest migration buttons?

Basically, the addition of the extra code lens would assist us in communicating to the developer that there's a difference between running all migrations and migrating to a specific version, which should help prevent confusion.

@andyw8
Copy link
Contributor

andyw8 commented Feb 20, 2024

Is there a different between migrating to a specific version vs rolling back to that version? For example, is migrating to the second to last version exactly the same as db:rollback?

I noticed this in the docs:

Neither of these rails commands do anything you could not do with db:migrate. They are there for convenience, since you do not need to explicitly specify the version to migrate to.

@vinistock
Copy link
Member

Thanks for sharing. Do people agree with using 3 code lenses?

Run all migrations | Migrate to this version | Rollback latest migration

I feel like usually people want to run bin/rails db:migrate and bin/rails db:rollback. Having those there will be handy, but it will also help communicate that Migrate to this version may actually be rolling back their database.

@gmcgibbon
Copy link
Member Author

Seems like my PRs are getting stale. I'll see if I can find some time to rebase and stuff the logic into different listeners.

@gmcgibbon
Copy link
Member Author

gmcgibbon commented Mar 9, 2024

Do people agree with using 3 code lenses?

Personally I would be supportive of this order: Migrate to this version | Run all migrations | Rollback latest migration. Idiomatically, if we're putting these lenses on all migrations, the developer would open the migration they want to go to. I would go as far as to say the other lenses should only be visible on the latest migration.

Is there a difference between migrating using the latest migration version and simply running db:migrate to run all of them? I mean the end result of doing each action

Andy is correct, VERSION=<latest> and no args to run all are the same.

Is there a different between migrating to a specific version vs rolling back to that version? For example, is migrating to the second to last version exactly the same as db:rollback?

Yes, I think so. Rollbacks can specify a step count though, so that's slightly different. The default step is one, but you can do something like bin/rails db:rollback STEP=3.

It is just really convenient to use the VERSION for my use-case because Rails can figure out if I mean I want to go up or down, and by how many steps.

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we go with a single code lens, but that says Migrate to this version (2 versions behind latest)?

I believe that would provide the full context and avoid needing multiple buttons.

lib/ruby_lsp/ruby_lsp_rails/code_lens.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/code_lens.rb Outdated Show resolved Hide resolved
@vinistock
Copy link
Member

On second thought, I think we may be over optimizing this too early. Figuring out how many migrations behind the current one is from the latest will require listing files and consume time in IO.

How about we ship this with only Migrate to this version and gather feedback?

@gmcgibbon gmcgibbon requested a review from a team as a code owner June 27, 2024 20:49
@gmcgibbon gmcgibbon requested a review from st0012 June 27, 2024 20:49
@gmcgibbon gmcgibbon force-pushed the run_migration branch 6 times, most recently from 8b006a9 to 83a5a86 Compare June 28, 2024 01:55
Adds code lens to run migrations to specific versions in the terminal.
This is convenient because it allows developers to quickly rollback or
fast-forward to specific schema versions with a click.
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@vinistock vinistock merged commit 5ee21de into main Jul 2, 2024
40 checks passed
@vinistock vinistock deleted the run_migration branch July 2, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add code lens to run or rollback migration
3 participants