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-maven-plugin to plugins and update flyway related configuration #6159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
flyway.user=kitodo
flyway.password=kitodo
flyway.url=jdbc:mysql://localhost/kitodo?useSSL=false
flyway.locations=filesystem:../Kitodo-DataManagement/src/main/resources/db/migration
flyway.url=jdbc:mysql://localhost/kitodo
flyway.locations=filesystem:Kitodo-DataManagement/src/main/resources/db/migration
Comment on lines -3 to +4
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this changes come in than this file and flyway.properties.actions are identical and the copying while CI execution is not needed anymore which is resulting in deleting the copy line in the CI workflow file and the deleting of the flyway.properties.action file.

39 changes: 35 additions & 4 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,37 @@ from system library in Java 11+ -->
<artifactId>maven-surefire-plugin</artifactId>
<version>3.2.5</version>
</plugin>
<plugin>
<groupId>org.flywaydb</groupId>
<artifactId>flyway-maven-plugin</artifactId>
<version>${flyway-maven-plugin.version}</version>
<configuration>
<configFiles>
<!-- local configuration file -->
<!--<configFile>${main.basedir}/config-local/flyway.properties</configFile>-->
<configFile>
${main.basedir}/Kitodo-DataManagement/src/main/resources/db/config/flyway.properties
</configFile>
</configFiles>
</configuration>
<dependencies>
<dependency>
<groupId>com.mysql</groupId>
<artifactId>mysql-connector-j</artifactId>
<version>${mysql.version}</version>
</dependency>
<dependency>
<groupId>org.flywaydb</groupId>
<artifactId>flyway-mysql</artifactId>
<version>${flyway-maven-plugin.version}</version>
</dependency>
<dependency>
<groupId>org.mariadb.jdbc</groupId>
<artifactId>mariadb-java-client</artifactId>
<version>${mariadb-java-client.version}</version>
</dependency>
</dependencies>
</plugin>
Comment on lines +846 to +876
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are coping the whole existing flyway profile and made the flyway execution available without the flyway profile. I don't know is this correct and wanted. In my opinion the flyway execution should only be possible if you add the flyway profile to the maven command and not without this profile.

Only one change is needed in the flyway profile: the adding of the new dependency org.flyway:flyway-mysql. Both other dependencies are already defined in the profile dependency.

</plugins>
</pluginManagement>
<plugins>
Expand Down Expand Up @@ -1016,16 +1047,16 @@ from system library in Java 11+ -->
<configuration>
<configFiles>
<!-- local configuration file -->
<!--<configFile>${basedir}/config-local/flyway.properties</configFile>-->
<!--<configFile>${main.basedir}/config-local/flyway.properties</configFile>-->
<configFile>
${basedir}/Kitodo-DataManagement/src/main/resources/db/config/flyway.properties
${main.basedir}/Kitodo-DataManagement/src/main/resources/db/config/flyway.properties
</configFile>
</configFiles>
</configuration>
<dependencies>
<dependency>
<groupId>mysql</groupId>
<artifactId>mysql-connector-java</artifactId>
<groupId>com.mysql</groupId>
<artifactId>mysql-connector-j</artifactId>
Comment on lines -1027 to +1059
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for fixing this overseen old mysql dependency location which cause trouble in #6205 . I had overseen this in #5849.

Copy link
Member Author

@stweil stweil Oct 7, 2024

Choose a reason for hiding this comment

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

@henning-gerhardt, @solth, how should we proceed with this pull request? Would you prefer a separate PR for mysql-connector-j?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a solution like in #6230 for the 3.7.x release branch as the change is needed for more recent MySQL versions too.

<version>${mysql.version}</version>
</dependency>
<dependency>
Expand Down