-
Notifications
You must be signed in to change notification settings - Fork 84
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
Bug 1502044 - deprovision fixes #494
Conversation
Testing:
ResultsBefore fix (broker):
After fix (broker):
After fix (service-catalog):
|
executionContext, err := ExecuteApb( | ||
"deprovision", clusterConfig, instance.Spec, | ||
instance.Context, instance.Parameters, log, | ||
) | ||
defer sm.DestroyApbSandbox(executionContext, clusterConfig) |
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.
How do we handle the error, if DestroyApbSandbox
has an error? We can make sure that the log of the error happens in the Destroy function, but maybe make it return nothing to make it more clear how to use it?
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.
Unless you want to wrap it in another call just for this area.
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.
defer func(context ExecutionContenxt config ClusterConfig) {
err := sm.DestoryApbSandbox(execution, config)
if err != nil {
log.Errorf("Error occurred destroying sandbox during deprovision: \n%s", err)
}
}(executionContext, clusterConfig)
Would this suffice? Should we be taking any additional action in the presence of an error here aside from loudly logging?
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.
I updated to the DestroyApbSandbox
to simply log loudly when it fails and return nothing. Let me know if I should go with this method instead.
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.
VISIACK
executionContext, err := ExecuteApb( | ||
"deprovision", clusterConfig, instance.Spec, | ||
instance.Context, instance.Parameters, log, | ||
) | ||
defer sm.DestroyApbSandbox(executionContext, clusterConfig) |
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.
Unless you want to wrap it in another call just for this area.
465dd5f
to
a96804a
Compare
Since I added changes for bind/unbind, I'm going to manually test to verify those are still working as expected. Done. ✅ |
a96804a
to
e85fe13
Compare
* Bug 1502044 - return 202 on async deprovision * Defer sandbox teardown to prevent leaks * Destroy APB Sandbox should not return anything * Improve apb error logging
Describe what this PR does and why we need it:
This PR is meant to address a few errors with respect to service catalog requests to provision/deprovision.