tlberglund / groovy-liquibase

Yet Another Groovy DSL for Liquibase
Other
85 stars 66 forks source link

Version 0.7.6 breaks old behavior of the databaseChangeLog's include tag. #37

Closed stevesaliman closed 10 years ago

stevesaliman commented 11 years ago

The behavior of the include(path: "some/path/to/changesets") tag of dabaseChangeLog has changed significantly since the 0.7.3 release. In the 0.7.3 release, only groovy files in the given path were included, and they were included in alphabetical order.

Release 0.7.6 Includes all files in the given directory, and in no particular order. This breaks change logs that were counting on the changesets being run in a particular order, which is how I think most people use changesets. There will also be errors if the given directory contains other files, such as backup files, documentation in text files, or even .swp files that exist if the user has a changeset file open in the editor.

There is a workaround:

databaseChangeLog {
  include(path: "some/path")
}

can be replaced with

databaseChangeLog {
    def filenames = []
    def dirname = "src/main/db/changelogs"
    new File("some/path").eachFileMatch(~/.*\.groovy/) { f ->
        filenames << f
    }
    filenames.sort().each() { f ->
        include(file: "some/path/${f.name}")
    }
}

This works, but it is a bit messy and I think we should consider reverting back to the old behavior. I can submit a pull request if you agree.

Steve

mbruner commented 11 years ago

Well, that's very strange, If you look at the following lines: https://github.com/tlberglund/groovy-liquibase/blob/master/src/main/groovy/com/augusttechgroup/liquibase/delegate/DatabaseChangeLogDelegate.groovy#L111-L123 You will see that they are pretty similar to your code and it works well in our project - we have same case with alphabetic ordering of groovy scripts.

I'll try to figure out what's broken. Could you please provide me with some examples from your project?

Max

stevesaliman commented 11 years ago

I think the issue begins on line 104. The call to resourceAccessor.getResources(params.path) loads all the files in the given path. It loads everything in the path, regardless of extension, and in random order.

Then the code goes through resource returned. If the resource is a file, it gets included (line 122). If it is a directory, we include all the groovy files in that directory (lines 111 to 123). This has an interesting side effect of its own. Let's say you have a directory called src/main/db/changesets with all the changeset files you want to include. Let's also say that there is a src/main/db/changesets/bak directory with backup copies. include(path:"src/main/db/changesets") would include all the files in the first directory, and all the .groovy files in the second. I didn't know if this was intentional or not...

I think we may want to skip the resource loading, create a File object out of the path, and iterate through its children. I can attempt a patch if you like, but I wasn't sure what lines 106 to 108 were intended to do.

Steve

PS: We may also want to update the Liquibase dependency to 2.0.5 to match the updated dependency of the Liquibase plugin.

stevesaliman commented 10 years ago

This is fixed in Version 1.0.0 of the DSL