-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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-3978] - PicketLink EAP 6.4 Issues #6951
Conversation
Linux Build 5471 is now running using a merge of 56646ec |
Linux Build 5471 outcome was SUCCESS using a merge of 56646ec |
|
||
@Override | ||
protected void revertUpdateToRuntime(OperationContext context, ModelNode operation, String attributeName, ModelNode valueToRestore, ModelNode valueToRevert, Object handback) throws OperationFailedException { | ||
|
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.
Should you not unset the configuration here or reset it to the configuration resulting from the current model (following rollback, i.e. the same as before the operation failed)?
My comments regarding rollback and why this is now necessary, is that in most cases before these were installed by services. The service verification handlers would have taken care of uninstalling the services, and so undoing the config changes, on rollback. Now that needs to happen manually. |
Linux Build 5510 is now running using a merge of c4db92f |
I've updated the handlers and resources with the rollback/revert code. |
Linux Build 5510 outcome was SUCCESS using a merge of 858f955 |
Linux Build 5511 is now running using a merge of 858f955 |
Linux Build 5511 outcome was SUCCESS using a merge of 858f955 |
protected void revertUpdateToRuntime(OperationContext context, ModelNode operation, String attributeName, ModelNode valueToRestore, ModelNode valueToRevert, Object handback) throws OperationFailedException { | ||
ModelNode restored = context.readResource(PathAddress.EMPTY_ADDRESS).getModel().clone(); | ||
|
||
restored.get(attributeName).set(valueToRestore); |
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.
updateConfiguration seems to use the operation's address for the alias, which is contained by the write-attribute operation used in applyUpdateToRuntime(). Here you are passing in a 'restored' which has no address field so it seems the functionality will be different?
} | ||
|
||
private void updateConfiguration(OperationContext context, ModelNode operation) throws OperationFailedException { | ||
PathAddress pathAddress = PathAddress.pathAddress(operation.get(ModelDescriptionConstants.ADDRESS)); |
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.
Nitpick, when reading the operation address the standard constant to use is OP_ADDR.
I might have missed it but I don't see the add handlers handling the rollback case: |
For the add handler case, and what you have correctly done for the remove handler case the 'model' parameter passed in to rollbackRuntime()/recoverServices() is the original model before any changes were made. So the remove handlers look ok, and the add handler rollback can use the same mechanism. |
|
||
if (serviceController != null) { | ||
IdentityProviderService service = serviceController.getValue(); | ||
ModelNode identityProviderNode = context.readResource(EMPTY_ADDRESS, false).getModel(); |
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.
In the rollback case I don't think this is the actual rolled back model. Normally for a write attribute handler we only deal with the one attribute and use the value passed in, but since you seem to be using a few other attributes from the model, I don't think that will work when doing a rollback. So something like:
final ModelNode identityProviderNode;
if (!rollback) {
identityProviderNode = context.readResource(EMPTY_ADDRESS, false).getModel();
} else {
Resource rc = ctx.getOriginalRootResource().navigate(pathAddress);
identityProviderNode = rc.getModel();
}
Linux Build 5516 is now running using a merge of 2c1ad0b |
PathAddress pathAddress = PathAddress.pathAddress(operation.get(ModelDescriptionConstants.OP_ADDR)); | ||
ModelNode restored = context.readResource(PathAddress.EMPTY_ADDRESS).getModel().clone(); | ||
|
||
restored.get(attributeName).set(valueToRestore); |
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.
restored is no longer used
Linux Build 5516 outcome was SUCCESS using a merge of dfb3bd6 |
Linux Build 5518 is now running using a merge of dfb3bd6 |
Linux Build 5518 outcome was SUCCESS using a merge of dfb3bd6 |
[WFLY-3978] - PicketLink EAP 6.4 Issues
Improving usability, removing unnecessary services and reload state.