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

clarify beans.xml handling in CDI Lite #591

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jan 31, 2022

No description provided.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 31, 2022

This is my proposal for jakartaee/cdi-tck#336.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

I am fine specifying it like this so long as all interested parties acknowledge that while we spec it, we still cannot test it with TCKs.

@@ -38,6 +38,9 @@ An _implicit bean archive_ is:
Any other archive which contains a `beans.xml` file is not portable in {cdi_lite}.
More kinds of bean archives exist in {cdi_full}.

Implementations that do not support {cdi_full} are required to ignore the content of the `beans.xml` file, except for the `bean-discovery-mode` attribute.
Implementations that do not support {cdi_full} are required to detect presence of an archive which contains a `beans.xml` file that has `bean-discovery-mode` attribute set to `all` and treat it as a deployment problem.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am tempted to say we should add a commented out note about this assertion not being testable with a link to GH issue discussing that. Statement like this is bound to bounce back eventually and stir the same discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps we could water it down (just to make it clearer it's not being asserted anywhere) and say:

Implementations that do not support {cdi_full} are encouraged/expected to...

Copy link
Contributor

Choose a reason for hiding this comment

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

How about rewording something like:
Implementations that do not support {cdi_full} are required to detect presence of an archive which contains a beans.xml file that has bean-discovery-mode attribute. The attribute value neither none nor annotated can be treated as a deployment problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

During the CDI meeting we agreed that we want to state this as required even though it cannot currently be tested with TCKs.
With that in mind, I think we can go ahead and merge this PR in its current form.

@Ladicek Ladicek marked this pull request as ready for review February 1, 2022 15:17
@starksm64 starksm64 merged commit 2ae2275 into jakartaee:master Feb 7, 2022
@Ladicek Ladicek deleted the beans-xml-lite-clarifications branch February 8, 2022 11:31
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.

5 participants