Feature/liquibase configuration #340
Conversation
hohwille
added
common
|
FYI: I typically do not start reviews before PRs are up-to-date with So please fix CI and merge |
|
@dixyushi can you fix the merge conflicts and bring this PR to a "ready for review" state? |
…dixyushi/devon4j into feature/liquibase-configuration
| </plugins> | ||
| </pluginManagement> | ||
| </build> | ||
| </project> |
There was a problem hiding this comment.
I have restored the file but still it is showing in files changed.
There was a problem hiding this comment.
Current pom.xml has linux EOL style:
cat -ben pom.xml
1 <?xml version="1.0" encoding="UTF-8"?>$
2 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"$
3 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">$
4 <modelVersion>4.0.0</modelVersion>$
....
Yours has windows encoding. Maybe something with your git setup or the editor you are using to edit XML files.
cat -ben pom.xml.ayushi
1 <?xml version="1.0" encoding="UTF-8"?>^M$
2 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"^M$
3 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">^M$
4 <modelVersion>4.0.0</modelVersion>^M$
...
We once had such changes that added a Byte-Order-Mark to a pom.xml that was not easily visible in the diff but broke the release as maven pull-parser can not handle BOM. That is why am a little picky about such things.
| <module>server</module> | ||
| </modules> | ||
| </project> |
There was a problem hiding this comment.
I have restored the file here too but still it is showing in files changed.
| @@ -8,7 +8,7 @@ | |||
| public class TestCleanerPluginNone implements TestCleanerPlugin { | |||
|
|
|||
| @Override | |||
| public void cleanup() { | |||
| public void cleanup() throws Exception { | |||
There was a problem hiding this comment.
Do not do this. Please use RuntimeExceptions and avoid checked exceptions wherever possible:
https://github.com/devonfw/devon4j/blob/develop/documentation/guide-exceptions.asciidoc
Also we can not afterwards change APIs in an incompatible manner:
Existing projects would get compiler errors after upgrading devon4j when you do this change.
| #if($dbMigration == 'flyway') | ||
| testCleanerPlugin = new TestCleanerPluginFlyway(); | ||
| #else if($dbMigration == 'liquibase') | ||
| testCleanerPlugin = new TestCleanerPluginLiquibase(); | ||
| #end |
There was a problem hiding this comment.
testCleanerPlugin is already annotated with @Inject.
So we do we manually have to initialize it here?
IMHO this does not make sense.
| } | ||
| catch(Exception exception) { | ||
| LOG.error("Exception occurred while performing cleanup", exception); | ||
| } |
There was a problem hiding this comment.
This is what happens due to this checked exception...
Please remove the checked exception and then also this try catch around the cleanup. Catching and further ignoring errors is not a good style if no reasonable compensation of the error can be done.
|
|
||
| private Flyway flyway; | ||
|
|
||
| private TestCleanerPlugin testCleanerPlugin; |
There was a problem hiding this comment.
Use @Inject. You should get familiar with DI when doing Java coding in devon4j:
https://github.com/devonfw/devon4j/blob/master/documentation/guide-dependency-injection.asciidoc
|
This issue seems state. |
I have resolved the issue(i.e. Consider liquibase #303). In that I have changed the configuration of flyway into liquibase.