-
Notifications
You must be signed in to change notification settings - Fork 107
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 PostgreSQL driver #307
add PostgreSQL driver #307
Conversation
add postgres driver as dependency, so it will be bundled into Docker image
@nitram509 thank you for your contribution 🎉 I'll review your PR soon 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nitram509 your PR looks good 👍
I've one suggestion regarding the test. Please have a look. Thanks 🍪
@SpringBootTest | ||
@ContextConfiguration( | ||
classes = {TestContextJpaConfiguration.class}, | ||
loader = AnnotationConfigContextLoader.class) | ||
@Transactional | ||
@ActiveProfiles({"postgres-docker", "application-junittest.yaml"}) | ||
public class ZeebeApplicationPostgresTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 We could remove this special test for Postgres with a generic configuration in the pom.xml
.
Please have a look at the following commit: camunda-community-hub/zeeqs@b491e2b. With the configuration, it will run all existing tests with Postgres.
I'm sorry that I forgot to point this out earlier 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saig0 I did insert your recommended changes in the POM, but must admit, I don't see a tree in the forest ;)
I mean, when executing mvn test
I can't verify postgres is profile is activated, nor used.
Any recommendations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 🙈 There are so many log statements that it's impossible to observe anything 🌳 I found a configuration to reduce the logs.
But you're right. It doesn't run the tests with the PostgreSQL data source. The problem is that the test setup is slightly different from ZeeQS (i.e. one module vs. a separated module for data). I guess that the easiest way is to bring your initial test back 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saig0
Phillip,
nothing is simpler than that. I just did a revert but left it in history.
Feel free to squash them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nitram509 thanks for your fast reply 👍
I'll add my configuration for the log and merge your PR 🚀
@saig0 can you please release 2.1.1 (not just a snapshot)? 2.1.0 doesn't work with PSQL driver. |
@danshapir yes 😅 I'll build a release in the next few days 🏗️ |
@danshapir a new version 2.2.0 is released 🎉 |
Hi, can you please add Mysql Support, driver not found since 2.1.0 version |
There is no MySQL support until now. Please create a new issue for it. |
add PostgreSQL driver as a dependency, so it will be bundled into Docker image...
as proposed in #254