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

Refactored code to have a Provider. #360

Closed
wants to merge 3 commits into from
Closed

Conversation

gdey
Copy link

@gdey gdey commented Jun 9, 2022

Refactored code to have a Provider.

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 `defaultProvider`.

See:
* https://github.com/pressly/goose/pull/351
* https://github.com/pressly/goose/issues/114
* https://github.com/pressly/goose/issues/114#issuecomment-950338785

@gdey gdey force-pushed the goose-provider branch 4 times, most recently from 155f0ce to 94c029e Compare June 9, 2022 06:18
@mfridman
Copy link
Collaborator

mfridman commented Jun 9, 2022

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 /v4 version of this project. This will be a breaking change on a new major version that will address many of the issues folks have requested in the past.

Copy link
Collaborator

@mfridman mfridman left a comment

Choose a reason for hiding this comment

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

See comment above.

@gdey
Copy link
Author

gdey commented Jun 9, 2022

I don't think it's necessary to go to a v4 just to add a Provider and get the features requested. I did this as I needed the following:

  1. Have multiple migrations in the same bin (with different table_names in the same db), that don't step on each other.
  2. Have the ability to have UTC timestamps for versioning.
  3. Change the formating for sequential numbers

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.

#114
#351
#242

@zmitry
Copy link

zmitry commented Jun 16, 2022

wow, excited to have this merged

@DBarney
Copy link

DBarney commented Jun 24, 2022

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.
@gdey
Copy link
Author

gdey commented Jun 25, 2022

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)

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 defaultProvider, basically the package-level API, then the only way to change the TZ is to change it via the ENV variables. This PR is fully backward compatible. It does not break behavior at all.

gdey added 2 commits June 25, 2022 11:49
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:
* pressly#351
* pressly#114
* pressly#114 (comment)
Added and example that make use of Multiple providers.
@mfridman
Copy link
Collaborator

Closing PR. There is ongoing work in #507 to refactor goose in a way that supports evolving the project.

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 /v4.

We'll continue to maintain /v3 of this project, but adding larger features is unlikely. Hope you understand where we're coming from :)

@mfridman mfridman closed this Jun 29, 2023
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.

None yet

4 participants