[WFCORE-6756] Subsystem model bump and transformer adjustments needed for the dynamic SSLContext support #5941
Conversation
This file contains 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
github-actions
bot
added
the
deps-ok
Skyllarr
force-pushed
the
WFCORE-6756
branch
2 times, most recently
from
28969af
to
5df09ac
Compare
|
Core -> WildFly Preview Integration Build 13507 outcome was FAILURE using a merge of 5df09ac Failed tests |
|
Core -> WildFly Preview Integration Build 13508 outcome was FAILURE using a merge of 5df09ac Failed tests |
|
Core -> Full Integration Build 13704 outcome was FAILURE using a merge of 5df09ac Failed tests |
Skyllarr
commented
Apr 8, 2024
| ELYTRON_8_0_0, ELYTRON_7_0_0, ELYTRON_6_0_0, ELYTRON_5_0_0, ELYTRON_4_0_0, ELYTRON_3_0_0, ELYTRON_2_0_0, ELYTRON_1_2_0 }); | ||
| } | ||
|
|
||
| private static void from19(ChainedTransformationDescriptionBuilder chainedBuilder) { |
There was a problem hiding this comment.
@pferraro Following from your comment here, I have added a new version 19 of the elytron subsystem, and added schemas for both the DEFAULT and the COMMUNITY stability for this version. I also added a transformer from the new version 19 to version 18, that you can see here.
I have ignored the stability levels when I added this transformer. And I did not add any transformers between different stability levels of the same version. Just wanted to double check that this is correct, please? Thank you!
There was a problem hiding this comment.
Since the transformer is rejection only, this should function correctly for either stability level (rejecting a child resource with no registration should just be no-op).
Additionally, stability level of the target host is not yet exposed via SubsystemTransformerRegistration - so this would not yet be possible anyway.
|
Core -> Full Integration Build 13710 outcome was FAILURE using a merge of 5df09ac Failed tests |
|
@fjuma Just a note that there is a failure in the org.wildfly.test.manual.management.ManagementOnlyModeTestCase.java that has this cause:
but it is not related to this PR and it is occuring in other PRs as well |
This is because the test ordering when the OIDC tests are executed before ManagementOnlyModeTestCase, the latter will fail |
|
I've just opened https://issues.redhat.com/browse/WFLY-19219 to track it |
This comment was marked as outdated.
This comment was marked as outdated.
Thank you @yersan ! |
pferraro
reviewed
Apr 9, 2024
Comment on lines
56
to
57
| VERSION_19_0(19), | ||
| VERSION_19_0_COMMUNITY(19, Stability.COMMUNITY), |
There was a problem hiding this comment.
WFCORE-6756 indicates the need to increment the subsystem model. Why, then, is the XML schema version changing?
There was a problem hiding this comment.
@pferraro we are always bumping the schema when incrementing the subsystem model, see for example previous bump jira https://issues.redhat.com/browse/WFCORE-6312 .
So I have moved the COMMUNITY resource dynamic-ssl-context from the community_18 schema to the new schema community_19, if that is okay for the Final release? Maybe that's too late now, let me know. @fjuma Do you think we should increment both the model and the XML schema?
There was a problem hiding this comment.
we are always bumping the schema when incrementing the subsystem model
That is not correct. Schema versions and model versions are not inherently related. A new version of a subsystem schema is only required if there are changes to the XML representation of the model. A new version of a subsystem model is only required when there are changes to the subsystem's resource description (read recursively). The XML representation can change without the model, just as the model can change without affecting the XML representation.
see for example previous bump jira https://issues.redhat.com/browse/WFCORE-6312
As a general note, it is not advisable to create JIRA issues for schema/model version bumps. I realize that the current situation is an exception, since the original change should have included a model version bump, IIUC.
Changes like WFCORE-6312 are not supposed to be merged independently from a schema and/or model change that requires it - otherwise this can easily result in redundant versions. While redundant model versions have no signficant cost (other than being a potential source of confusion), redundant schema versions require parser registration and are otherwise a waste of heap space. Tracking this creates a completely avoidable burden to the release coordinator, who would otherwise need to keep track of version bumps for every subsystem, and rollback changes if there were no associated subsystem changes. Consequently, version bumps to a subsystem model and/or schema are not appropriate as a standalone pull request.
So I have moved the COMMUNITY resource dynamic-ssl-context from the community_18 schema to the new schema community_19
Unless the XML representation of the subsystem's model has changed, there is reason to change this.
There was a problem hiding this comment.
@pferraro I see, thank you very much for explanation! That makes sense, I will remove the schema update from this PR
There was a problem hiding this comment.
On the general note I would say it is preferred that we do use a Jira issue for the model bump - where we have multiple engineers working on features in a subsystem independently we do want a coordinated approach so one does the work handling the bump(s) and then the individual features can depend on the common commits.
Yes we do need to ensure the bump only goes in as a feature goes in and generally this can happen when the first of any are merged. Working at different stability levels is going to confuse that a bit more now so we may need to think some more on the coordination.
There was a problem hiding this comment.
+1, I agree that it's good to create a Jira issue for the model bump. Typically, what we do is get a draft PR open with the changes needed so that others working on new features can make use of the common commits. We make sure that the PR is only merged once we have approved the PRs for at least one of the features that make use of it.
pferraro
reviewed
Apr 9, 2024
Comment on lines
196
to
197
| if (this.since(ElytronSubsystemSchema.VERSION_19_0_COMMUNITY) && this.enables(getDynamicClientSSLContextDefinition())) { | ||
| builder.addChild(tlsParser.tlsParserCommunity_19_0); |
There was a problem hiding this comment.
@pferraro we are naming these parsers based on the subsystem schema it was introduced in, and I moved this resource to version 19 before the Final tag
There was a problem hiding this comment.
@pferraro So you think I should just bump the model version and don't touch the schema 18 at this point, right?
There was a problem hiding this comment.
@pferraro So you think I should just bump the model version and don't touch the schema 18 at this point, right?
Right.
pferraro
reviewed
Apr 10, 2024
| @@ -75,8 +75,9 @@ public class ElytronExtension implements Extension { | |||
| static final ModelVersion ELYTRON_16_0_0 = ModelVersion.create(16); | |||
| static final ModelVersion ELYTRON_17_0_0 = ModelVersion.create(17); | |||
| static final ModelVersion ELYTRON_18_0_0 = ModelVersion.create(18); | |||
| static final ModelVersion ELYTRON_19_0_0 = ModelVersion.create(19); | |||
There was a problem hiding this comment.
OK - it looks like ModelVersion 18.0.0 was associated with WF 29-31, meaning that we need a new ModelVersion 19.0.0 to identify the model changes for WF32 and later, correct?
(I'm trying to make sure I understand the context of this change)
There was a problem hiding this comment.
@pferraro Yes that's correct. The context of this change is just to fix the fact that I did not do the subsystem model bump when I added a resource to the community stability level previously.
… for the dynamic SSLContext support
|
@pferraro I removed the schema bump, thank you! |
darranl
approved these changes
Apr 12, 2024
fjuma
approved these changes
Apr 12, 2024
|
This seems to be ready to go, @pferraro do you have any additional requests? otherwise, I'll mark it ready-for-merge |
pferraro
approved these changes
Apr 15, 2024
yersan
added
the
ready-for-merge
|
Thanks @Skyllarr and all the reviewers! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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-6756