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

Gradle plugin #13

Merged
merged 1 commit into from Jul 9, 2013
Merged

Gradle plugin #13

merged 1 commit into from Jul 9, 2013

Conversation

ben-manes
Copy link
Contributor

A gradle plugin based on the Flyway 2.x series (original).

This plugin is integrated into the Maven build using the gradle-maven-plugin to invoke the gradle build. The gradle wrapper is invoked so that the maven packaging phase has the compile jars in the target folder. This allows maven to pick up the jars in subsequent phases, like install.

@axelfontaine
Copy link
Contributor

Could you have a look at the build helper plugin to see if it works with the generated artifacts? -> http://mojo.codehaus.org/build-helper-maven-plugin/attach-artifact-mojo.html

Also, Java migrations WILL need to be supported. In the case of a project with JOOQ, it should then be split into two modules:

  • Module1: Compile Java Migrations, Copy SQL migrations and migrate DB
  • Module2: Generate JOOQ artifacts and do the rest

@ben-manes
Copy link
Contributor Author

Yes, I expected the build-helper-plugin would be needed. I haven't setup all of the dependencies for the maven build from the contrib page yet. Its been long enough that I forget if Maven's dry-run will let me inspect what artifact it thinks will be published, since I won't actually be publishing them anywhere when debugging the build.

I added the current limitations with the expectation that Java migrations would be added in a future iteration of the plugin. Given the life-cycle of a Gradle multi-project build (init, config, exec), splitting into two project with a dependency is doubtful to work. Gradle is much more multi-project aware than Maven is, as phases span all projects. This is why it may be required to use a separate process so that the sourceSets.main.runtimeClasspath can be captured during the execution phase.

@axelfontaine
Copy link
Contributor

Another question: why did you favor doing the detour through a properties object and flyway.configure() instead of invoking the setters directly (my preference)?

@axelfontaine
Copy link
Contributor

Also, can the sample be merged into the existing one?

@ben-manes
Copy link
Contributor Author

Properties:
I originally started trying to fix bugs in katta's implementation, where he used that approach. Since I thought his was vetted and just needing a Maven build, I replicated the strategy. I would have otherwise been more explicit, which is my preference too. I'll make that change.

Sample:
Sure, I'll merge it in. I just coerced my example into one of your samples, so there's no wasted effort.

@axelfontaine
Copy link
Contributor

Hi Ben,

how are the changes coming along? Once you're ready, we'll merge it into the main distribution.

Cheers
Axel

P.S.: The Java Migrations are really a must even if they may be a little more trouble to implement.

@ben-manes
Copy link
Contributor Author

Sorry, I haven't had a chance to work on this recently.

Java migrations are tricky. Its not a lack of interest, just need to experiment and see if there's a workable way. I might need to rewrite to fork a process in order to get the compiled classpath

@axelfontaine
Copy link
Contributor

Hi Ben,

no worries. I still believe Java migrations are only tricky in conjunction with JOOQ or QueryDSL-style code generation. And for that working with two modules solves the problem. Maybe we could ask a member of the Gradle team what their recommended practice would be in this case...

Cheers
Axel

@ben-manes
Copy link
Contributor Author

The problem is that a plugin has its classpath set in the buildscript. A multi-project build can have dependencies, but you need them to be on the classpath for the task. This is commonly only done for tasks that fork a new process, like the JavaExec. If I go down that route then it will work fine, but if I run Flyway inside of a plugin then it should have the buildscript classpath.

It may be that a two module solution will work if I set a buildscript dependency on the other module. It may be lazy and work, but typically a buildscript is resolved before the gradle process in order to setup the environment.

@ben-manes
Copy link
Contributor Author

tried having a buildscript depend on a project and it didn't work, as expected. The only other option is to use a buildSrc directory, but then the Java code is not bundled with the application. That may not be acceptable depending on the use-case.

@axelfontaine
Copy link
Contributor

I've posted a StackOverflow question. Let's see what comes back... http://stackoverflow.com/questions/15625080/gradle-plugin-that-must-load-the-compiled-classes-from-the-project

@ben-manes
Copy link
Contributor Author

okay, hacking the classloader seems to do the trick. I'll try to wrap it up over the weekend

@ben-manes
Copy link
Contributor Author

Java migrations are now (optionally) supported.

@ben-manes
Copy link
Contributor Author

Release 0.4

  • Java migration support
  • Better location defaulting behavior
  • Explicitly configuring Flyway (no longer using properties)
  • Improved logging to include the full configuration

I think this covers your concerns. I'll reboot on the task of integrating into your build.

@axelfontaine
Copy link
Contributor

Hi Ben,

just wanted to let you know that I think the latest code you have in your own repo is starting to look really good!!

