-
Notifications
You must be signed in to change notification settings - Fork 465
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
[WFCORE-6503]:Add support for unmanaged deployments with YAML extension. #5860
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
826b453
to
18a3f13
Compare
For WARN message it seems to be logging some null values. For example:
for yaml:
I should log the name of yaml tag which is ignored. |
d00ce87
to
3d5976c
Compare
controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java
Outdated
Show resolved
Hide resolved
cf04996
to
fd8e69e
Compare
c6dcfa8
to
b3b2923
Compare
This comment was marked as outdated.
This comment was marked as outdated.
controller/src/main/java/org/jboss/as/controller/logging/ControllerLogger.java
Outdated
Show resolved
Hide resolved
Hello @bstansberry, you added "Feature" and "missing-reqs" labels to this PR. So far we are treating this as an enhancement, do you want this to be treated as a Feature Request instead? There is more information about this on the Jira. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Added some comments to think about whether some stuff that could be moved to simply a bit the changes on the reload handlers and ModelControllerImpl, they can be evolved later, but it took me some time to figure out why we needed them.
In general, it looks pretty isolated to yaml work, so, except the additional trailing white space on the --yaml option, looks good and I think we can go with them. However, if possible, we should move the conditions added on reload handlers and ModelControllerImpl to the ConfigurationExtension.shouldProcessOperations.
controller/src/main/java/org/jboss/as/controller/persistence/ConfigurationPersister.java
Outdated
Show resolved
Hide resolved
testsuite/test-runner/src/main/java/org/wildfly/core/testrunner/Server.java
Outdated
Show resolved
Hide resolved
...test/manualmode/management/persistence/yaml/test-remove-attribute-removes-above-resource.yml
Outdated
Show resolved
Hide resolved
...oller/src/main/java/org/jboss/as/controller/persistence/yaml/YamlConfigurationExtension.java
Outdated
Show resolved
Hide resolved
controller/src/main/java/org/jboss/as/controller/ModelControllerImpl.java
Outdated
Show resolved
Hide resolved
controller/src/main/java/org/jboss/as/controller/RunningModeControl.java
Outdated
Show resolved
Hide resolved
@bstansberry @yersan I've rebased on #5934 so only the feature part where unmanaged deployments are supported is here now. |
@bstansberry I removed the feature label due to the https://issues.redhat.com/browse/WFCORE-6503 comments. That probably was a mistake since you added them after the Jira discussions. Just to be clear, do you want to treat this as a pure feature request instead of an enhancement that adds more value to a released feature request? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
/retest |
This comment was marked as outdated.
This comment was marked as outdated.
OperationEntry operationEntry = rootRegistration.getOperationEntry(PathAddress.pathAddress("deployment", name), ADD); | ||
if (deployment.getValue() != null && deployment.getValue() instanceof Map) { | ||
Map<String, Object> attributes = (Map<String, Object>) deployment.getValue(); | ||
Map<String, Object> content = (Map<String, Object>) (((Iterable<? extends Object>) attributes.get("content")).iterator().next()); |
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.
This can NPE if there is no 'content' and fail in other confusing ways if it's not a list or an empty list.
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.
Fixing that as it is only for validation purpose
...oller/src/main/java/org/jboss/as/controller/persistence/yaml/YamlConfigurationExtension.java
Outdated
Show resolved
Hide resolved
...oller/src/main/java/org/jboss/as/controller/persistence/yaml/YamlConfigurationExtension.java
Outdated
Show resolved
Hide resolved
...oller/src/main/java/org/jboss/as/controller/persistence/yaml/YamlConfigurationExtension.java
Outdated
Show resolved
Hide resolved
Path[] supplementalConfigurationFiles = findSupplementalConfigurationFiles(null, supplementalConfiguration); | ||
ConfigurationExtension configurationExtension = ConfigurationExtensionFactory.createConfigurationExtension(supplementalConfigurationFiles); | ||
if (configurationExtension != null) { | ||
configInteractionPolicy = configurationExtension.shouldProcessOperations(runningModeControl) ? ConfigurationFile.InteractionPolicy.READ_ONLY : configInteractionPolicy; |
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.
Note to reviewers:
The configInteractionPolicy var is never read in this branch, making the assignment here pointless and thus the other removed lines pointless as well.
There are uses of the var in other branches.
@yersan Sorry for not answering your questions until now. This is new user controllable behavior so it's a feature. We should communicate that to WF users by using Feature Request issue type. It's been validated as part of validating the earlier YAML work, so once code review stuff comments here are addressed by @ehsavoie this can be merged. |
…ion. * checking that the YAML deployment is unmanaged. * adding the unmanaged deployment to the list of operations * adding some light testing on this * fixing issue WFCORE-6857 where you couldn't enable an unmanaged deployment with yaml Jira: https://issues.redhat.com/browse/WFCORE-6503 https://issues.redhat.com/browse/WFCORE-6857 Proposal: wildfly/wildfly-proposals#554 Signed-off-by: Emmanuel Hugonnet <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
@yersan This LGTM.
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.
Thanks for helping with the review @bstansberry .
The points I reviewed were finally moved to a different issue and were already merged. I've checked it again and I have no more questions.
There is still pending paperwork on the RFE Jira that I guess, still prevents merging this.
RFE pending paperwork was resolved, so merging this |
Thanks @ehsavoie @mnovak1 @bstansberry |
Jira: https://issues.redhat.com/browse/WFCORE-6503
https://issues.redhat.com/browse/WFLY-19442
Proposal: wildfly/wildfly-proposals#554
Documenation PR: wildfly/wildfly#17970