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

OIDC client integration for GraphQL clients #36375

Merged
merged 1 commit into from
Oct 11, 2023
Merged

OIDC client integration for GraphQL clients #36375

merged 1 commit into from
Oct 11, 2023

Conversation

jmartisk
Copy link
Contributor

@jmartisk jmartisk commented Oct 10, 2023 •
edited
Loading

Fixes #35878

@jmartisk jmartisk requested a review from sberyozkin October 10, 2023 09:50
@quarkus-bot quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation area/graphql area/oidc area/smallrye labels Oct 10, 2023
@sberyozkin
Copy link
Member

sberyozkin commented Oct 10, 2023 •
edited
Loading

Thanks @jmartisk It looks good, I can see from the tests how it works, I hope GraphQL users will like it.
My only suggestion is to improve the docs with more information, more examples, to make it very easy to start

Cheers

@jmartisk jmartisk force-pushed the graphql-client-oidc branch from e7320cf to d9ff6fd Compare October 10, 2023 10:35
@jmartisk jmartisk force-pushed the graphql-client-oidc branch from d9ff6fd to fb0fe29 Compare October 10, 2023 11:16
sberyozkin
sberyozkin approved these changes Oct 10, 2023
View reviewed changes
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.

LGTM, a few more tiny doc suggestions proposed

@jmartisk jmartisk force-pushed the graphql-client-oidc branch from fb0fe29 to 7bc9afe Compare October 11, 2023 06:00
@jmartisk jmartisk force-pushed the graphql-client-oidc branch from 7bc9afe to bd2beff Compare October 11, 2023 06:19
@jmartisk jmartisk marked this pull request as ready for review October 11, 2023 06:20
@github-actions
Copy link

github-actions bot commented Oct 11, 2023 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@sberyozkin
Copy link
Member

Proposing a backport in case it can still be an option post 3.5.0.CR1 but before the final 3.5.0 release

@jmartisk
Copy link
Contributor Author

jmartisk commented Oct 11, 2023 •
edited
Loading

I'm kinda hoping to get this into 3.5, but we will let @gsmet decide. It's a new extension so it's not that trivial.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 11, 2023

Failing Jobs - Building bd2beff

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

Full information is available in the Build summary check run.

Failures

⚙️ Maven Tests - JDK 11 Windows #

- Failing: integration-tests/maven

📦 integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testExternalReloadableArtifacts line 1448 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.DevMojoIT was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.JarRunnerIT.testNonAsciiDir line 70 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

✖ io.quarkus.maven.it.DevMojoIT.testExternalReloadableArtifacts line 1448 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.DevMojoIT was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.JarRunnerIT.testNonAsciiDir line 70 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

@sberyozkin sberyozkin merged commit e0ca7c6 into quarkusio:main Oct 11, 2023
66 of 68 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 11, 2023
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 11, 2023
@sberyozkin
Copy link
Member

@jmartisk i don't think these test failures are related

@jmartisk jmartisk deleted the graphql-client-oidc branch October 12, 2023 05:31
@sberyozkin
Copy link
Member

I've remove the backport label until we figure out what is wrong with the tests

@sberyozkin sberyozkin mentioned this pull request Oct 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sberyozkin sberyozkin sberyozkin approved these changes

@bulldog98 bulldog98 bulldog98 left review comments

Assignees
No one assigned
Labels
area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation area/graphql area/oidc area/smallrye kind/enhancement New feature or request release/noteworthy-feature
Projects
Quarkus Documentation
Status: Done
Milestone
3.6.0.CR1
Development

Successfully merging this pull request may close these issues.

GraphQL typesafe client - support for @OidcClientFilter
3 participants
@jmartisk @sberyozkin @bulldog98