-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add a Maven profile to remove GELF support #22615
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
Conversation
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/Environment.java
Outdated
Show resolved
Hide resolved
346b1d4 to
d0961c7
Compare
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/HelpCommandDistTest.java
Outdated
Show resolved
Hide resolved
b299296 to
0246380
Compare
|
Should be ready for review now. |
ghost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.ui.account2.SessionTest#welcomeScreenAsyncLogoutTestKeycloak CI - Account Console IT (firefox) |
0246380 to
2a28be3
Compare
|
I've removed the comprehensive approach for |
vmuzikar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have some test coverage to check GELF is not present in the help. But I agree adding more approved help txts is a bit cumbersome.
Maybe we could create some CheckGelfRemoved test that would just assert the string gelf is not present in help?
2a28be3 to
174b580
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vmuzikar That should be sufficient. I manually tested for GELF enabled/disabled and it works as expected.
Is it ok for you?
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/GelfRemovedTest.java
Show resolved
Hide resolved
ghost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.VerifyProfileTest#testAttributeRequiredAndSelectedByScopeKeycloak CI - Forms IT (chrome) |
|
We'll also need to update the docs to exclude GELF on |
quarkus/config-api/src/main/java/org/keycloak/config/LoggingOptions.java
Outdated
Show resolved
Hide resolved
|
@mabartos - the logging guide mentions GELF explicitly, so I assume it needs to be changed as well (either in this PR, or another PR). |
|
@ahus1 Yes, I'm on it. |
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/GelfRemovedTest.java
Outdated
Show resolved
Hide resolved
Closes keycloak#22515 Co-authored-by: Václav Muzikář <[email protected]>
5765bea to
f8a4b67
Compare
|
After a discussion with @pedroigor, the property persisting during the build time is not a blocker for this PR. Documentation stuff works as expected; I've tried it with product doc. @vmuzikar Could you please check it? (Sorry, I've already squashed all commits, but no big changes were provided since the last time) |
pedroigor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mabartos Sure. We can do it as a follow-up.
|
@mabartos Thanks for the update! Can you please create an issue for the follow-up? |
|
Created follow-up general task related to the static initialization : #22799 |
ghost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.example.cors.CorsExampleAdapterTest#angularCorsProductTest |
Closes #22515