A few small things:

  • outputting config settings is really for troubleshooting, hence shouldn't the log level be debug instead of info?
  • Java migrations really should work out of the box, without any setting to enable them. Why not reverse the logic and include a flag to disable them if you really need this?
  • is checking for the java plugin to be present enough to also support groovy and scala projects?

Great work!
Axel

@axelfontaine
Copy link
Contributor

One more small point: please use initVersion and initDescription instead of the deprecated initialVersion and initialDescription

@ben-manes
Copy link
Contributor Author

outputting config settings is really for troubleshooting, hence shouldn't the log level
be debug instead of info?

In Gradle it will not be displayed by default. Think of info --> debug and debug --> trace in a typical logger. The -i is useful for debugging and -d is extremely noisy.

Java migrations really should work out of the box, without any setting to enable them.
Why not reverse the logic and include a flag to disable them if you really need this?

The task can't determine if the Java plugin is detected until after the project is evaluated. If we defaulted the dependsOnTasks to include the compileJava task, then we would have to force that the Java plugin was applied. This isn't a preferred style since Gradle supports non-Java environments, like JavaScript and C++. Its also more common to build up the task graph instead of removing dependencies, especially due to forcing ordering with parallel task execution. Instead of breaking task dependencies, one uses conditional logic in the build to decide whether to add them in the first place.

I'd rather not use a flag, since some builds may desire more complex DAGs. Some may also want to write their Java migrations in Scala, so they would want to add the compileScala task. I would be okay with forcing the java plugin and defaulting the dependsOnTasks if that's your preference, but its not idiomatic in Gradle builds.

Since this is a contribution, just confirm your preference.

is checking for the java plugin to be present enough to also support groovy and scala
projects?

Yes, they both depend on the Java plugin.

One more small point: please use initVersion and initDescription instead of the deprecated
initialVersion and initialDescription

Done. These were mapped to the non-deprecated methods, but now properly labeled.


private def addLocationsTo(props) {
def locations = project.flyway.locations
if (project.plugins.hasPlugin('java')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this if block needed at all? Aren't the flyway defaults and the next if block enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why you're looking at this, as its outdated.

flyway's defaults would expect to find it on the classpath. That's not going to happen in this version since the classpath only is for Gradle, not the compiled output. This block put the resources path in scope for flyway's search.

The problem with this approach, though, was that I always added this directory in and flyway does recursive scanning. That caused problems later when we discovered that we wanted to have subfolders for H2 and MySql due to differences in the dialects. The latest version is friendlier about this.

@axelfontaine
Copy link
Contributor

Hi Ben,

Could you push your latest code back onto this pull request so we can start tidying up things and do the merge? I am confident this will work great and will make 2.2 a fabulous release. You have done a fantastic job on this one.

I thought about it, and I think I agree with you regarding having to specify the depends on explicitly for the java plugin. After all, people will need to run compile first before they'll be able to use their java migrations anyway.

As for outputting the config settings, I stand by my point that the main use is troubleshooting, hence logging level should be debug, regardless of Gradle's default logging level.

Cheers
Axel

@ben-manes
Copy link
Contributor Author

Hi Alex,

Can you please try running a Gradle task, then at info, and then debug levels? I think you'll quickly see why I don't think that debug is appropriate. I mentally map Gradle's info -> debug and debug -> trace to match the norm in application loggers. You'll find that trying to grok the debug output to catch the flyway configuration will be challenging.

Sorry for the delay on the maven build aspects. I'll try to give that another go this week. I'm thinking that the least error prone approach may be to have mvn compile trigger a gradle build that outputs to the target directory. I think maven could do the packaging, etc. so its easier to integrate with the build's life cycle. I think no matter what, making maven build the gradle plugin will seem a bit hacky.

@axelfontaine
Copy link
Contributor

Hi Ben,

no worries. I think the best would be to have Gradle build the artifact, and then let Maven re-attach it into the build so it can be deployed to central. I'll be doing similar things with Christian tomorrow night while merging the SBT plugin. I'll let you know if there are any gotchas.

Cheers
Axel

@ben-manes
Copy link
Contributor Author

Hi Axel,

I think I have the integration working, but I'd need you to test that the deploy/release phase work correctly. You should be able to perform those goals and reject the artifacts on the Sonatype dashboard. I didn't attach the artifacts directly due to conflicts with the artifact id, so instead I overwrite the maven jars with the gradle ones. This appears to install into the local repository correctly.

I haven't redone the sample project yet

@ben-manes
Copy link
Contributor Author

Updated the sample project to now include a Gradle build. For some reason V1_2__Another_user is not picked up, but V1_3__And_his_brother is. If I add a V1_4 it is found, so I'm a bit confused.

@ben-manes
Copy link
Contributor Author

nevermind, forgot to add spring to the classpath so flyway ignored the SpringJdbcMigration type.

All works. Should be ready for you to test, review, and merge.

@ben-manes
Copy link
Contributor Author

Released v0.6 with multi-db support. I'll try to update this pull request soon to reflect the latest changes.

@axelfontaine
Copy link
Contributor

Hi Ben,

great stuff! The SBT plugin is now merged. My plan is to merge yours ASAP as well, to have some time left to prepare for a fabulous release at the end of this month.

Do you see a way to simplify the syntax of the common scenario (single db)? 0.5 was a bit easier IMHO.

Cheers
Axel

@ben-manes
Copy link
Contributor Author

The syntax is only slightly worse and is still very readable. I can try to add Luke's (Gradle) suggestion for a simplified form. I didn't do that proactively because the I thought it might create more confusion than it solved by adding complexity. I use the single database configuration and don't find the syntax to bad.

As a refresher, currently the single db configuration looks like,

flyway {
  databases
    main {
      url = "jdbc:h2:${buildDir}/db/flyway"
    }
  }
}

Unfortunately since databases requires a closure to add elements, this could only be flattened out to flyway.databases and not flyway.databases.main.

Luke's suggestion was to add a new syntax that acts like an alias so the syntax could be,

flywaySingle {
  url = "jdbc:h2:${buildDir}/db/flyway"
}

Do you think this is worth adding? Its more terse, but I'm not sure if it carries its weight for a configuration dsl.

@axelfontaine
Copy link
Contributor

How about doing the exact opposite?

flyway {
  url = "jdbc:h2:${buildDir}/db/flyway"
}

and

flywayMulti {
  databases
    main {
      url = "jdbc:h2:${buildDir}/db/flyway"
    }
  }
}

