Skip to content

Remove MS SQL JDBC driver from the Keycloak product - #22986

Merged
ahus1 merged 1 commit into
keycloak:mainfrom
ahus1:is-22983-remove-mssql-driver
Sep 7, 2023
Merged

Remove MS SQL JDBC driver from the Keycloak product#22986
ahus1 merged 1 commit into
keycloak:mainfrom
ahus1:is-22983-remove-mssql-driver

Conversation

@ahus1

@ahus1 ahus1 commented Sep 5, 2023

Copy link
Copy Markdown
Member

Closes #22983

@ahus1 ahus1 self-assigned this Sep 5, 2023
@ahus1
ahus1 requested a review from mhajas September 5, 2023 13:41
@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from 5ed96ba to b80c27b Compare September 5, 2023 13:43
@ahus1

ahus1 commented Sep 5, 2023

Copy link
Copy Markdown
Member Author

@mhajas - the builds are still running, still, maybe you can have a look already at this PR if I missed something.

Thanks!

mhajas
mhajas reviewed Sep 5, 2023
View reviewed changes

@mhajas mhajas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we add a note also to the Release notes? https://github.com/keycloak/keycloak/blob/main/docs/documentation/release_notes/topics/23_0_0.adoc

@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from b80c27b to 250a817 Compare September 5, 2023 14:28
@ahus1
ahus1 requested a review from mhajas September 5, 2023 14:29
@ahus1

ahus1 commented Sep 5, 2023

Copy link
Copy Markdown
Member Author

Thank you @mhajas for catching this, updated!

@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from 250a817 to e3c688b Compare September 5, 2023 14:33
mhajas
mhajas previously approved these changes Sep 5, 2023
View reviewed changes

@mhajas mhajas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @ahus1!

@ghost ghost added the flaky-test label Sep 5, 2023
This was referenced Sep 5, 2023
Closed
Closed
@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from e3c688b to b0b9a1b Compare September 5, 2023 17:48
@ghost ghost mentioned this pull request Sep 5, 2023
Closed
@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from b0b9a1b to 0602613 Compare September 6, 2023 07:31
vmuzikar
vmuzikar requested changes Sep 6, 2023
View reviewed changes

@vmuzikar vmuzikar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we perhaps use similar approach as here? I.e. a specific maven profile to remove it and the same profile to add it to the testsuite?

vmuzikar
vmuzikar reviewed Sep 6, 2023
View reviewed changes

@vmuzikar vmuzikar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The driver should be removed from product only, in community it should remain.

@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from 0602613 to c41d9b3 Compare September 6, 2023 10:06
@mhajas

mhajas commented Sep 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

The driver should be removed from product only, in community it should remain.

If we want to follow the same as for the Oracle driver, this should not be done in this PR. See your comment in the Oracle PR: #22453 (review)

Here is the PR to main: #22577

@mhajas

mhajas commented Sep 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Could we perhaps use similar approach as here? I.e. a specific maven profile to remove it and the same profile to add it to the testsuite?

The same as above, we don't want to do this here if we follow the same as for the Oracle driver.

@vmuzikar

vmuzikar commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

@mhajas After discussion with @stianst, we decided to take a bit different approach and include all drivers in upstream, see also #22999

@ahus1

ahus1 commented Sep 6, 2023

Copy link
Copy Markdown
Member Author

Will wait for #23006 to be merged first, then this will be rebased/updated to be simpler to be merged to the 22 branch.

This was referenced Sep 6, 2023
Closed
Closed
Closed
Closed
@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from c41d9b3 to c426b42 Compare September 6, 2023 19:26
@ahus1
ahus1 force-pushed the is-22983-remove-mssql-driver branch from c426b42 to 59bdaa1 Compare September 7, 2023 06:40
@ahus1
ahus1 marked this pull request as ready for review September 7, 2023 09:25
@ahus1
ahus1 requested review from a team as code owners September 7, 2023 09:25
@ahus1
ahus1 requested a review from a team September 7, 2023 09:25
@ahus1
ahus1 requested a review from vmuzikar September 7, 2023 09:25
@ahus1

ahus1 commented Sep 7, 2023

Copy link
Copy Markdown
Member Author

@vmuzikar - this now applies the changes to similar to the Oracle driver. I also verified that it can be cherry-picked without modifications to KC22.

Please review

Thanks!

/cc @mabartos

mabartos
mabartos approved these changes Sep 7, 2023
View reviewed changes

@mabartos mabartos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ahus1 LGTM. Works as expected.

@ahus1

ahus1 commented Sep 7, 2023

Copy link
Copy Markdown
Member Author

Hi @vmuzikar - if you agree, this should now be ready to be merged. Thanks!

vmuzikar
vmuzikar approved these changes Sep 7, 2023
View reviewed changes

@vmuzikar vmuzikar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ahus1 Thank you, LGTM.

Comment thread quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java
@ahus1 ahus1 mentioned this pull request Sep 7, 2023
Closed
2 tasks
@ahus1
ahus1 merged commit 2eb37db into keycloak:main Sep 7, 2023
@ahus1 ahus1 mentioned this pull request Sep 7, 2023
Merged
@ahus1
ahus1 deleted the is-22983-remove-mssql-driver branch September 21, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar approved these changes
@mabartos mabartos mabartos approved these changes
+1 more reviewer
@mhajas mhajas mhajas left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@ahus1 ahus1

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove MS SQL JDBC driver from the Keycloak product

4 participants

@ahus1 @mhajas @vmuzikar @mabartos