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

PR for issue 651 #709

Merged
merged 13 commits into from Apr 15, 2014
Merged

PR for issue 651 #709

merged 13 commits into from Apr 15, 2014

Conversation

@dlbunker
Copy link

@dlbunker dlbunker commented Mar 3, 2014

... hooks on the db migrate task. This is an effort to address issue 651 located here:

#651

This task could be updated to include a context that gets passed from listener to listener if there is a need for that. This would allow listeners to communicate with each other.

…ost hooks on the db migrate task. This is an effort to address issue 651 located here:

flyway#651

This task could be updated to include a context that gets passed from listener to listener if there is a need for that.  This would allow listeners to communicate with each other.
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 4, 2014

Thanks for submitting this. This is definitely somthing that I want to be part of 3.0. It is however not exactly the direction I would like to take this.

Things to change:

  • as this is for public sonumption, it should move to the api package
  • name the interface FlywayCallback
  • it should have these methods: beforeClean, afterClean, beforeMigrate, afterMigrate, beforeEachMigration, afterEachMigration, beforeValidate, afterValidate, beforeInit, afterInit, beforeRepait, afterRepair, beforeInfo, afterInfo. Each method should have a single connection argument. The "each" methods should also have a second MigrationInfo argument.
  • add setCallbacks(FlywayCallback... callbacks) and FlywayCallback[] getCallbacks() methods to the Flyway class
  • default to []
  • add the possibility to configure all clients with a new flyway.callbacks property, taking a comma-separated list of fully-qualified lass names to instantiate
  • add tests
  • write docs for flywaydb.org

I know this list seems complex and exhaustive, but adding a central piece of the API must be done correctly and consistently to make it useful and avoid a support burden.

Looking forward to the improvements!
Axel

@axelfontaine axelfontaine added this to the Flyway 3.0 milestone Mar 4, 2014
@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 4, 2014

I'm good moving to a callback adapter. Give me a few days and I'll re-submit. What are your thoughts with making some of the callbacks annotation based in addition to the main Callback Interface?

ie.
@BeforeMigration
public void doSomething(){...}

@AfterMigraiton
public void doSomethingElse(){...}

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 4, 2014

All things combined, I feel annotations would provide no added value. The only thing going for them would be avoiding some empty methods in the implementation of the callback interface for the functionality you don't need. But on the other hand, you loose the compile-time safety of the interface regarding method names and parameters.

So if you ask me it's a clear no: the drawbacks outweigh the benefits.

…terface. Also added lifecycle events for all of the flyway tasks, as well as junit tests for each event.
@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 4, 2014

Hey Axel,
I made the main changes as you outlined and pushed another commit for this PR. I haven't added any documentation to the site yet. If this looks like something you are ok with then I'll update the site docs. Thanks

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 5, 2014

A good step on the way there!

What's left (code):

  • integrate the latest code changes from master. Currently this PR is based on an older commit and conflicts. It therefore can't automatically be merged.
  • change the collection type to an array instead of a list, to be more orthogonal with the rest of the api
  • add the new property to the various clients (maven, ant, gradle, sbt)
  • document the property in the command-line usage docs

For flywaydb.org proper, I will create a new branch for 3.0 which will be our base for integrating these changes.

Keep up the good work!
Axel

Daniel L Bunker added 5 commits Mar 5, 2014
…so fixed flyway-core tests to be compatible with the callbacks array
…dapter for clients not wanting to implement all interface methods. Also added command line callback usage and parameter tests.
@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 5, 2014

