-
Notifications
You must be signed in to change notification settings - Fork 545
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
Refactored code to have a Provider. #360
Conversation
gdey
commented
Jun 9, 2022
•
edited
Loading
edited
Hi, thanks for submitting a PR. But we're unlikely to accept this. This sort of change should have an issue and a discussion around the approach given how large of a change this is. We'll be adding the concept of a Provider, but in a |
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.
See comment above.
I don't think it's necessary to go to a
This is what is provided with this PR. As you can see with the added example (e4fcd5b). Worse case, you can use this as starting point for discussion. I needed this functionally and looking through the issues and PR I noticed it was common enough that I thought I share. |
wow, excited to have this merged |
I am not a maintainer for this project, however I agree that a breaking change should hold off for a major version bump, and not just sneak into a project somewhere. So I don’t object to the default TZ staying as is for now. Especially since you can set the TZ: #242 (comment) However, I don’t see any breaking changes with just making the currently global functions point to a provider. @mfridman seeing how there is discussion around having a “Provider” already in at least one issue, #114, and how it has been mentioned that it would a good thing to do back in 2019 #114 (comment), If this PR was broken up into its different components, would the new Provider code be a candidate for release into a new minor version as it wouldn’t introduce any breaking changes? Especially since it does not remove the globals, but just implements them as a defaultProvider. I ask because I am trying to use goose programmatically on multiple databases that are created dynamically, and having to implement locking around all the globals that Goose provides is a little cumbersome. |
The system is really not made to run in parallel. Lot's of global variables that track state of running migration, with no locks. This was causing test to run inconsistently on my machine.
This does not change the default TZ at all, there are no breaking changes here. You are now just given the option to adjust the TZ if you set up a provider yourself. If you use the |
All migration code is now in a Provider that provides migrations functionality. This helps isolate various parts of the migrations and prevents multiple migrations from stepping on top of each other. This refactor is completely backward-compatible and does not break the current Public API. This is accomplished by aliasing the Public functions to a `defaultProvider` that is initialized to the default settings as it is currently. Those who don't wish to use the new functionality can do so without any changes. To use the new system, one will need to Initialize a new Provider via the `goose.NewProvider()` method which will return a provider that will be initialized to the default settings, unless otherwise overwritten. Note: No new tests were added, and the tests were purposely left* alone so to ensure that the API did not break. * except for runMigrationSQL as that is a private API. Since behavior did not change, this means all the race conditions that existed before still exists, just now isolated to the `defailtProvider`. See: * #351 * #114 * #114 (comment)
Added and example that make use of Multiple providers.
Closing PR. There is ongoing work in #507 to refactor Thank you for the contribution, but in order to maintain the project effectively we'll need to make a few breaking changes and move certain bits of goose around. We've learned a lot over the years and believe this is the best way forward. In the coming weeks, I'll be posting a recap of the changes (and new features) coming in We'll continue to maintain |