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

Implementation for #805: JdbcMigrations, Resolvers and FlywayCallbacks should have access to core configuration #811

Merged
merged 1 commit into from Jan 22, 2016

Conversation

@pauxus
Copy link
Contributor

@pauxus pauxus commented Jul 28, 2014

Introduced a generic ConfigurationAware interface.

If a custom resolver or callback implements ConfigurationAware, the FlywayConfiguration get automatically injected. That way, resolvers and callbacks have access to important configurations like the locations (for custom resolvers) or PlaceholderResolvers.

Right now, Flyway implements FlywayConfiguration directly.

@axelfontaine axelfontaine added this to the Flyway 4.0 milestone Nov 25, 2014
@collinpeters
Copy link

@collinpeters collinpeters commented Jun 16, 2015

Is there a status update for this issue? Really need access to the config from within a JdbcMigration myself.

@pauxus pauxus force-pushed the pauxus:better-callbacks branch from 37f6fba to 5f15ffd Jan 13, 2016
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 13, 2016

I provided an adapted implementation. Once again: This implementation is backward compatible, because injection of config data is done via an optional interface.

@pauxus pauxus changed the title Implementation for #805: JdbcMigrations and new FlywayCallback should have access to core configuration Implementation for #805: JdbcMigrations, Resolvers and FlywayCallbacks should have access to core configuration Jan 15, 2016
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 15, 2016

Resolved conflicts. However, currently tests fail that also fail in master, too.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 15, 2016

This patch in its current form does way too much. At this point I certainly don't feel like making DbSupport part of the public API. Could you isolate ONLY the FlywayConfiguration/ConfigurationAware part?

Thanks,
Axel

@pauxus pauxus force-pushed the pauxus:better-callbacks branch from 1235571 to db03e51 Jan 17, 2016
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 17, 2016

Agreed, moving DbSupport was too much. However, the other two changes work hand in hand and do not make much sense without each other.

@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 17, 2016

Fixed conflicts. Unfortunately, your current changes are all in the code changed by this PR, so this results in a lot of work in keeping it current.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 18, 2016

This still does way too much. PlaceholderReplacer and Scanner are now exposed in a public package, there is a new skipDefaultResolvers property, etc.

Please isolate ONLY the FlywayConfiguration/ConfigurationAware part. Also please include the JavaDocs in FlywayConfiguration.

@pauxus pauxus force-pushed the pauxus:better-callbacks branch from ab9d0ee to ee219e8 Jan 18, 2016
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 18, 2016

Ok, I worked around exposing PlaceholderReplacer and Scanner (the way it was in the original PR). I also included JavaDoc as requested. However, there is still one internal api left: DbSupport. In order to replace built in Resolvers, the CustomResolver needs access to the DbSupport Object.

In that case, I can not do a workaround, because of the state of DbSupport. I could (and acutally did in a previous step) refactor the internal part (DbSupport) into a separate, use-at-your-own-risk interface (DbSupportAware?).

What do you think? (waiting on your feedback before merging again)

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 18, 2016

Please move the CustomResolver support to a separate pull request. I would like to keep this one to literally just the FlywayConfiguration/ConfigurationAware part. It's big enough already with just that.

@pauxus pauxus force-pushed the pauxus:better-callbacks branch from ee219e8 to bb19e0c Jan 18, 2016
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 18, 2016

Ok, I removed the custom resolver part.

When you give me the go, I would start to merge to resolve the conflicts.

I will create a new PR with the custom resolver part.

@pauxus pauxus force-pushed the pauxus:better-callbacks branch from bb19e0c to f559d4f Jan 19, 2016
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 19, 2016

Squashed and rebased.

@pauxus pauxus force-pushed the pauxus:better-callbacks branch 2 times, most recently from a366ca6 to ddf6889 Jan 19, 2016
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 19, 2016

Getting closer, but still a little more trimming required