I've pushed changes that merge the upstream and enhance the Maven, Ant and Command line tools. I'll see if I can get to Gradle and SBT next (although I'm not very familiar with SBT).

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 5, 2014

Great work!

A couple of things:

  • The default implementation should go. It currently uses private API (Log) and creates a cycle between the util and the api packages. This is the kind of thing that can always easily be added later.
  • The setter should use varargs and the parameter should have javadoc
  • The callbacks are currently only working for parts of the calls. Flyway has a number of cascading calls triggerred by the xOnY properties such as validateOnMigrate. Suggested solution: pass the callback array to the contructor of DbInit, DbMigrate, ... . They should then invoke the callbacks internally whenever required.
  • The callbacks field in Maven should be of type String[] to support element nesting as is typical in Maven POMs. Have a look at how the schemas property does it.

Cheers
Axel

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 5, 2014

Also:

  • the default config file for the command-line tool should be extended
  • the usage for the command line tool should fit within 79 chars

I know, it's a lot of small details to get something like this in consistently across the board...

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 5, 2014

The default impl is useful when testing from outside of core to check for output as well as provides an adapter like pattern for those not wishing to implement all of FlywayCallback methods (Similar to the old WindowAdapter class). If the methods in there need to be empty or use something else, that's fine. Checking the stdout for lifecycle calls makes for easy testing without having to add that as a custom class in each of the modules.

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 5, 2014

I understand why it's convenient, but that doesn't solve the issues I outlined above. I tend to be extremely conservative with was becomes public API, as this is basically stuff you have to support for a long time and it's a major pain to change later.

How about adding this to the flyway-sample module instead? This gets pulled in by all the large test modules and kinda serves the same role of a common test case.

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 5, 2014

I'm good with moving it to the sample module if it can serve the same purpose.

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 6, 2014

I've moved the Default impl to the sample module (nothing checked in yet). It works with the classpath in Maven tests. For some reason it's not finding that class with my ant and command line tests. Any hints on how to get the classpath setup correctly for those modules?

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 6, 2014

Sure.

Flyway Command-line will load all jars files in the directory specified by flyway.jarDir (default: installDir/jars)

The Ant Tasks load classes from either the classpath attribute, the classpathref attribute, ${flyway.classpath} or ${flyway.classpathref}

… accordingly. Also discovered a class loading issue in Flyway core so I changed it to use the ClassUtils which loads classes off of the Thread class loader.
@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 10, 2014

I'll move the callbacks into their respective classes later this week so they always get called. I think most of the other issues have been addressed in the last push. I'm running down some dev fires on some other stuff currently. I haven't forgot about this.

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 11, 2014

Hey,
I've moved the callbacks into their command classes so they should always be called now and not missed based off of cascades. Other than adding the callback properties to SBT and Gradle, what else is missing?

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 24, 2014

Fantastic work! (Sorry for the late reply, I was on holiday.)

Still open:

  • Gradle
  • SBT
  • flywaydb.org docs

Did you have time to look into these?

Cheers
Axel

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Mar 25, 2014

Hey Axel,
I will get these last tasks wrapped up. I'm on vacation as well right now. I will try and get the Gradle and SBT done first and have you look it over. Then if we are good there I'll update the website docs.

Dan

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Mar 25, 2014

No worries Dan. Enjoy the vacation!

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Apr 1, 2014

Hey,
Gradle and SBT support has been pushed. I'm working on the docs next.

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Apr 2, 2014

Axel,
Do you want to add a left menu item in the documentation on the website that covers callbacks or do you want me to just add the properties and configuration to the maven sections?

@ptahchiev
Copy link

@ptahchiev ptahchiev commented Apr 11, 2014

Any updates on this one? I can't wait to test it :)

@axelfontaine axelfontaine merged commit 367d9e7 into flyway:master Apr 15, 2014
1 check passed
@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Apr 15, 2014

Thank you so much for all your work Dan!

I've merged it all. For the docs, don't worry about the API. I want to redesign that section of the site anyway. It would be great if you could add the property and a value example (at the bottom) for the operations of the various clients. You can war that in a PR for the flyway-3.0 branch of flywaydb.org

Cheers
Axel

P.S.: I've added you to the hall of fame page (currently in the 3.0 branch).

@dlbunker
Copy link
Author

@dlbunker dlbunker commented Apr 30, 2014

Hey, I see you've released version 3. I will pull the new website and send you a PR to get this documented. Thanks

@axelfontaine
Copy link
Contributor

@axelfontaine axelfontaine commented Aug 17, 2017

@dlbunker Could you please contact me by email at axel at boxfuse dot com ? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants