Enable asynchronous deprovision in TSB #16815
Conversation
openshift-ci-robot
added
the
size/XL
| if err != nil && !kerrors.IsNotFound(err) { | ||
| return api.InternalServerError(err) | ||
| } | ||
|
|
||
| // The OSB API requires this function to be idempotent (restartable). If | ||
| // any actual change was made, per the spec, StatusOK is returned, else |
There was a problem hiding this comment.
comment needs to be updated.
| @@ -259,25 +280,78 @@ var _ = g.Describe("[Conformance][templates] templateservicebroker end-to-end te | |||
|
|
|||
| deprovision := func() { | |||
| g.By("deprovisioning a service") | |||
| err := brokercli.Deprovision(context.Background(), cliUser, instanceID) | |||
| err := cli.TemplateClient().Template().Templates(cli.Namespace()).Delete(privatetemplate.Name, &metav1.DeleteOptions{}) | |||
| o.Expect(err).NotTo(o.HaveOccurred()) | |||
There was a problem hiding this comment.
what's the point of deleting the template here?
There was a problem hiding this comment.
What I've implemented is that the test explicitly checks that the namespace is left empty (inasmuch as this is possible). We manually put that template in the namespace earlier (in the BeforeEach), therefore we must manually remove it for the test to succeed.
| err := cli.TemplateClient().Template().Templates(cli.Namespace()).Delete(privatetemplate.Name, &metav1.DeleteOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| err = brokercli.Deprovision(context.Background(), cliUser, instanceID) |
There was a problem hiding this comment.
doesn't this need to wait until lastoperation returns success? (should have a test for that anyway)
There was a problem hiding this comment.
brokercli (already) handles that internally. What additional test are you looking for?
There was a problem hiding this comment.
ok, I didn't dig into brokercli explicitly. I see now that it already handles both sync and async responses (I was assuming it only handled sync and would need changes to add support for async)
jim-minter
force-pushed
the
tsb-async-delete
branch
from
a240971 to
1ea5bad
Compare
|
i think this is lgtm but will revisit after #16808 merges, i don't want to merge this beforehand since it's got a cherrypicked commit. |
(and ensure that all objects are deleted once the TSB says that deprovision is finished)
jim-minter
force-pushed
the
tsb-async-delete
branch
from
1ea5bad to
90f5bba
Compare
openshift-ci-robot
added
size/L
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, jim-minter The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-merge-robot
added
the
approved
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Automatic merge from submit-queue. |
No description provided.