WFCORE-4041: supply default values for nested attributes #4853
+73
−38
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
|
Core - Full Integration Build 11130 outcome was FAILURE using a merge of 69a375f Failed tests |
|
Ok, so |
michpetrov
force-pushed
the
wfcore-4041
branch
from
69a375f
to
244ab3b
Compare
|
Core - Full Integration Build 11133 outcome was FAILURE using a merge of 244ab3b Failed tests |
|
/retest |
|
Core - Full Integration Build 11173 outcome was FAILURE using a merge of 244ab3b Failed tests |
|
Core - Full Integration Build 11175 outcome was FAILURE using a merge of 244ab3b Failed tests |
|
@bstansberry it turns out the change has a greater impact, these tests fail because of how some Elytron attribute definitions are set, it's easy to fix but there more tests failing down the line where the models don't align with expected values |
michpetrov
force-pushed
the
wfcore-4041
branch
from
244ab3b
to
4c5cc0b
Compare
|
Core - Full Integration Build 11196 outcome was FAILURE using a merge of 4c5cc0b Failed tests |
|
/retest ? |
darranl
reviewed
Nov 29, 2021
| @@ -113,7 +113,6 @@ | |||
|
|
|||
| static final SimpleAttributeDefinition RECURSIVE_SEARCH = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.SEARCH_RECURSIVE, ModelType.BOOLEAN, true) | |||
| .setRequires(ElytronDescriptionConstants.FILTER) | |||
| .setDefaultValue(ModelNode.TRUE) | |||
There was a problem hiding this comment.
What is the reason for making this change? By having the default defined which also matches our XML schema it means we can avoid complicating it with the isDefined check.
There was a problem hiding this comment.
If an attribute A has a default value and requires attribute B then B has to have a default value as well, otherwise it will fail validation. Top-level attributes are being checked for this but I didn't implement it correctly for nested attributes.
If there is an available default value for FILTER and TO I can change that.
There was a problem hiding this comment.
@darranl how do we proceed with this?
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
|
Core - Full Integration Build 11359 outcome was FAILURE using a merge of 4c5cc0b Failed tests |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
|
Core - Full Integration Build 11389 outcome was FAILURE using a merge of b6ec232 |
|
Core - Full Integration Build 11521 outcome was FAILURE using a merge of b6ec232 |
|
Core - Full Integration Build 11298 outcome was FAILURE using a merge of b6ec232 |
|
Core - Full Integration Build 11431 outcome was FAILURE using a merge of b6ec232 |
|
Core - Full Integration Build 11449 outcome was FAILURE using a merge of 117acc3 |
|
@ehsavoie I have deleted the default value from |
|
Core - Full Integration Build 11312 outcome was UNKNOWN using a merge of 117acc3 |
|
Core - Full Integration Build 11324 outcome was UNKNOWN using a merge of 117acc3 |
|
Core -> Full Integration Build 12624 outcome was FAILURE using a merge of d9e7d32 Failed tests |
|
Core -> Full Integration Build 12634 outcome was FAILURE using a merge of d9e7d32 |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
|
There has been no activity on this PR for 90 days and it has been closed automatically. |
michpetrov
force-pushed
the
wfcore-4041
branch
from
0b6b5d6
to
d8207c7
Compare
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
michpetrov
force-pushed
the
wfcore-4041
branch
from
d8207c7
to
100215f
Compare
|
Core -> WildFly Preview Integration Build 13291 outcome was FAILURE using a merge of 100215f Failed tests |
|
@darranl this has been open for a while, what can I do to expedite it? |
|
/retest |
darranl
reviewed
Mar 8, 2024
| @@ -27,7 +27,7 @@ | |||
| <!-- Remote domain controller configuration with a host and port --> | |||
| <remote authentication-context="secondaryHostAContext" ignore-unused-configuration="true" admin-only-policy="${jboss.test.admin-only-policy}"> | |||
| <discovery-options> | |||
| <static-discovery name="primary" host="${jboss.test.host.primary.address}" port="9999"/> | |||
| <static-discovery name="primary" protocol="remote" host="${jboss.test.host.primary.address}" port="9999"/> | |||
There was a problem hiding this comment.
These additions look problematic, if we need to make configuration changes in test cases as a result of work in the management model this indicates that these changes will potentially break existing user configurations that don't have these attributes defined.
|
I think this is still going to need some discussion regarding implications for backwards compatibility, IMO preserving backwards compatibility feels like a higher priority than the original issue. I don't know if this is correct but one strategy might be to use model bumps and update the older parsers to compensate for the attribute with the default not being present. If we remove defaults we may need to change the schemas as well. Then transformers likely are impacted as well. Another option to consider is relaxing the constraints that means defaults could not be used on these attributes, that may reduce the backwards compatibility impact but then would need review of how the attributes are actually used. |
|
@darranl I'm gonna doublecheck but I don't think this actually causes compatibility issues. I should also mention that there are two larger issues, which I figure we should be aware of:
Now, for Elytron the default values are present in the underlying classes, so whether Regarding the config files - per current schema wildfly-core/server/src/main/resources/schema/wildfly-config_20_0.xsd Lines 1443 to 1447 in f4221cf the absence of protocol doesn't mean we're expecting a default value to be used (the attribute has no default value defined in the schema).
|
|
There has been no activity on this PR for 45 days. It will be auto-closed after 90 days. |
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.
Issue: WFCORE-4041
Resubmitting because I cannot reopen the original PR.