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

Use Quarkus Platform BOM #21426

Merged
merged 2 commits into from
Jul 6, 2023
Merged

Conversation

vmuzikar
Copy link
Contributor

@vmuzikar vmuzikar commented Jul 4, 2023

Closes #20570
Closes #15870
Closes #9075

Supersedes #21078
Changes from the original PR:

Umbrella for post-22 follow-up issues: #21356

This is currently blocked as we're waiting for the release of Quarkus Platform BOM for 3.2.0.Final

@vmuzikar vmuzikar force-pushed the platform-bom-pzaoral branch from 54a6ec4 to eb0e23f Compare July 4, 2023 15:12
@vmuzikar vmuzikar closed this Jul 5, 2023
@vmuzikar vmuzikar reopened this Jul 5, 2023
@vmuzikar vmuzikar marked this pull request as ready for review July 5, 2023 10:20
@vmuzikar vmuzikar requested review from a team as code owners July 5, 2023 10:20
@vmuzikar vmuzikar requested a review from a team July 5, 2023 10:20
@vmuzikar
Copy link
Contributor Author

vmuzikar commented Jul 5, 2023

In order to merge this, we need reviews from @keycloak/store and @keycloak/core.

@miquelsi Could you please confirm the QE pipelines are happy with this change?

@ASzc Could you please confirm it's ok for prod?

@abstractj Can you please double check the dependencies upgrades/downgrades are ok from CVEs perspective?

@pedroigor With regards to #9075, I currently went the simple path and excluded the Quarkus dev services on mvn level (without using profiles). Are you aware of any use cases we need the dev services for during development currently?

Copy link
Contributor

@sguilhen sguilhen left a comment

Choose a reason for hiding this comment

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

Thsanks @vmuzikar ! I'm ok with the much needed cleanup being in this PR - don't think we need a separate one just for that.

From store perspective, it should be ok!

Copy link
Contributor

@ASzc ASzc left a comment

Choose a reason for hiding this comment

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

I see com.fasterxml.jackson.core:jackson-core and com.fasterxml.jackson.core:jackson-annotations are being removed? There are some product build parameters related to those two, specifically forcing version 2.13.4.redhat-00002, I can't remember why. If they're being removed then easy enough to drop these related build parameters too.

I also see there are some new exclusions for deployment dependencies. That's good, in theory should help clean up non-RHBQ jars in our zip.

Otherwise seems ok. Ideally with the volume of pom changes a trial product build would be performed before merging this PR, but we don't have automation for that yet. If you're interested in doing it manually I can guide you, otherwise I'll fix any errors that crop up in the nightly build after this is merged

Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Reviewed the changes, thank you for this cleanup. I see that the distribution is now down to 168 MB instead of 188 MB, which is great.

I did a short local test, and it looks fine to me. Thanks!

Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Reviewed the changes, thank you for this cleanup. I see that the distribution is now down to 168M instead of 188 MB, which is great.

I did a short local test, and it looks fine to me. Thanks!

@abstractj abstractj merged commit 3f4d971 into keycloak:main Jul 6, 2023
@vmuzikar
Copy link
Contributor Author

Thanks for all the reviews!

@miquelsi Just wanted to double check if you were able to run the internal pipelines with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants