-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
SmallRye GraphQL 2.11 #43850
SmallRye GraphQL 2.11 #43850
Conversation
FYI @RoMiRoSSaN and @dpolysiou this includes your commits |
This comment has been minimized.
This comment has been minimized.
...loyment/src/test/java/io/quarkus/smallrye/graphql/client/deployment/ssl/SSLTestingTools.java
Outdated
Show resolved
Hide resolved
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good to me but I added a few questions and comments.
Thanks!
it is essential to override the bean's scope to `RequestScoped` (or another similar scope). By default, the | ||
Typesafe client is an application-scoped bean. This guarantees that each new instance of the bean created | ||
after a certificate reload will be configured with the latest certificate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's just me misunderstanding but this looks confusing to me.
AFAICS from the first sentence, ApplicationScoped
beans won't work well.
But the second sentence says some beans are application scoped and then it says This guarantees...
. So you might think that the fact that they are application scoped guarantees it to work. Which is not the case unless I'm mistaken?
Also is it a general limitation of the TLS registry because it seems quite problematic to require all the beans to be request scoped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll clarify that a bit.
Yeah, I think we simply have to make the beans short-lived to get new instances configured with the new certificates. We probably don't want to hot-swap it inside a running GraphQL client instance because that would require closing the underlying HTTP client (and there might be running websocket connections etc), that would be very dangerous.
Btw Dynamic clients are Dependent right now, we plan to figure out a way to make that configurable.
*/ | ||
@ConfigItem | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add since
and forRemoval
. Valid for all the configuration properties you deprecated.
* The name of the TLS configuration (bucket) used for client authentication in the TLS registry. | ||
*/ | ||
@ConfigItem | ||
public Optional<String> tlsConfigurationName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure, this is consistent with the other extensions, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we copied this from the gRPC client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I confirm this is the name used everywhere and documented in the ADR.
0e909a6
to
a06c47f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a06c47f
to
82db312
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Remove field arguments from expected field error path, add separate assertions for the violation property path
82db312
to
3375bb2
Compare
Hopefully fixed the Windows failures now 🫤 |
Status for workflow
|
Status for workflow
|
Fixes #42534 (and more)