-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix issue where web.xml broke augmentation #1979
Conversation
7342930
to
8ab8a83
Compare
@rsvoboda Could you please make sure the original issue is addressed with this PR? |
8ab8a83
to
7f0485a
Compare
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.
Added some questions.
@@ -61,9 +61,6 @@ public AdditionalBeanBuildItem(Class<?>... beanClasses) { | |||
} | |||
|
|||
AdditionalBeanBuildItem(List<String> beanClasses, boolean removable, DotName defaultScope) { | |||
if (beanClasses.isEmpty()) { | |||
throw new IllegalArgumentException("No bean classes specified"); | |||
} |
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.
Wouldn't the right fix be to not create the build item in the first place? I think it was the initial design.
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.
That is what I thought initially as well, but then I thought that the consumer of AdditionalBeanBuildItem
doesn't really care - for them it's much easier to just pass whatever collection they have and get on with business.
Either way we want to do it, I'm fine with it - it's just that I think this way would avoid similar surprises in the future
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.
That's fine with me. It's easier for AdditionalBeanBuildItem
producers.
7f0485a
to
4d4d26b
Compare
I'll update the tests according to @stuartwdouglas 's suggestions |
b619876
to
c662921
Compare
This happened because an empty list of additional beans is being passed by the Undertow build step Fixes: quarkusio#1976
c662921
to
5ce30ca
Compare
Test converted to "QuarkusUnitTest" |
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.
OK, all good then. Let's wait for CI.
The test passed with this change, I got a bit stuck as I built just arc extension and not devtools/maven module ... |
@rsvoboda Thanks for the input! I had tried it as well on the reproducer project and it also passed for me :) |
This happened because an empty list of additional beans is being passed
by the Undertow build step
Fixes: #1976