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

Fix issue #278: add compliancemode parameter in JDBC URL if absent #298

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

maximevw
Copy link
Contributor

This closes #278

This adds a CassandraDatabaseConnection implementation for DatabaseConnection interface, extending JdbcConnection. In this implementation, the compliance mode parameter Liquibase is added to the JDBC URL if necessary when the used JDBC driver is com.ing.data.cassandra.jdbc.CassandraDriver.

@MalloD12
Copy link
Contributor

Hey @maximevw,

It seems the unit tests you added on this PR are failing because it cannot find CassandraDatabaseConnection. See below:

Error:  liquibase.ext.cassandra.database.CassandraDatabaseConnectionTest.open with jdbc:cassandra://localhost:9042/betterbotz?compliancemode=Liquibase&localdatacenter=datacenter1 -- Time elapsed: 0.010 s <<< ERROR!
java.lang.NoClassDefFoundError: liquibase/ext/cassandra/database/CassandraDatabaseConnection

Same for the other scenario. Would you mind having a look?

Thanks,
Daniel.

@maximevw
Copy link
Contributor Author

maximevw commented May 24, 2024

Hello @MalloD12,

Running the command mvn -B test -P 'coverage' (the same as in the failed pipeline) locally gives me a build success and all tests pass:

...
[INFO] Running liquibase.ext.cassandra.database.CassandraDatabaseConnectionTest
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.904 s -- in liquibase.ext.cassandra.database.CassandraDatabaseConnectionTest
[INFO] liquibase.ext.cassandra.database.CassandraDatabaseConnectionTest.open with jdbc:cassandra://localhost:9042/betterbotz?compliancemode=Liquibase&localdatacenter=datacenter1 -- Time elapsed: 0.833 s
[INFO] liquibase.ext.cassandra.database.CassandraDatabaseConnectionTest.open with jdbc:cassandra://localhost:9042/betterbotz?localdatacenter=datacenter1 -- Time elapsed: 0.002 s
[INFO] liquibase.ext.cassandra.database.CassandraDatabaseConnectionTest.open with jdbc:cassandra://localhost:9042/betterbotz -- Time elapsed: 0.002 s
[INFO] liquibase.ext.cassandra.database.CassandraDatabaseConnectionTest.open with #url -- Time elapsed: 0.852 s
[INFO] Running liquibase.ext.cassandra.MysqlCompatibilityCheckTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.705 s -- in liquibase.ext.cassandra.MysqlCompatibilityCheckTest
[INFO] liquibase.ext.cassandra.MysqlCompatibilityCheckTest.Liquibase should continue to work with Mysql when Cassandra Extension is in classpath -- Time elapsed: 1.684 s
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] 
[INFO] --- jacoco-maven-plugin:0.8.11:report (report) @ liquibase-cassandra ---
[INFO] Loading execution data file /home/maxime/dev/liquibase-cassandra/target/jacoco.exec
[INFO] Analyzed bundle 'Liquibase Extension: Cassandra Database Support' with 21 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

But, I found these differences:

On GitHub - test step:

[INFO] --- resources:3.3.1:resources (default-resources) @ liquibase-cassandra ---
[INFO] Copying 6 resources from src/main/resources to target/classes

[INFO] --- compiler:3.11.0:compile (default-compile) @ liquibase-cassandra ---
[INFO] Changes detected - recompiling the module! :input tree
[INFO] Compiling 20 source files with javac [debug deprecation target 1.8] to target/classes

...

[INFO] --- gplus:3.0.2:compileTests (default) @ liquibase-cassandra ---
[INFO] Using isolated classloader, without GMavenPlus classpath.
[INFO] Using Groovy 4.0.20 to perform compileTests.
[INFO] invokedynamic enabled.
[INFO] Parallel parsing enabled.
[INFO] Compiled 8 files.

In my local environment and on GitHub - build and package step:

[INFO] --- maven-resources-plugin:3.3.1:resources (default-resources) @ liquibase-cassandra ---
[INFO] Copying 7 resources from src/main/resources to target/classes

[INFO] --- compiler:3.11.0:compile (default-compile) @ liquibase-cassandra ---
[INFO] Changes detected - recompiling the module! :source
[INFO] Compiling 21 source files with javac [debug deprecation target 1.8] to target/classes

...

[INFO] --- gmavenplus-plugin:3.0.2:compileTests (default) @ liquibase-cassandra ---
[INFO] Using isolated classloader, without GMavenPlus classpath.
[INFO] Using Groovy 4.0.20 to perform compileTests.
[INFO] invokedynamic enabled.
[INFO] Parallel parsing enabled.
[INFO] Compiled 10 files.

The differences in the number of compiled files come from the files added by this PR.
But, I guess it's not really relevant because the test pipeline checks out the main branch then copies the content of target folder previously built from the branch to merge.

So, I don't really understand why the CassandraDatabaseConnection class is missing in the classpath when running tests here.

Do you prefer I skip the added test for now? Or there is something in particular to do when adding new tests in this project?

@MalloD12
Copy link
Contributor

@filipelautert @sayaliM0412: I know that discrepancy with the compiled (GH vs Local) files Maxime is showing us could be for different reasons, but is there anything you guys can suggest to try?

@maximevw: I will generally recommend moving forward with a PR by adding some unit tests as you already provided.

Thanks,
Daniel.

@maximevw
Copy link
Contributor Author

Additional question: is there a reason to re-build the main branch before running tests in the steps "Test - Java xx" instead of just running tests with the classes copied to the target folder from the step "Build & package" (containing the missing class only present in the branch to merge)?
This might explain the observed difference, although this seems unlikely.

@shubha11m
Copy link

can anyone please help to connect aws keypace and creeate table using liquibase in springbooot project

@maximevw
Copy link
Contributor Author

can anyone please help to connect aws keypace and creeate table using liquibase in springbooot project

Hello @shubha11m,
Regarding AWS Keyspaces, there is currently an open issue about it: #297
For integration with Spring Boot, please take a look at: #212

Also please create separate issues in case of questions. This will avoid polluting unrelated topics like here. 🙏🏻

@MalloD12
Copy link
Contributor

Hi @maximevw,

I just want to let you know the team has found a build issue in one of our workflow configurations. We have created an internal ticket for that. I'll leave this PR on hold until that issue is fixed so then we can see a clean build run on this PR.

Thanks,
Daniel.

@maximevw
Copy link
Contributor Author

maximevw commented May 28, 2024

@MalloD12 thank you for the feedback.
I assume this one could fix the issue: liquibase/build-logic#210 😄

@MalloD12
Copy link
Contributor

Nice, good finding @maximevw!

Copy link

sonarcloud bot commented Jun 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@maximevw
Copy link
Contributor Author

maximevw commented Jun 5, 2024

@MalloD12 Good news! The build is fixed and all the tests pass now 🙂

Copy link
Contributor

@MalloD12 MalloD12 left a comment

Choose a reason for hiding this comment

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

Approved.

Thank you @maximevw!

@MalloD12 MalloD12 requested a review from filipelautert June 5, 2024 18:50

@Override
public int getPriority() {
return 201;
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we do CONSTANT + increment, but we can fix it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the recommandation @filipelautert: I didn't know how was fixed this value, so I took a value for which I was almost sure the CassandraDatabaseConnection will be used. I can fix that in a separate PR, as you suggested; do you have an example of the good way to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@filipelautert filipelautert merged commit 56a5e0f into liquibase:main Jun 5, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Append compliancemode=Liquibase to JDBC URL
6 participants