or even better

flyway {
  // Common properties
  user = "sa"

  // Trigger multi-db mode if databases element is present
  databases
    main {
      url = "jdbc:h2:${buildDir}/db/flyway"
    }
  }
}

What do you think? Would having a common supertype for both the outer flyway element and the main element in databases be a good way to solve this?

Cheers
Axel

@ben-manes
Copy link
Contributor Author

flywayMulti

This was the original approach that Allan took, but it felt confusing because it had no way to handle shared defaults and changed the "modes". I thought that having the same configuration act differently because another section was defined broke the principle of least astonishment.

When consulting the Gradle team they suggested the current approach.

Would having a common supertype for both the outer flyway element and the main element
in databases be a good way to solve this?

For shared configuration we currently have a defaults section, e.g.

flyway {
  // Common properties
  defaults {
    user = "sa"
  }

  databases {
    main {
      url = "jdbc:h2:${buildDir}/db/flyway"
    }
  }
}

I think that's a little nicer by being more explicit as to the intent (common properties).

I do like the consistency and clarity of the current approach. If behavior changed if databases was defined to treat the supertype as defaults its very surprising. If the supertype was another database always present, then the multi-db configuration is confusing. Either it would have to detect no properties were set on the supertype and ignore it or always be run, which would have a weird syntax.

I know that we're arguing about 4 lines of configuration noise and that does seem a little ridiculous. I am open to changes or, once contributed, you're welcome to adapt the plugin in any direction. I find one of Gradle's strength over maven is the consistency and clarity of configuration, so I want to avoid losing that with this plugin.

@axelfontaine
Copy link
Contributor

Thanks for taking the time and helping me understand.

First some background. Over the last three years I have been very conservative about the features I've let into the main distribution. This has very often been hard as it meant saying no to interesting things. On the other hand, this bit of conservatism has allowed Flyway to keep great backwards compatibility, with a smooth migration path whenever something did change. I am a strong believer in the concept of published interfaces and really like this quote by Joshua Blosh about API design:

If you are unsure about something, leave it out. You can always add it later.

And this is how I currently feel about the multiple database support as it stands today. For me, adding support for multiple databases could mean two things: adding the option to choose which DB to migrate, or the option to migrate multiple DBs in batch. In Maven the former is addressed by profiles and the latter by executions. My current understanding is that we only address the latter here (multiple DBs in batch).

How could this (in the future) tie in to a suitable syntax to address the former (choose which DB to migrate)?

@ben-manes
Copy link
Contributor Author

A profile in Maven is as simple as an if-else condition in an appropriate place in Groovy. Choosing the database to migrate doesn't require any enhancement to the plugin, its a natural part of how one can configure the build.

Multiple executions is also possible with Gradle, of course. Unlike maven where phases might be used to run a goal multiple times, Gradle's tasks are run once within the task graph. This means that the build tool would have to be invoked multiple times with a configuration flag, once per database.

The problem is when a build tool is used to orchestrate a release as a single execution, which is sometimes done in Ant or Maven. For Gradle that's harder as the task graph is evaluated and a task may only be executed once. This is why I don't use a build tool beyond publishing the artifacts and use tools like Capistrano / Fabric / etc to coordinate the overall process. While I authored the plugin, its only for compile time and I migrate using Java code for the additional flexibility (schema-per-service).

