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 bean discovery mode to annotated in all beans.xml #1765

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented Mar 1, 2023

Follow up on the discussion in Quarkus issue quarkusio/quarkus#31378 (comment)

In short, a Quarkus app using graphql now logs warnings because graphql artifacts contains beans.xml with discovery mode all which is unsupported in CDI Lite and in Quarkus/Arc.

I quick scanned the sub modules containing beans.xml and it seems that all classes meant to be beans already have bean defining annotations (although keep in mind I am very much unfamiliar with this project).
The change seems to pass all tests in this repo and shouldn't affect Quarkus (since even up until now, Arc treated these JARs as annotated anyway).
It would be nice to test this with some other container such as WFLY to see if we're good. @jmartisk what's the easiest way to verify that? Are there some WFLY tests we could run? If so, where can I find them?

@manovotn manovotn requested a review from jmartisk March 1, 2023 12:45
@jmartisk
Copy link
Member

jmartisk commented Mar 1, 2023

Let me run the tests for the wildfly graphql feature pack, that should be pretty quick (I'll just need to cherry-pick it into the 2.0.x branch, because we don't have a WildFly version that is compatible with 2.1 right now)

@jmartisk
Copy link
Member

jmartisk commented Mar 1, 2023

Cool, it worked!
Let's now wait for the Quarkus tests and then merge it

@manovotn
Copy link
Contributor Author

manovotn commented Mar 1, 2023

Glad to hear that, thanks for checking!

@jmartisk jmartisk merged commit 37eee63 into smallrye:main Mar 1, 2023
@jmartisk
Copy link
Member

jmartisk commented Mar 1, 2023

Thanks! We just had a release yesterday, but we'll make another one ~ next week or so

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