WFCORE-6995 Allow stability-specific resource transformations for mixed domains#6214
Open
pferraro wants to merge 3 commits into
Open
WFCORE-6995 Allow stability-specific resource transformations for mixed domains#6214pferraro wants to merge 3 commits into
pferraro wants to merge 3 commits into
Conversation
pferraro
force-pushed
the
WFCORE-6995
branch
3 times, most recently
from
7be71fb to
e05436b
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Core -> WildFly Preview Integration Build 14066 outcome was FAILURE using a merge of e05436b |
|
Core -> WildFly Preview Integration Build 14067 outcome was FAILURE using a merge of e05436b |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
pferraro
force-pushed
the
WFCORE-6995
branch
from
e05436b to
c997c15
Compare
|
Core -> Full Integration Build 14427 outcome was UNKNOWN using a merge of c997c15 |
|
Core -> Full Integration Build 14126 outcome was UNKNOWN using a merge of c997c15 |
|
Core -> WildFly Preview Integration Build 14208 outcome was UNKNOWN using a merge of c997c15 |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
…s using SubsystemModel enumerations.
pferraro
force-pushed
the
WFCORE-6995
branch
from
c997c15 to
aa426d5
Compare
yersan
reviewed
Feb 14, 2025
Contributor
yersan
left a comment
yersan
left a comment
There was a problem hiding this comment.
Just added comments, I do not have the expertise on transformers API to validate the work done here, it should be fine since the BasicResourceTestCase explicitly tests all of those transformations, but I got the impression there is still pending work to do to enable the server to use the expected stability level when it is registering the transformers, that;s still a bit unclear to me
Comment on lines
+45
to
+50
| for (E model : EnumSet.complementOf(EnumSet.of(this.currentSubsystemModel))) { | ||
| ModelVersion version = model.getVersion(); | ||
| ResourceTransformationDescriptionBuilder builder = registration.createResourceTransformationDescriptionBuilder(); | ||
| this.transformation.accept(builder, version); | ||
| TransformationDescription.Tools.register(builder.build(), registration, version); | ||
| } |
Contributor
There was a problem hiding this comment.
Do we have an example of how to use this class?
I'm not a SME in these transformers and it is too big to digest it easily, but I guess what I cannot see is how this new way to register the transformers will be able to match what we currently have. At this moment, transformers are registered as a chain, with a common builder that registers the transformation from one version to another, however here we are only registering a single version with a specific builder, so I get lost on this.
Comment on lines
+293
to
296
| @Deprecated(forRemoval = true) | ||
| public static TransformerRegistry create() { | ||
| return new TransformerRegistry(); | ||
| return create(Stability.DEFAULT); | ||
| } |
Contributor
There was a problem hiding this comment.
What happens with the current usage of this method in our code? Shouldn't we at least force the creation of a transformer registry using a specific Stability level? Otherwise, we will continue using the wrong registry and we won't be able yet to add specific transformations to the subsystems.
Comment on lines
+69
to
+74
| ChainedTransformationDescriptionBuilderFactory factory = KernelAPIVersion.createChainedTransformationDescriptionBuilderFactory(registry); | ||
| registerChainedManagementTransformers(registry, factory); | ||
| registerChainedServerGroupTransformers(registry, factory); | ||
| registerProfileTransformers(registry); | ||
| registerSocketBindingGroupTransformers(registry); | ||
| registerDeploymentTransformers(registry); | ||
| registerSocketBindingGroupTransformers(registry, factory); | ||
| registerDeploymentTransformers(registry, factory); |
Contributor
There was a problem hiding this comment.
Why we are only changing the Domain Transformers? Shouldn't we also change and adapt in this PR how we are registering the other transformers?
For example, at ElytronSubsystemTransformers we have:
@Override
public void registerTransformers(SubsystemTransformerRegistration registration) {
ChainedTransformationDescriptionBuilder chainedBuilder = TransformationDescriptionBuilder.Factory.createChainedSubystemInstance(registration.getCurrentSubsystemVersion());
What would be the equivalent code to create ChainedTransformationDescriptionBuilder using the current Stability level?
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
Contributor
Author
|
Bump (to prevent github autoclose) |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
Contributor
Author
|
Bump (to prevent github autoclose). @bstansberry Are we still interested in this? If so, can you take a look? |
bstansberry
reviewed
May 22, 2025
Contributor
bstansberry
left a comment
bstansberry
left a comment
There was a problem hiding this comment.
@pferraro I had a look and made comments. I think Yeray's earlier are the more critical ones.
Re whether we are interested in this, TBH I'm relying on you and @yersan to tell me. I think we are because in all the discussions about it we keep working on it and I think we'd stop if we decided not to do it. ;) But the details of the back-and-forth are flushed from memory, partly because I'm relying on you guys.
| registry.addAttributeCheck(attribute, rejectChecker); | ||
| for (String rejectedAttribute : rejectedAttributes) { | ||
| this.registry.addAttributeCheck(rejectedAttribute, rejectChecker); | ||
| } |
Contributor
There was a problem hiding this comment.
Should this method be deprecated? Just reading the diff it seems like it's unable to apply the desired 'enables' check so it's just assuming. Which is ok temporarily but perhaps should be deprecated.
It seems like the javadoc in the interface should explain the behavior re the 'enables' call for all the methods.
Contributor
Author
There was a problem hiding this comment.
Thanks for that - I'll rebase this branch on that change next week.
| public T addRejectChecks(List<RejectAttributeChecker> rejectCheckers, String... rejectedAttributes) { | ||
| for (RejectAttributeChecker rejectChecker : rejectCheckers) { | ||
| addRejectCheck(rejectChecker, rejectedAttributes); | ||
| this.addRejectCheck(rejectChecker, rejectedAttributes); |
Contributor
There was a problem hiding this comment.
Same comment re deprecation.
| public T addRenames(Map<String, String> renames) { | ||
| for (Map.Entry<String, String> rename : renames.entrySet()) { | ||
| registry.addRenamedAttribute(rename.getKey(), rename.getValue()); | ||
| this.addRename(rename.getKey(), rename.getValue()); |
Contributor
There was a problem hiding this comment.
Same comment re deprecation.
| @@ -131,10 +132,6 @@ public T setValueConverter(AttributeConverter attributeConverter, String... conv | |||
| return thisBuilder(); | |||
Contributor
There was a problem hiding this comment.
Same comment re deprecation.
| * @deprecated Use {@link ResourceTransformationDescriptionBuilder#addChildResource(ResourceRegistration)} instead. | ||
| */ | ||
| ResourceTransformationDescriptionBuilder addChildResource(ResourceDefinition definition); | ||
| @Deprecated(forRemoval = true) |
Contributor
There was a problem hiding this comment.
Note to reviewers: I only see 5 uses of this in WF, all in the legacy Picketbox extensions.
| */ | ||
| ResourceTransformationDescriptionBuilder addChildResource(ResourceDefinition definition, DynamicDiscardPolicy dynamicDiscardPolicy); | ||
|
|
||
| @Deprecated(forRemoval = true) |
Contributor
There was a problem hiding this comment.
Note to reviewers: I don't see any uses of this in WF.
| * @return the builder for this resource | ||
| * @deprecated Use other methods for creating child transformation descriptions | ||
| */ | ||
| @Deprecated(forRemoval = true) |
Contributor
There was a problem hiding this comment.
Note to reviewers: I see no use in WF.
Contributor
This is something good to have, since it will allow us to test community-community paired stability levels in mixed domains. I don't see it as something urgent to do; I have never read issues with mixed domains on the community side, so, probably something that is not widely used. It brings up more maintenance, and the fix indeed looks complex. However, having those tests in practice is good and necessary. It will also help later to future promotions by moving a feature from community stability to a higher one. @pferraro I don't know how complex this would end up at the end, not only on the solution but also on the future maintenance, so if you still have doubts whether this is necessary, I would be fine because I don't think this scenario is widely used in upstream. |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
Contributor
Author
|
Bump to keep alive. |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
Contributor
Author
|
Bump to keep alive. |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
Contributor
|
/retest |
|
Core -> Full Integration (with SecurityManager) Build 15181 outcome was FAILURE using a merge of aa426d5 Failed tests |
|
Core -> Full Integration (with SecurityManager) Build 15273 outcome was FAILURE using a merge of aa426d5 Failed tests |
|
Core -> Full Integration Build 15004 outcome was FAILURE using a merge of aa426d5 Failed tests |
|
Core -> Full Integration (with SecurityManager) Build 13 outcome was FAILURE using a merge of aa426d5 Failed tests |
|
Core -> Full Integration (with SecurityManager) Build 80 outcome was FAILURE using a merge of aa426d5 Failed tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/WFCORE-6995
Submitting as a draft while I collect feedback and add tests.@yersan Until https://github.com/wildfly/wildfly-legacy-test publishes releases for WildFly versions, it will be impossible to create model transformation tests for COMMUNITY stability.