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

port to 25 #245

Merged
merged 4 commits into from
Jun 19, 2024
Merged

port to 25 #245

merged 4 commits into from
Jun 19, 2024

Conversation

xgp
Copy link
Member

@xgp xgp commented Jun 10, 2024

  • ported to keycloak 25
  • cleaned old team entities
  • still need to get events-dependent tests working and import config

@xgp
Copy link
Member Author

xgp commented Jun 10, 2024

@rtufisi the testImportConfig test isn't working in 25. Can you take a look in this branch? I've disabled the test, but I'd like us to understand why it's not working and fix the issue.

@xgp xgp requested a review from rtufisi June 10, 2024 19:54
@rtufisi
Copy link
Collaborator

rtufisi commented Jun 12, 2024

@rtufisi the testImportConfig test isn't working in 25. Can you take a look in this branch? I've disabled the test, but I'd like us to understand why it's not working and fix the issue.

Sure. I will take a look

@@ -807,7 +790,7 @@ public void testAddGetDeleteRolesBulk() throws Exception {
}
};
resp =
SimpleHttp.doPut(url, httpClient)
Copy link
Collaborator

@rtufisi rtufisi Jun 13, 2024

Choose a reason for hiding this comment

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

I saw that the latest implementation: https://github.com/keycloak/keycloak/blob/ca0833b2e48ef3fac6c524bcb5b0680a73fdb6fa/server-spi-private/src/main/java/org/keycloak/broker/provider/util/SimpleHttp.java#L4 has a new implementation with signature doPut(String url, HttpClient client, long maxConsumedResponseSize)
Couldn't we use the default?

    default long getMaxConsumedResponseSize() {
        return DEFAULT_MAX_CONSUMED_RESPONSE_SIZE;
    }

and not create a new class?
E.q:

public static SimpleHttp doPut(String url, KeycloakSession session) {
        HttpClientProvider provider = session.getProvider(HttpClientProvider.class);
        return doPut(url, provider.getHttpClient(), provider.getMaxConsumedResponseSize());
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

That takes a KeycloakSession which we don't have. The one that takes an HttpClient is protected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry . I didn't saw the access modifier.

@xgp xgp marked this pull request as ready for review June 19, 2024 16:56
@xgp xgp merged commit 22a77ee into main Jun 19, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants