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

feat: Bundle a MySQL driver #451

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

nhrrsn
Copy link
Contributor

@nhrrsn nhrrsn commented Sep 30, 2022

Add the mysql driver as a dependency.
As discusse in issue: #337
Based on previouse work to add the Postgres driver: #307

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2022

CLA assistant check
All committers have signed the CLA.

@saig0 saig0 self-requested a review September 30, 2022 11:58
@saig0
Copy link
Contributor

saig0 commented Oct 4, 2022

@ian4h thank you for your contribution. 🎉 I'll have a look at your PR in the next few days. 👀

@nhrrsn
Copy link
Contributor Author

nhrrsn commented Oct 4, 2022

Hi @saig0 ok thanks no rush.
Actually I've noticed an issue with the changes to use MYSQL so I'll ask for some feedback from you before making any changes.
MYSQL defaults to a lower case table naming strategy. So when the repository goes to use a nativeQuery (for example in the ProcessRepository getElementInstanceStatisticsByKeyAndIntentIn() method) and it uses the upper case table names, then the query fails.
The solution to this is add the following environment variable:
- spring.jpa.hibernate.naming.physical-strategy=org.hibernate.boot.model.naming.PhysicalNamingStrategyStandardImpl
I've tested this and it works as expected.
I was going to just add some additional info to the README to document this solution for MYSQL databases. Is this ok with you or would you rather another solution?
Thanks

@saig0
Copy link
Contributor

saig0 commented Oct 5, 2022

I was going to just add some additional info to the README to document this solution for MYSQL databases. Is this ok with you or would you rather another solution?

Sounds fine to me. 👍

Thank you for coming up with the solution immediately.

@saig0
Copy link
Contributor

saig0 commented Oct 5, 2022

@ian4h it would be nice to add a new profile for MySQL to the docker-compose file. It would make the testing a lot easier.

@nhrrsn
Copy link
Contributor Author

nhrrsn commented Oct 6, 2022

HI @saig0 ok that makes sense. I've added a new profile to the docker compose file.
I've also added an example MySql configuration to the README.

In order to test the docker-compose was working I built the docker image locally and changed the image for the simple-monitor-mysql container to point at my local version instead.

Copy link
Contributor

@saig0 saig0 left a comment

Choose a reason for hiding this comment

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

@ian4h looks all good. Your PR is ready to merge. 🎉

@saig0 saig0 changed the title Add mysql support by bundling the mysql driver feat: Bundle a MySQL driver Oct 13, 2022
@saig0 saig0 merged commit dd6301d into camunda-community-hub:main Oct 13, 2022
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.

None yet

3 participants