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 -dry-run flag #479

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

elijahcarrel
Copy link

@elijahcarrel elijahcarrel commented Mar 13, 2023

Adds a -dry-run flag which can be used on up, up-to, up-by-one, down, and down-to commands which performs all of the same print statements and error checking, aside from the ones performed as part of actually executing the up or down migrations respectively.

This adds the following utility:

  • Can be used in CI/CD pipelines.
  • Developers running migrations against a production database can vet a migration command they are about to run before executing it (ensures they have gotten their flags correct, etc).

This change is fully backwards-compatible both for go clients as well as command-line clients, as it simply adds a new command-line flag and adds a new go option which defaults to false (as one would expect).

To validate this change, nearly the entire e2e test suite was replicated into a new file dry_run_test.go and tests were modified to perform appropriate dry runs before performing the command for real. In each case, the test validates that the dry run produced the same errors (or lack of errors) as the non-dry run command, but that it does not have any effect on the database. If the maintainers so desire, I could instead port these test cases back into the existing test suite (which would have the benefit of less copied test code, but the downside of making each test longer and more complex).

Fixes #281

@elijahcarrel
Copy link
Author

Hey @mfridman! Thanks so much for all your work keeping this repository maintained. I know you're in the middle of several other refactors and sorry to bother, but just wanted to see if you might be willing to take a look at this at some point. Thanks in advance!

@mfridman
Copy link
Collaborator

mfridman commented Apr 3, 2023

Thanks for contributing a PR, I'll take a closer look by the end of the week.

I presume the goose validate command isn't enough, because in addition to parsing you also want to know what migrations would be applied based on the current database state?

@elijahcarrel
Copy link
Author

That's exactly it, yes!

@elijahcarrel
Copy link
Author

Hey @mfridman, just wanted to bump this once more if you're willing to review. Thanks so much!

@mfridman
Copy link
Collaborator

mfridman commented May 9, 2023

Sorry @elijahcarrel, I've been holding off on reviewing this PR because I'm a bit hesitant about adding more functionality to the /v3 module of this package, with the exception of context support, dependency upgrades and reasonable bug fixes.

My challenge is finding the right balance of adding new features in a way that makes sense with the existing goose API. This project is 10+ yo with many folks contributing, and new Go best practices have evolved over time, and it has been a bit challenging adding new features while compromising ergonomics. The functional options were a means to an end, but I'm not sure how sustainable it is.

Most of my efforts have been concentrated on the /v4 module because it opens the doors to A LOT of interesting functionality.

Having said that, I'll try to find some time to revisit this issue. But at least wanted to give you an update on the delay.

@elijahcarrel
Copy link
Author

Makes total sense, thanks so much for your consideration! I'm open to holding off until v4 as it's not urgent (as long as the timeline for v4 isn't years out).

As additional context, the motivation for me is that my company made our own fork years ago (https://github.com/grailbio-external/goose), made several changes, and ceased to keep it up to date with this one, and it's now woefully out of sync . -dry-run is the only important feature present in our fork that isn't already present in this repo, so the motivation here was to rebuild the feature in the upstream/modern goose so that my company can transition back to this upstream goose without having to drop this feature (or make our own fork again, and I don't have confidence we'll do any better at staying up to date this time around…).

@mfridman
Copy link
Collaborator

Gotcha, thanks for that explanation. This makes total sense, and it's always appreciated when folks take the time to contribute back upstream.

I need a bit of time to think this through, but I think this is reasonable. Given this touches quite a bit of the internals I'll spend some time reviewing it carefully.

Bonus, this also partially addresses something I've been thinking about previously.

#281

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.

feature: add dry-run option
2 participants