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

[WFCORE-7031] Do not provision unstable annotation module for stabili… #6218

Merged
merged 1 commit into from
Nov 20, 2024
Merged

[WFCORE-7031] Do not provision unstable annotation module for stabili… #6218

merged 1 commit into from
Nov 20, 2024

Conversation

yersan
Copy link
Collaborator

@yersan yersan commented Oct 16, 2024

…ty levels higher than preview

Jira issue: https://issues.redhat.com/browse/WFCORE-7031

@yersan yersan requested a review from kabir October 16, 2024 11:12
kabir
kabir reviewed Oct 16, 2024
View reviewed changes
...n/resources/modules/system/layers/base/org/wildfly/extension/core-management/main/module.xml
@@ -30,7 +30,7 @@
<module name="org.wildfly.extension.core-management-client"/>
<module name="org.wildfly.security.elytron-private"/>
<module name="org.wildfly.service"/>
<module name="org.wildfly.unstable.annotation.api.indexer"/>
<module name="org.wildfly._internal.unstable-api-annotation-index"/>
<module name="org.wildfly.unstable.annotation.api.indexer" optional="true"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not totally familiar with how this works.
Does being optional AND the dependency having jboss.stability=preview mean it gets provisioned?

Copy link
Contributor

Choose a reason for hiding this comment

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

When core-management is transformed onto a Galleon package, dependencies on optional modules are ignored. An optional module is expected to be provisioned from another root. For example, a layer, a subsystem, ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking for 'org.wildfly._internal.unstable-api-annotation-index' and 'org.wildfly.unstable.annotation.api.indexer' in the wildfly-core sources, I don't see those being referenced anywhere that could make a difference apart from via this module dependency

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added the packages via model.xml, let's see how it goes

@wildfly-ci

This comment was marked as outdated.

Sign in to view
@wildfly-ci

This comment was marked as outdated.

Sign in to view
@yersan yersan requested review from kabir and jfdenise October 17, 2024 10:24
@wildfly-ci

This comment was marked as outdated.

Sign in to view
@wildfly-ci

This comment was marked as outdated.

Sign in to view
@kabir
Copy link
Contributor

kabir commented Oct 17, 2024

@yersan Failures seem related

@jfdenise
Copy link
Contributor

@yersan I think that the inclusion thanks to model.xml of these 2 modules need to be replicated in WildFly Full repo.
https://github.com/wildfly/wildfly/blob/main/ee-feature-pack/galleon-local/src/main/resources/configs/standalone/model.xml
https://github.com/wildfly/wildfly/blob/main/ee-feature-pack/galleon-local/src/main/resources/configs/domain/model.xml

jfdenise
jfdenise approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@jfdenise jfdenise left a comment

Choose a reason for hiding this comment

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

Other than the WildFly full failure that should require changes in WildFly itself, that is good with me.

@wildfly-ci

This comment was marked as outdated.

Sign in to view
@yersan
Copy link
Collaborator Author

yersan commented Oct 17, 2024

@jfdenise @kabir I am missing something here.

If we add packages via model.xml, are those packages provisioned when we are trimming the server using layers?

I mean, the error:

java.lang.AssertionError: Some expected modules have not been provisioned in test-all-layers: org.wildfly._internal.unstable-api-annotation-index,org.wildfly.unstable.annotation.api.indexer
  at org.junit.Assert.fail(Assert.java:89)
  at org.jboss.as.test.layers.LayersTest.testLayersModuleUse(LayersTest.java:172)
  at org.jboss.as.test.shared.LayersTestBase.testLayersModuleUse(LayersTestBase.java:413)
  at org.jboss.as.test.layers.base.LayersTestCase.testLayersModuleUse(LayersTestCase.java:37)

Looks legit to me, but I guess adding them to model.xml won't fix it in WildFly. Also, I don't get why the Galleon Jobs passed under the current situation for WildFly Core, I would expect them to fail due the above assumption where packages are not added to layers if they are declared only in model.xml

What I am missing?

@jfdenise
Copy link
Contributor

jfdenise commented Oct 17, 2024 •
edited
Loading

@yersan , when included in standalone/model.xml, layers will benefit from them. Layers based provisioning inherits from model.xml content.

@yersan
Copy link
Collaborator Author

yersan commented Oct 17, 2024

Integration Jobs (until the WildFly Counterpart gets merged)

https://ci.wildfly.org/viewLog.html?buildId=462644 (Linux integration)
https://ci.wildfly.org/viewLog.html?buildId=462646 (Bootable JAR)
https://ci.wildfly.org/viewLog.html?buildId=462642 (Galleon)
https://ci.wildfly.org/viewLog.html?buildId=462648 (Microprofile)
https://ci.wildfly.org/viewLog.html?buildId=462659 (WildFly Preview)
https://ci.wildfly.org/viewLog.html?buildId=462632 (Windows)

@wildfly-ci

This comment was marked as outdated.

Sign in to view
@wildfly-ci

This comment was marked as outdated.

Sign in to view
@yersan
Copy link
Collaborator Author

yersan commented Oct 25, 2024

This one needs wildfly/wildfly#18287 merged first

@yersan
Copy link
Collaborator Author

yersan commented Nov 18, 2024

wildfly/wildfly#18287 was merged, so kicking off the integration Jobs again

@yersan
Copy link
Collaborator Author

yersan commented Nov 19, 2024

Hello @kabir , now CI looks green, so I think we are ready to go, would you mind reviewing it again and approved from your side? thanks!

@kabir
Copy link
Contributor

kabir commented Nov 19, 2024

Hi,

According to my understanding it now looks correct.

From @jfdenise previous comment:

  • The original dependencies is now optional
  • Then the model.xml parts should count as being referenced from a root (and only kick in when preview or below)

@yersan
Copy link
Collaborator Author

yersan commented Nov 19, 2024

From @jfdenise previous comment:

The original dependencies is now optional
Then the model.xml parts should count as being referenced from a root (and only kick in when preview or below)

Yes, this is what we did here and in WildFly with the "optional="true" valid-for-stability="preview" configuration on the model.xml.

@kabir based on your feedback, I assume this is approved from your side (I don't see the green tick under your review though)

@yersan yersan added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Nov 20, 2024
@yersan yersan merged commit 04627bd into wildfly:main Nov 20, 2024
13 checks passed
@yersan yersan deleted the WFCORE-7031 branch November 20, 2024 10:32
@yersan
Copy link
Collaborator Author

yersan commented Nov 20, 2024

Thanks @kabir @jfdenise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfdenise jfdenise jfdenise approved these changes

@kabir kabir kabir approved these changes

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yersan @wildfly-ci @kabir @jfdenise