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

Bump strimzi kafka-oauth-client to 0.14.0 with fix for native #36698

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

ozangunalp
Copy link
Contributor

Revamp kafka-oauth-keycloak test using keycloak-server, moving KeycloakContainer to test framework.

Revert 62909e5

Adds kafka-oauth-common to application/bom because of strimzi/strimzi-kafka-oauth#209
It is added as provided scoped dependency to kafka-client extension runtime module bacause of the new substitutions required. These substitutions are applied only when strimzi-oauth is found in classpath.

This opens the discussion to move strimzi-oauth support to a quarkiverse extension.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/kafka area/testing labels Oct 25, 2023
@ozangunalp ozangunalp force-pushed the strimzi_oauth_native_fix branch from 591b19e to 4308428 Compare October 25, 2023 19:02
@quarkus-bot

This comment has been minimized.

extensions/kafka-client/runtime/pom.xml Show resolved Hide resolved
import io.quarkus.runtime.configuration.ConfigurationException;
import io.restassured.RestAssured;

public class KeycloakContainer extends GenericContainer<KeycloakContainer> {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have a dev service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the kafka-oauth-keycloak IT we need to configure the kafka instance and the app with keycloak config. That's why we had an ad-hoc Keycloak test container – and the config was significantly complex, I simplified it in this change.

The quarkus-test-keycloak-server is recommended only when there is a reason not to use the Keycloak dev service for testing.

Copy link
Member

Choose a reason for hiding this comment

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

@ozangunalp The refactoring looks good, and it is sound in any case, but Dev Service can be configured with a custom realm path, the Keycloak test factory is not really recommended any more, but indeed, quite a few OIDC tests still depend on it to test against legacy Keycloak distributions.

When you get a chance, please check what is missing from DevServicesConfig and may be we can enhance something, please open issues if you'll identify what can be. For example, I'm planning to support copying various test resources to the container to migrate KeycloakAuthorization tests to Dev Service

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Keycloak test factory and KeycloakTestClient related refactoring looks good

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 26, 2023

Failing Jobs - Building abdfa22

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11
JVM Tests - JDK 17 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 21 Build ⚠️ Check → Logs Raw logs
✔️ Maven Tests - JDK 11
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

@cescoffier cescoffier merged commit 7b0f197 into quarkusio:main Oct 30, 2023
49 of 53 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 30, 2023
@github-actions
Copy link

🙈 The PR is closed and the preview is expired.

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

Successfully merging this pull request may close these issues.

3 participants