Could you:

  • remove the original javadoc from the getters (that are now in FlywayConfiguration) in the Flyway class
  • remove the Flyway.injectConfiguration method and its usages as well as the changes to setXYZAsClassNames
  • remove the changes to the creation of Scanner objects as they effectively undo the effects of the newly added caching
  • document the ConfigurationAware.setFlywayConfiguration method and its argument
  • remove the imports of internal classes from FlywayConfiguration
  • remove the changes to the existing migration resolvers
  • remove injectionutils
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 19, 2016

Javadoc and imports: sure

As for the other changes:

  • Flyway.injectConfiguration(): This is an important part of the PR. We need a place to perform that automatic injection. In the current version, the most feasible place is the setters. In the second part of the original PR (the replacement of the built-in resolvers), I moved that part into CompositeMigration Resolver - which might be an option, Injection into Callbacks could be done somewhere else, but this would be scattered throughout the code. Do you agree?
  • Scanner Object: yes, this removes caching again (thus the earlier approach in providing Scanner via FlywayConfiguration). If I move the injection into CompositeMigrationResolver, it would allow the caching again.
  • Changes to existing Resolvers: I could roll back the constructor changes (still, almost every information needed by the resolvers is part of the FlywayConfiguration, so replacing the constructor arguments seems a logical step). However, the injection steps in both jdbc resolvers are essential. My initial reason for creating this PR (1,5 years ago) was to create a useful GroovyResolver that needed access to placeholder information.
  • Why remove InjectionUtils? It needly incapsulates the injection part, which is needed in at least three places (jdbcResolvers, callbacks, and compositeResolver).

(sent you an email via your website, might be easier to discuss this via phone)

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 21, 2016

InjectionUtils is mileading as its name is generic, yet it only works for FlywayConfiguration. Simply add a private method to the Flyway class.

Flyway.injectConfiguration() is something that can be done right before command.execute(...) in Flyway.execute(Command)

@pauxus pauxus force-pushed the pauxus:better-callbacks branch from ddf6889 to f05875c Jan 21, 2016
@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 21, 2016

Actually, that is what I did. Which works for the callbacks. Resolvers can be injected in the CompositeResolver (which I did). Thats already the second place for injection. Third place is the injection into custom migrations (jdbc/spring jdbc). So it makes sense to put it into a separate helper class (I am open for name suggestions).

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 21, 2016

OK, I feel we're getting there.

Can you remove that new sqlline dependency?

@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 21, 2016

Sure, sorry, that slipped into it because 1.1.8 can not be resolved from central.

…e configuration

Introduced a generic ConfigurationAware interface.

This allows to inject Core configuration into resolvers, migrations and callbacks. Configuration is injected by using a new FlywayConfiguration interface, which provides read only access to flyway configuration.

Right now, Flyway implements FlywayConfiguration directly.
@pauxus pauxus force-pushed the pauxus:better-callbacks branch from f05875c to 9d437f6 Jan 21, 2016
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 21, 2016

So? No new dependencies please.

@pauxus
Copy link
Contributor Author

@pauxus pauxus commented Jan 21, 2016

I did not introduce a new dependency... It was only an entry in dependencyManagement... phoenix has a dependency on sqlline 1.1.8 which I did replace with 1.1.9, to be able to build it in my corporate environment. It was not meant to be included in the pull request and I did remove it already. (still, might make things easier for developers to put it back in - apart from this PR)

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Jan 21, 2016

OK. Thanks for clearing things up.

axelfontaine added a commit that referenced this pull request Jan 22, 2016
Implementation for #805: JdbcMigrations, Resolvers and FlywayCallbacks should have access to core configuration
@axelfontaine axelfontaine merged commit 55d6346 into flyway:master Jan 22, 2016
@pauxus pauxus deleted the pauxus:better-callbacks branch Jan 26, 2016
@pauxus pauxus mentioned this pull request Mar 23, 2017
5 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants