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

Change the JsonRPC Service for OIDC to use recorder #44590

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

phillip-kruger
Copy link
Member

This is for #44181. It does not fix it, but takes Dev UI out of the picture.

This PR change the way we make data available to the JSON RPC service. It now use a recorder to record it directly on the JsonRPC Service.

On another note, we can probably use build time data (that can also accept a runtime value) for some of the json-rpc methods. However this is a bigger and riskier change (with the same result) so I kept it simple. If the maintainers (@sberyozkin) wants to pursue this I can show what I suggest.

This comment has been minimized.

Copy link

quarkus-bot bot commented Nov 20, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 0d98259.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/oidc-code-flow

io.quarkus.it.keycloak.CodeFlowTest.testTokenRefresh - History

  • expected: <true> but was: <false> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at io.quarkus.it.keycloak.CodeFlowTest$3.call(CodeFlowTest.java:733)
	at io.quarkus.it.keycloak.CodeFlowTest$3.call(CodeFlowTest.java:720)
	at org.awaitility.core.CallableCondition$ConditionEvaluationWrapper.eval(CallableCondition.java:99)
	at org.awaitility.core.ConditionAwaiter$ConditionPoller.call(ConditionAwaiter.java:248)
	at org.awaitility.core.ConditionAwaiter$ConditionPoller.call(ConditionAwaiter.java:235)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

lgtm; I am not sure I agree it doesn't fix the issue, isn't remaining issue duplicate of the #39644 ?

@michalvavrik
Copy link
Member

On another note, we can probably use build time data (that can also accept a runtime value) for some of the json-rpc methods. However this is a bigger and riskier change (with the same result) so I kept it simple. If the maintainers (@sberyozkin) wants to pursue this I can show what I suggest.

I am interested.

@sberyozkin
Copy link
Member

Thanks @phillip-kruger @michalvavrik, I'll quickly try this PR with both Keycloak and non Keycloak provider, soon and merge then

@phillip-kruger
Copy link
Member Author

On another note, we can probably use build time data (that can also accept a runtime value) for some of the json-rpc methods. However this is a bigger and riskier change (with the same result) so I kept it simple. If the maintainers (@sberyozkin) wants to pursue this I can show what I suggest.

I am interested.

@michalvavrik - So for the getProperties method, that can move to a BuildTimeAction. This means that you do not need to create a JsonRPC method, you just provide the code in the deployment module. See https://quarkus.io/guides/dev-ui#jsonrpc-against-the-deployment-classpath

The BuildTimeAction can also take a RuntimeValue that will be returned, meaning you can record the data in the recoreder, and that will be returned on the getProperties call.

@sberyozkin
Copy link
Member

Tested with Keycloak and Auth0, thanks @phillip-kruger

@sberyozkin sberyozkin merged commit 8220502 into quarkusio:main Nov 24, 2024
23 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 24, 2024
@michalvavrik
Copy link
Member

michalvavrik commented Nov 27, 2024

On another note, we can probably use build time data (that can also accept a runtime value) for some of the json-rpc methods. However this is a bigger and riskier change (with the same result) so I kept it simple. If the maintainers (@sberyozkin) wants to pursue this I can show what I suggest.

I am interested.

@michalvavrik - So for the getProperties method, that can move to a BuildTimeAction. This means that you do not need to create a JsonRPC method, you just provide the code in the deployment module. See https://quarkus.io/guides/dev-ui#jsonrpc-against-the-deployment-classpath

The BuildTimeAction can also take a RuntimeValue that will be returned, meaning you can record the data in the recoreder, and that will be returned on the getProperties call.

I put it on my list and will apply it before long, thanks @phillip-kruger

@phillip-kruger
Copy link
Member Author

phillip-kruger commented Nov 27, 2024

With this new change (#44700) it's less important, as we will remove all dev ui runtime from prod builds, so moving things to BuildTimeActions not really needed.

@michalvavrik
Copy link
Member

With this new change (#44700) it's less important, as we will remove all dev ui runtime from prod builds, so moving things to BuildTimeActions not really needed.

good, thanks really

@michalvavrik
Copy link
Member

I'll add a backport label so that #45360 can be fixed.

@geoand
Copy link
Contributor

geoand commented Jan 4, 2025

👍🏽

@gsmet gsmet modified the milestones: 3.18 - main, 3.17.6 Jan 7, 2025
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.

5 participants