-
-
Notifications
You must be signed in to change notification settings - Fork 479
WFCORE-7439 Port attribute definition implementations from wildfly-clustering-common to wildfly-subsystem #6590
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
base: main
Are you sure you want to change the base?
Conversation
0840912 to
6aba754
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Timing wise this change feels like the kind of thing that is good at the start of a development cycle rather than the end? I don't think this is a priority for WildFly 39 Beta but let me know if there is a need to get this in. |
| <groupId>com.fasterxml.jackson.core</groupId> | ||
| <artifactId>jackson-core</artifactId> | ||
| <version>${version.com.fasterxml.jackson}</version> | ||
| <scope>test</scope> |
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.
Out of curiosity, why remove the scope of these dependencies if they are expected to be used always in test classloaders? Even if that could be considered as an antipattern for a bom, in this case, the intention of this bom is clear: is for test only dependencies.
I'm assuming that when a dependency incorporated from this test bom declared in a dependency management ssection does not specify the scope, it will get the test scope but, is not that true?
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.
Out of curiosity, why remove the scope of these dependencies if they are expected to be used always in test classloaders?
- Because that's not how this stuff is meant to work
- This pom defines many artifacts (the one you highlighted is a great example) that are not inherent to test scope.
- The effective pom will otherwise contain artifacts of mixed scopes, since imported artifacts will not have test scope (because, again, that's not how BOMs are supposed to work).
Even if that could be considered as an antipattern for a bom
This isn't actually a BOM. It's an SBOM. A BOM enumerates the artifacts produced by a given build, while an SBOM enumerates the artifacts required to use a given build.
The only scopes that are relevant to the dependencyManagement section of either a BOM or SBOM are: provided and import.
If a module that imports this pom adds a dependency on one the artifacts managed therein, its scope should be predicatable (without requiring help:effective-pom analysis).
| <groupId>org.wildfly.security</groupId> | ||
| <artifactId>wildfly-elytron-tests-common</artifactId> | ||
| <type>test-jar</type> | ||
| <classifier>tests</classifier> |
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.
Out of curiosity, they are equivalent, aren't they? In such a case, what's the motivation for changing it from type to classifier?
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.
Because a BOM should only define artifact identifiers, i.e. groupId, artifactId, version, classifier; not packaging types.
controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java
Outdated
Show resolved
Hide resolved
controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/jboss/as/controller/operations/validation/BoundedParameterValidator.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/jboss/as/controller/operations/validation/BoundedParameterValidator.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public Module apply(ModuleLoader loader) { | ||
| try { | ||
| return loader.loadModule(moduleId); |
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.
There is some related work about this, where we are forcing any attribute definition that loads a module to load the module using the module canonicalized form. See #6584.
Not sure if we can somehow force the Builder for this attribute to always include a ModuleIdentifierUtil.MODULE_NAME_CORRECTOR.
The loader.loadModule(moduleId) accepts an attribute identifier, so my understanding is it should be able to accept both "module:main", "module" or "module:any-slot-name"
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.
Ah - thanks for pointing this out. At the very least, I should be able to auto-add a custom corrector that delegates to this.
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.
There is some related work about this, where we are forcing any attribute definition that loads a module to load the module using the module canonicalized form. See #6584.
Is a parameter corrector sufficient for this? AFAICT, the corrector is invoked before the validator, implying that expression are not yet resolved.
What if the resolved expression is not yet in canonicalised form?
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 - I've added corrector behaviour to the Module and List-based attribute defitions. Let me know if you think there is any additional handling needed.
subsystem/src/main/java/org/wildfly/subsystem/resource/ModuleAttributeDefinition.java
Show resolved
Hide resolved
5b28e0d to
5470d1a
Compare
…idator in favor of safer alternatives - a ParameterValidator should never modify its input.
…on to wildfly-subsystem.
…common to wildfly-subsystem.
…mmon to wildfly-subsystem.
…g-common to wildfly-subsystem.
…g-common to wildfly-subsystem.
…ustering-common to wildfly-subsystem.
5470d1a to
ba6e665
Compare
https://issues.redhat.com/browse/WFCORE-7439 via sub-tasks.