Fix last op endpoint unbind response #765
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
openshift-bot
added
needs-rebase
pkg/broker/broker.go
Outdated
| return nil, false, err | ||
| err = cleanupUnbind(&bindInstance, &serviceInstance, a.dao, true) | ||
| } else { | ||
| err = cleanupUnbind(&bindInstance, &serviceInstance, a.dao, false) |
There was a problem hiding this comment.
I think that we should remove the if else and just call cleanupUnbind and pass in the bind creds to cleanupUnbind and do the nil check in that method. I think that might be more clear what that is doing.
There was a problem hiding this comment.
Yeah sounds good to me.
shawn-hurley
added
needs-review
3.10 | release-1.2
shawn-hurley
changed the title
pkg/broker/broker.go
Outdated
| } | ||
| if jobInProgress { | ||
| log.Infof("Unbind requested for instance %s, but job is already in progress", instance.ID) | ||
| return &UnbindResponse{Operation: jobToken}, false, ErrorDeprovisionInProgress |
There was a problem hiding this comment.
If I'm reading it correctly, I think the handler needs to be augmented to understand this error and make sure it results in the correct response.
There's also a potentially difficult question to answer, which is: we know there is a job running, but is it executing asynchronously? If yes, the response should be a 202. If not, I don't think the spec is very clear. Maybe the best response in that case would be 400. Here is what the proposed spec changes say:
There was a problem hiding this comment.
Good point. Ive added logic to the handler to check the error but I think returing 202 regardless of async or not is appropriate since the job in running and it will force the client to poll last op endpoint for the result. This meets the spec 202 MUST be returned if the unbinding is in progress. which in this case it is.
This logic is also used for the deprovision handler
|
Is this considered complete and ready for review / merge? The PR message still states it's not ready, I might have missed something though. |
|
@eriknelson ready for review yes, dont think its ready for merge yet |
Ready for review
Changes proposed in this pull request
Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #709