testcontainers / testcontainers-jooq-codegen-maven-plugin

jOOQ code generator using Testcontainers
Apache License 2.0
49 stars 10 forks source link

Feature/more properties #3

Closed romchellis closed 1 year ago

romchellis commented 1 year ago

Added schema related properties to flyway. It will more correct to provide default schema and history table. @sivaprasadreddy please review

sivaprasadreddy commented 1 year ago

@romchellis I was thinking of adding the support for specifying any flyway properties in a generic way instead of creating specific fields in FlywayProps.java.

For example, in the official JOOQ plugin there is support for adding properties under <database> as described in: https://www.jooq.org/doc/latest/manual/code-generation/codegen-advanced/codegen-config-database/codegen-database-name/

Maybe we can implement something along similar lines so that we don't have to keep adding logic for every property.

romchellis commented 1 year ago

Actually we can, but regarding flyway props there are no need to use all of them, i think users are needed only those ones

sivaprasadreddy commented 1 year ago

@romchellis At the moment, we can assume those fields are sufficient, but we never know what all customisation options people want to use. So, I think implementing generic properties approach to support any Flyway property would be better.

romchellis commented 1 year ago

@sivaprasadreddy Should i figure out how to do it in this PR or i can do it in next PR?

sivaprasadreddy commented 1 year ago

You can do it in this PR.

romchellis commented 1 year ago

Hi @sivaprasadreddy, most of the Flyway properties are necessary for long-term maintenance, not for on-the-fly migration. As it is not flexible enough (like JOOQ, for example), it doesn't have XML configuration. So my proposal remains the same: I think we can add properties to the plugin one by one. Also, it's unlikely that new properties will need to be added frequently.

romchellis commented 1 year ago

Hi @sivaprasadreddy I found a solution. I provided a map to flyway properties, what do you think about such way?

sivaprasadreddy commented 1 year ago

@romchellis The changes are looking good.

Just one thing to keep in mind, I am thinking of supporting Liquibase also in future versions. So, there should be a mechanism to check which migration to run flyway or Liquibase? if user configure both in plugin then throw Exception.

<configuration>
        <database>
            <type>POSTGRES</type>
            <containerImage>postgres:15.2-alpine</containerImage>
        </database>
        <!-- only one of flyway or liquibase config should be here. If both are configured, throw error. -->
        <flyway>
            <locations>
                filesystem:src/main/resources/db/migration/postgres
            </locations>
        </flyway>
        <liquibase>
            ....
        <liquibase>
</configuration>
romchellis commented 1 year ago

@sivaprasadreddy If you are ok with it , let's push changes further. At that point we haven't included Liquibase support, so let's keep them as separate tasks/branches.

Actually there can be mechanism where we can pick one or another tool for migration. I can do it later , we can collaborate.

sivaprasadreddy commented 1 year ago

@romchellis Can you please change the plugin version from 0.0.1 to 0.0.2-SNAPSHOT in pom.xml files?

romchellis commented 1 year ago

Done, 0.0.2-SNAPSHOT