Adding batch migrations later to the plugin makes the configuration inconsistent. If its considered a valid requirement then it should be done before cementing the api design. The batch support doesn't extend the conceptual weight of the plugin, but the approaches to also provide a concise syntax do. They add new concepts (flywaySingle) or auto-magic coercion to different execution behavior. I also don't think that the terser format provides a valuable improvement, as Gradle scripts are very readable and composable. For these reasons I have been more favorable to adding batch support and not a simplified syntax, if I accept the feature as a reasonable use-case.

@axelfontaine
Copy link
Contributor

I am planning to release 2.2 by the end of the month. Could you update your pull request with the latest code so it can be merged?

Thanks
Axel

@ben-manes
Copy link
Contributor Author

updated

@axelfontaine
Copy link
Contributor

Thanks! Did you see the travis-ci build broke? -> https://travis-ci.org/flyway/flyway/builds/8334494

There seems to be a problem with reading the version from the POM. (I haven't looked into it yet)

Also, the pom needs to be configured so the artifacts from build.gradle are reattached to the Maven lifecycle in order to be available for install and deploy (Maven Central Upload)

Cheers
Axel

@ben-manes
Copy link
Contributor Author

The copyLibs task will allow Maven to discover the jars and auto-attach them to the lifecycle. When setting up that integration I verified with install, so it should work with deploy. It seemed a little hacky regardless of which way I went, and I wanted to avoid accidentally also publishing the empty JARs Maven creates as well.

I'll look into the build break. I don't have the env setup at the moment.

@ben-manes
Copy link
Contributor Author

fixed, gtg

A gradle plugin based on the Flyway 2.x series
([original](https://github.com/ben-manes/gradle-flyway-plugin)).

This plugin is integrated into the Maven build using the
[gradle-maven-plugin](https://github.com/if6was9/gradle-maven-plugin)
to invoke the gradle build. The gradle wrapper is invoked so that the
maven packaging phase has the compile jars in the target folder. This
allows maven to pick up the jars in subsequent phases, like install.
@axelfontaine
Copy link
Contributor

I've thought long and hard about this, and read and re-read your arguments. However for 2.2, I would prefer to launch with the flyway {} and flywayMulti{] option. I still feel the regular use-case should be as simple as possible. I may be proven wrong, but I feel the multi-DB config will be less common than the single DB one.

Could you change it back to that?

Thanks
Axel

P.S.: I'm starting the final wrap up for 2.2 and am currently busy writing docs and updating the website. Would you like to contribute a getting started guide for Gradle, and the usage docs, or would you prefer me to do it?

axelfontaine pushed a commit that referenced this pull request Jul 9, 2013
@axelfontaine axelfontaine merged commit 49b0be5 into flyway:master Jul 9, 2013
@axelfontaine
Copy link
Contributor

Thank you so much for your contribution Ben. I'm going to do the last changes as I described in my previous comment and then this should be able to go out the door.

Unless you tell you can get around to it in the next few days, in which case a pull request would be greatly appreciated :-)

@axelfontaine
Copy link
Contributor

I've made the first round of changes. The plugin now builds with Maven and I've also fixed some issues in the sample app.

One thing that caught my eye though, is that it is necessary to declare dependencies (Spring, JDBC driver, ...) on both the plugin and the normal build. Is there a way to automatically pick up the dependencies of the build in the plugin so they don't need to be redeclared? Maven does it this way and I find it quite convenient.

@ben-manes
Copy link
Contributor Author

cool, glad Maven repos works now so it can be build directly. I have other plugins that I haven't contributed back due to that.

You should be able to declare a project.dependency block in the task's constructor so that it is resolved at configuration time (not execution time). You can programmatically inspect the dependencies, such as perhaps finding the flyway plugin, getting its version, and then adding a compile time dependency on flyway-core based on that (or just use a property and replace during process resources).

You may want to look at my gradle-versions-plugin, where I mimic Maven's ability to determine dependency updates. I used Gradle's configuration and dependency apis to figure it all out as a simple script. That may provide some hints and Gradle's docs are a pretty good resource.

@axelfontaine
Copy link
Contributor

Thanks for the tips. The Maven support was a bitch due to the Gradle guys not playing together well with Maven Central as far as their artifacts are concerned.

I got most of the stuff I needed from http://repo.gradle.org/gradle/libs-releases-local , but they forgot to include gradle-base-services-groovy. This meant I had to create a temporary repo at flywaydb.org to host this artifact until they get their act together.

Feel free to vote for these issues to speed things up :-)

Cheers
Axel

ben-manes referenced this pull request in jOOQ/jOOQ Aug 27, 2013
Integrated @ben-manes's Gradle plugin contribution
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

2 participants