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

Update to Keycloak 22.0.0 #899

Closed
wants to merge 2 commits into from
Closed

Update to Keycloak 22.0.0 #899

wants to merge 2 commits into from

Conversation

jonasvoelcker
Copy link
Collaborator

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@jonasvoelcker
Copy link
Collaborator Author

I guess the PR of Thomas should be more promising, but I wanted to also add my changes to the POM, maybe we need the best of both worlds ;)

Please work on that one: #898

@jonasvoelcker jonasvoelcker changed the title Update to Keycloak 22.0.0 WIP: Update to Keycloak 22.0.0 Jul 12, 2023
@jonasvoelcker jonasvoelcker mentioned this pull request Jul 12, 2023
1 task
@jonasvoelcker jonasvoelcker changed the title WIP: Update to Keycloak 22.0.0 Update to Keycloak 22.0.0 Jul 12, 2023
@jonasvoelcker jonasvoelcker marked this pull request as draft July 12, 2023 10:16
@thomasdarimont
Copy link
Contributor

@jonasvoelcker Thanks for your contribution! Your PR uses the same approach as I did, so that was a good start already ;-) However the real challenge is to get the changes in a form that works across versions. The "hack" in my PR is that I replace the jakarta to javax packages again and change the rest easy version back via sed before building older versions.

I don't know how sustainable this is in the long run, but this seems to work so far.

How about joining forces and work together on the other PR? :)

@@ -160,6 +155,12 @@
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter</artifactId>
<exclusions>
Copy link
Contributor

@thomasdarimont thomasdarimont Jul 12, 2023

Choose a reason for hiding this comment

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

I think it would make sense to have the different dependencies guarded with a profile.

Perhaps have a default profile that uses the new jakarta ee deps and an "javax" profile, which uses the old dependencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An interesting thought, normally I do write services that follow one simple ruleset which perfectly fits the surrounding services. Frameworks with multiple supported versions are completely new for me. ;)

@jonasvoelcker
Copy link
Collaborator Author

@thomasdarimont Unfortunately we are in the situation that we need to go live sooner than we would like to. We have many other things to solve and didn't start with configuration as code yet. So I am afraid, that I won't be able to contribute much. I will definitely check out your branch and try some things and maybe I'll try a little but I think you can't count on me for now, sorry.

@thomasdarimont
Copy link
Contributor

@jonasvoelcker thanks you for your honest feedback! The PR #898 seems to be on the finishing line. Maybe you will still find time to try it out :)

@sonarcloud
Copy link

sonarcloud bot commented Jul 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 13 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #899 (2581921) into main (bef94e9) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #899      +/-   ##
============================================
+ Coverage     95.45%   95.54%   +0.09%     
- Complexity     1304     1306       +2     
============================================
  Files            76       76              
  Lines          4220     4224       +4     
  Branches        469      469              
============================================
+ Hits           4028     4036       +8     
+ Misses           94       91       -3     
+ Partials         98       97       -1     
Impacted Files Coverage Δ
...sys/keycloak/config/provider/KeycloakProvider.java 86.04% <ø> (+3.48%) ⬆️
...onfig/repository/AuthenticationFlowRepository.java 93.44% <ø> (ø)
...s/keycloak/config/repository/ClientRepository.java 96.74% <ø> (ø)
...cloak/config/repository/ClientScopeRepository.java 97.18% <ø> (ø)
...eycloak/config/repository/ComponentRepository.java 88.57% <ø> (ø)
...oak/config/repository/ExecutionFlowRepository.java 87.50% <ø> (ø)
...ys/keycloak/config/repository/GroupRepository.java 96.19% <ø> (ø)
...g/repository/IdentityProviderMapperRepository.java 92.30% <ø> (ø)
.../config/repository/IdentityProviderRepository.java 94.28% <ø> (ø)
...cloak/config/repository/UserProfileRepository.java 58.82% <ø> (ø)
... and 14 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jonasvoelcker
Copy link
Collaborator Author

Closed in favor of this one: #898

@jonasvoelcker jonasvoelcker deleted the keycloak22 branch July 13, 2023 09:38
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.

2 participants