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

Add flyway defaults properties, autoloading default config file #19

Conversation

romchellis
Copy link
Contributor

In scope of this pull request i've done

  • Autoloading flyway.conf according to original Flyway implementation
  • Moved skip property to root configuration
  • Added default locations for Flyway as in original implementation
  • Improved Flyway logging
  • Added baseline for readme and links to examples (will extend readme more later)

Partially improves: #18
Resolves: #15

pom.xml Outdated Show resolved Hide resolved
@romchellis
Copy link
Contributor Author

Hi @sivaprasadreddy , hope you are doing well.
Could you share your opinion?

Copy link
Contributor

@sivaprasadreddy sivaprasadreddy left a comment

Choose a reason for hiding this comment

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

Please take a look at the review comments.


## Migration tools:

### Flyway
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding basic information about Flyway with a link to official flyway documentation would be good.

README.md Outdated
- Plugin migration and code generation might be skipped using `skip` property <br/>
- If you need to reuse existing database connection - take a look at [Jooq section](#Jooq)

## Databases:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be "Database Configuration:"?

README.md Outdated
The `testcontainers-jooq-codegen-maven-plugin` simplifies the jOOQ code generation
by using [Testcontainers](https://www.testcontainers.org/) and applying database migrations.
The `testcontainers-jooq-codegen-maven-plugin` simplifies the jOOQ code generation
by using [Testcontainers](https://www.testcontainers.org/) and applying database migrations. <br/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to add HTML <br/> tags. Adding 2 blank lines is sufficient.

</flyway>
```

### Liquibase
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding basic Liquibase info with a link to official Liquibase docs will be good.

README.md Outdated

#### Properties

`generator` - property to configure JOOQ generation plugin itself, original
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be rephrased to generator - property to configure JOOQ code generation settings. See https://www.jooq.org/doc/latest/manual/code-generation/codegen-configuration for all the supporting configuration properties.

README.md Outdated
existing database will be used, no container won't be spin up <br/>
`baseDir` - directory relative to which generated sources will be generated , `{project.basedir}` - default

#### In order to configure JOOQ add `jooq` block to your plugin `configuration`
Copy link
Contributor

Choose a reason for hiding this comment

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

Heading can be "jooq block configuration"

README.md Outdated

Reference to Liquibase properties - https://docs.liquibase.com/concepts/connections/creating-config-properties.html

#### In order to use Liquibase add `liquibase` block to your plugin `configuration`
Copy link
Contributor

Choose a reason for hiding this comment

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

Heading can be "liquibase block configuration"

README.md Outdated
</jooq>
```

#### Before run - Make sure your plugin has dependency on a chosen jdbc database driver implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Heading can be "Plugin dependencies configuration"

</dependency>
```

## Examples
Copy link
Contributor

Choose a reason for hiding this comment

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

Before Examples section, it would be good to have a complete plugin configuration example.

@@ -88,6 +89,11 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I didn't face any problems using Lombok, but a lot of people don't like to use Lombok because of some unforeseen issues, especially combined with other tools. So, I would prefer not to use Lombok as we might not even need Lombok features so much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree, we might, but this way I reduced code size thrice, there are pretty much duplications without Lombok.
if you are 100 percent against it then - I will remove it

@romchellis
Copy link
Contributor Author

Hi @sivaprasadreddy , pls review once again, I've fixed all of the comments except Lombok.

@sivaprasadreddy sivaprasadreddy merged commit 7ee5199 into testcontainers:main Jul 5, 2023
1 check passed
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.

Consider change default for flyway locations from classpath:db/migration to filesystem:db/migration
2 participants