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.

Sign up for GitHub

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

Jump to bottom

KeycloakContainer and Keycloak service refactoring, Keycloak image bump and resulting OCP fixes #986

Merged
merged 1 commit into from
Dec 18, 2023
Merged

KeycloakContainer and Keycloak service refactoring, Keycloak image bump and resulting OCP fixes #986

merged 1 commit into from
Dec 18, 2023

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Dec 18, 2023 •
edited
Loading

Summary

  • I'm updating Keycloak image in TS and there is repeated pattern as I can't use system property in image name. This should help.
  • Running tests for quay.io/keycloak/keycloak:14.0.0 seems beyond the point now as it is not supported in community or product. It makes sense to keep branches like 3.2 in TS to verify support for legacy images, but here we test upstream and I don't think KC 14 support is expected.
  • Bumping Keycloak version to 23.0
  • dropping long deprecated constructor
  • adding constants that are created over and over again in TS
  • making expected log more specific
  • fixing OCP args as we can't just join args with empty space, it was alright when only one argument was defined, but each argument needs it's own item in array
  • new Keycloak image in Docker doesn't accept args merged with empty space, we need to pass them one by one

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik michalvavrik requested a review from jedla97 December 18, 2023 11:36
@michalvavrik michalvavrik force-pushed the feature/kc-annotation-improvements branch from baf34c4 to 93adc23 Compare December 18, 2023 11:45
@michalvavrik
Copy link
Member Author

I didn't expect KC bump to require any changes, mmnt.

@michalvavrik michalvavrik marked this pull request as draft December 18, 2023 12:43
@michalvavrik michalvavrik force-pushed the feature/kc-annotation-improvements branch from 93adc23 to 74244d0 Compare December 18, 2023 12:54
@michalvavrik michalvavrik marked this pull request as ready for review December 18, 2023 12:57
@michalvavrik
Copy link
Member Author

run tests

@jedla97
Copy link
Member

jedla97 commented Dec 18, 2023

There seems to be Keycloak failure on Openshift with jvm and native as well what I just look. Something wasn't able to start in time. Can you look at it?

@michalvavrik
Copy link
Member Author

There seems to be Keycloak failure on Openshift with jvm and native as well what I just look. Something wasn't able to start in time. Can you look at it?

yes

@michalvavrik michalvavrik marked this pull request as draft December 18, 2023 15:43
@michalvavrik michalvavrik force-pushed the feature/kc-annotation-improvements branch from 74244d0 to e00c41f Compare December 18, 2023 17:25
@michalvavrik michalvavrik marked this pull request as ready for review December 18, 2023 17:28
@michalvavrik
Copy link
Member Author

run tests

@michalvavrik michalvavrik changed the title KeycloakContainer needs to resolve config properties, use newer image and have more specific expected log message KeycloakContainer and Keycloak service refactoring, Keycloak image bump and resulting OCP fixes Dec 18, 2023
@michalvavrik
Copy link
Member Author

There is a single failure - OpenShiftGreetingResourceIT in native. It's impossible for me to run this test as it takes eternity to upload binaries to OCP with my network (30 minutes and counting) but I've checked both native and JVM OCP template of this test - there are no args.

Therefore I suggested the failure cannot be related to changes here. @jedla97 please review the PR.

jedla97
jedla97 requested changes Dec 18, 2023
View reviewed changes
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

Overall it's look good just one comment. And looking through TF I see another usage of Keycloak 22 here. If there is need to for these stay on KC22 we can merge this.

quarkus-test-service-keycloak/src/main/java/io/quarkus/test/bootstrap/KeycloakService.java
@@ -11,6 +11,9 @@

public class KeycloakService extends BaseService<KeycloakService> {

public static final String DEFAULT_REALM_BASE_PATH = "/realms";
public static final String DEFAULT_REALM = "test-realm";
public static final String DEFAULT_REALM_FILE = "/keycloak-realm.json";
Copy link
Member

Choose a reason for hiding this comment

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

+1 for adding these in one place.

examples/keycloak/src/test/java/io/quarkus/qe/OpenShiftUsingCustomTemplateResourceIT.java
@@ -11,12 +15,12 @@
@OpenShiftScenario
public class OpenShiftUsingCustomTemplateResourceIT {

private static final String REALM_DEFAULT = "test-realm";
private static final String CLIENT_ID_DEFAULT = "test-application-client";
private static final String CLIENT_SECRET_DEFAULT = "test-application-client-secret";

@Container(image = "quay.io/keycloak/keycloak:22.0.1", expectedLog = "started", port = 8080)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't here be also newer Keycloak?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea why there is hardcoded image instead of @KeycloakContainer used, but I suppose we can do it in a separate PR? I'm happy to change annotation, but I'd like to make release and fix TS.

Also @gtroitsk have some ticket on getting rid of hardcoded images, maybe he can take care of it?

Copy link
Member

Choose a reason for hiding this comment

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

Ok no problem with this. Make sense to leave it the task with removing hardcoded images.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks; @gtroitsk please create PR (ideally with others if there are such hardcoded images) and link it with this one.

Copy link
Member

Choose a reason for hiding this comment

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

I will do it after finish with TS related stuff

jedla97
jedla97 approved these changes Dec 18, 2023
View reviewed changes
Copy link
Member

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

As the older version will be removed in different PR. It's look good and we can merge this.

@jedla97 jedla97 merged commit dcf617a into quarkus-qe:main Dec 18, 2023
8 of 9 checks passed
@michalvavrik michalvavrik deleted the feature/kc-annotation-improvements branch December 18, 2023 20:02
This was referenced Dec 18, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtroitsk gtroitsk gtroitsk left review comments

@rsvoboda rsvoboda rsvoboda left review comments

@jedla97 jedla97 jedla97 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michalvavrik @jedla97 @rsvoboda @gtroitsk