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.

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

WFLY-14544 Review for expansion expression testing in singleton subsystem #14100

Merged
merged 2 commits into from Mar 16, 2021

Conversation

rhusar
Copy link
Member

@rhusar rhusar commented Mar 9, 2021

@rhusar rhusar marked this pull request as draft March 9, 2021 22:30
@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Mar 9, 2021
@rhusar
Copy link
Member Author

rhusar commented Mar 10, 2021

@pferraro Please review.

@rhusar rhusar marked this pull request as ready for review March 10, 2021 14:56
@@ -81,6 +81,7 @@ static PathElement pathElement(String value) {

Attribute(String name, String alternative, CapabilityReferenceRecorder reference) {
this.definition = createBuilder(name, alternative)
.setAllowExpression(false)
Copy link
Member Author

@rhusar rhusar Mar 10, 2021

Choose a reason for hiding this comment

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

@bstansberry Can you confirm that disallowing expressions where previously inadvertently enabled (since these are capability references) does not require model version bump (given there is nothing to transform)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhusar I think it's ok here to not bump.

The AD stuff can be a bit tricky because it both describes the API, and thus is similar to javadoc, but it also controls behavior in some cases, e.g. setAllowExpression ends up driving validation of requests.

If an AD change basically just amounts to a 'javadoc correction' it doesn't require a bump. If it actually changes behavior, it requires a bump.

In this case the attribute has been a capability reference back to 2015 so presumably an expression would not have worked for a long time. So no behavior change and this is just a 'javadoc correction'.

@pferraro pferraro merged commit 361b79d into wildfly:master Mar 16, 2021
@rhusar rhusar deleted the WFLY-14544 branch March 16, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
3 participants