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

Add a Maven profile to remove GELF support #22615

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

mabartos
Copy link
Contributor

@mabartos mabartos commented Aug 22, 2023

Closes #22515

@mabartos mabartos added the status/hold PR should not be merged. On hold for later. label Aug 22, 2023
@mabartos mabartos self-assigned this Aug 22, 2023
@vmuzikar vmuzikar linked an issue Aug 23, 2023 that may be closed by this pull request
quarkus/deployment/pom.xml Outdated Show resolved Hide resolved
quarkus/runtime/pom.xml Outdated Show resolved Hide resolved
@mabartos mabartos changed the title WiP - Remove GELF from product distribution Add a Maven profile to remove GELF support Aug 24, 2023
@mabartos mabartos removed the status/hold PR should not be merged. On hold for later. label Aug 24, 2023
@mabartos mabartos marked this pull request as ready for review August 24, 2023 16:56
@mabartos mabartos requested review from a team as code owners August 24, 2023 16:56
@mabartos mabartos force-pushed the gelfProductRemoval branch 3 times, most recently from b299296 to 0246380 Compare August 24, 2023 17:03
@mabartos
Copy link
Contributor Author

Should be ready for review now.

Copy link

@ghost ghost left a 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

@ghost
Copy link

ghost commented Aug 24, 2023

Unreported flaky test detected

If 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#welcomeScreenAsyncLogoutTest

Keycloak CI - Account Console IT (firefox)

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.keycloak.testsuite.util.URLAssert URL expected to begin with: https://localhost:8543/auth/realms/test/protocol/openid-connect/auth ; actual URL: https://localhost:8543/auth/realms/test/account/#/personal-info within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
...

Report flaky test

@mabartos
Copy link
Contributor Author

I've removed the comprehensive approach for HelpCommandDistTest to be working for the product. It'd bring a lot of maintenance difficulties. You can check out what'd be necessary to make it work here: mabartos@0246380

Copy link
Contributor

@vmuzikar vmuzikar left a 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?

Copy link
Contributor Author

@mabartos mabartos left a 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?

Copy link

@ghost ghost left a 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

@ghost
Copy link

ghost commented Aug 25, 2023

Unreported flaky test detected

If 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#testAttributeRequiredAndSelectedByScope

Keycloak CI - Forms IT (chrome)

java.lang.IllegalArgumentException: No enum constant org.keycloak.testsuite.pages.AppPage.RequestType.
	at java.base/java.lang.Enum.valueOf(Enum.java:273)
	at org.keycloak.testsuite.pages.AppPage$RequestType.valueOf(AppPage.java:56)
	at org.keycloak.testsuite.pages.AppPage.getRequestType(AppPage.java:49)
	at jdk.internal.reflect.GeneratedMethodAccessor552.invoke(Unknown Source)
...

Report flaky test

quarkus/server/pom.xml Outdated Show resolved Hide resolved
@vmuzikar
Copy link
Contributor

We'll also need to update the docs to exclude GELF on -Dproduct.

@ahus1
Copy link
Contributor

ahus1 commented Aug 28, 2023

@mabartos - the logging guide mentions GELF explicitly, so I assume it needs to be changed as well (either in this PR, or another PR).

@mabartos mabartos marked this pull request as draft August 28, 2023 10:54
@mabartos
Copy link
Contributor Author

@ahus1 Yes, I'm on it.

Closes keycloak#22515

Co-authored-by: Václav Muzikář <vmuzikar@redhat.com>
@mabartos mabartos marked this pull request as ready for review August 29, 2023 13:53
@mabartos
Copy link
Contributor Author

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)

Copy link
Contributor

@pedroigor pedroigor left a 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.

@vmuzikar vmuzikar enabled auto-merge (squash) August 29, 2023 14:02
@vmuzikar
Copy link
Contributor

@mabartos Thanks for the update! Can you please create an issue for the follow-up?

@mabartos
Copy link
Contributor Author

Created follow-up general task related to the static initialization : #22799

@vmuzikar vmuzikar merged commit 7c013e8 into keycloak:main Aug 29, 2023
72 checks passed
Copy link

@ghost ghost left a 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

@ghost
Copy link

ghost commented Aug 29, 2023

Unreported flaky test detected

If 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

Keycloak CI - Base IT (2)

junit.framework.AssertionFailedError
	at junit.framework.Assert.fail(Assert.java:55)
	at junit.framework.Assert.assertTrue(Assert.java:22)
	at junit.framework.Assert.assertNotNull(Assert.java:256)
	at junit.framework.Assert.assertNotNull(Assert.java:248)
...

Report flaky test

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

Successfully merging this pull request may close these issues.

Add a Maven profile to remove GELF support
5 participants