Add Microsoft SQL Server support #130
Conversation
…oak and additional feedback from pull-request keycloak#114
|
I also need the Microsoft SQL Server support. |
|
Thanks for the PR. I've only had a chance to have a brief look so far and all looks very good. I especially like the fact that you caught the breaking changes I just made and fix it almost instantaneously ;) I will try to take a proper look next week so we can get this merged. |
|
We are also interested in this as we are essentially doing the same thing in our custom image. As a side note, the current version is 7.0.0, see https://github.com/Microsoft/mssql-jdbc/releases. Not sure its necessary to take this, we are currently on 6.4.0 as well. |
| @@ -0,0 +1,9 @@ | |||
| /subsystem=datasources/data-source=KeycloakDS: remove() | |||
| /subsystem=datasources/data-source=KeycloakDS: add(jndi-name=java:jboss/datasources/KeycloakDS,enabled=true,use-java-context=true,use-ccm=true, connection-url="jdbc:sqlserver://${env.DB_ADDR:mssql}:${env.DB_PORT:1433};databaseName=${env.DB_DATABASE:keycloak};integratedSecurity=false;user=${env.DB_USER:keycloak};password=${env.DB_PASSWORD:password};${env.JDBC_PARAMS:}", driver-name=sqlserver) | |||
There was a problem hiding this comment.
Wouldn't it make sense to also add "sendStringParametersAsUnicode=false"? We are using this and reduced the amount of deadlocks quite substantially on mass data imports, see Hyneks comment on https://issues.jboss.org/browse/KEYCLOAK-3210.
server/Dockerfile
Outdated
| @@ -4,6 +4,7 @@ ENV KEYCLOAK_VERSION 4.3.0.Final | |||
| ENV JDBC_POSTGRES_VERSION 42.2.2 | |||
| ENV JDBC_MYSQL_VERSION 5.1.46 | |||
| ENV JDBC_MARIADB_VERSION 2.2.3 | |||
| ENV JDBC_MSSQL_VERSION 6.4.0 | |||
There was a problem hiding this comment.
If I am not mistaken, downloading the JDBC driver needs to be added to build-keycloak.sh
|
Please update with latest changes to the image and add sendStringParametersAsUnicode=false. Once that's done I'll test it and merge. |
… driver download, add sendStringParametersAsUnicode=false to connection string)
|
Changes looks good, but can you please rebase and squash into a single commit? |
|
Tried this from steps in README and it doesn't work as the user and password are wrong. The db is also not created. Can someone update the README to include setting DB_USER/DB_PASSWORD and also get it to create the Keycloak database, or at least provide a docker exec command to do that prior to starting Keycloak? Also, I need this squashed to a single commit, which is awkward to do since it has merge commits in there. |
|
@stianst - looking at the SQL Server side of the house, it looks like the reason that the DB_USER/DB_PASSWORD setting and database autocreation aren't present is that the base MS SQL Server Docker image doesn't support them yet: |
|
That's fine, we just need to make sure that the README covers those aspects and that the example on how to run it includes the steps needed to get it working. At this point I couldn't even with giving it a bit of a go get it running. |
|
Completely fair point. Attached is a Docker Compose file I used to get it up and running based on some of the POC work I was doing. There is an additional image - ( |
|
@RolandTa, @sschu - I've been tinkering with this fork quite a bit for some POC work and I'd be happy to do any outstanding legwork (on a fork of a fork, if need be) to get this PR in a state where it can be merged. From what I gather, the open items are to clarify the examples / documentation and to squash the merge down to a single commit. Is this correct? |
|
@michaeljmcd - I'm glad if you can take over this PR, fell free to fork it if you have to. The open topics are as mentioned by you (clarify examples / documentation) and squash to a single commit. I'm currently on heavy workload, therefore I'm really happy if you can solve those open items and bring this topic further. |
|
@RolandTa @michaeljmcd I'm closing this PR for now. But feel free to reopen or open a new one, when you think it's ready to go. Also, please make sure you have a JIRA for this if you reopen. |
image, comparable to the support in place for databases like H2 or MySQL as per https://issues.jboss.org/browse/KEYCLOAK-9903. This combines feedback from pull requests jboss-dockerfiles#114, jboss-dockerfiles#130 and is sourced from RolandTa/keycloak for its basis.
Add Microsoft SQL Server support based on changes from jgorin86/keycloak and additional feedback from pull-request #114