WOR-1403: add error handling for missing billing profile and for missing landing zone #2669
Conversation
| @@ -864,7 +862,29 @@ class BpmBillingProjectLifecycleSpec extends AnyFlatSpec { | |||
| mock[WorkspaceManagerResourceMonitorRecordDao] | |||
| ) | |||
|
|
|||
| Await.result(bp.initiateDelete(billingProjectName, testContext), Duration.Inf) | |||
| Await.result(bp.initiateDelete(billingProjectName, testContext), Duration.Inf) shouldBe (Some(jobReportId)) | |||
There was a problem hiding this comment.
The result of this is more meaningful now, so we really should verify it.
| @@ -214,7 +216,10 @@ class BpmBillingProjectLifecycle( | |||
| private def cleanupLandingZone( | |||
| landingZoneId: UUID, | |||
| ctx: RawlsRequestContext | |||
| ): DeleteAzureLandingZoneResult = Try(workspaceManagerDAO.deleteLandingZone(landingZoneId, ctx)) match { | |||
| ): Option[DeleteAzureLandingZoneResult] = Try(workspaceManagerDAO.deleteLandingZone(landingZoneId, ctx)) match { | |||
There was a problem hiding this comment.
Made the return an optional value, so we could indicate a non-failure instance where we don't need to create the monitor job to wait for the result.
This matches up nicely with the result and return behavior of the initializeDelete method, and doesn't conflict with the usage in cleanup when creation fails.
| @@ -214,7 +216,10 @@ class BpmBillingProjectLifecycle( | |||
| private def cleanupLandingZone( | |||
| landingZoneId: UUID, | |||
| ctx: RawlsRequestContext | |||
| ): DeleteAzureLandingZoneResult = Try(workspaceManagerDAO.deleteLandingZone(landingZoneId, ctx)) match { | |||
| ): Option[DeleteAzureLandingZoneResult] = Try(workspaceManagerDAO.deleteLandingZone(landingZoneId, ctx)) match { | |||
| case Failure(e: ApiException) if e.getCode == HttpStatus.SC_FORBIDDEN => | |||
There was a problem hiding this comment.
The service returns a 403 instead of a 404 when there's no landing zone present.
It's fine to move on, because if this was a 403 for permission reasons, we won't be able to delete the billing profile anyway, and we don't have a valid instance of a user having access to the billing project but not the underlying resources.
There was a problem hiding this comment.
It would be good to have this comment in the code.
There was a problem hiding this comment.
And if we DO happen to get a 404 from the service (which seems more correct), I think we should also handle it and allow the deletion to proceed. Perhaps at some point the 403 will be corrected to a 404.
blakery
requested review from
a team,
marctalbott,
aherbst-broad and
cahrens
and removed request for
a team
| // The service returns a 403 instead of a 404 when there's no landing zone present. | ||
| // It's fine to move on here, because if this was a 403 for permission reasons, we won't be able to delete the billing profile anyway, | ||
| // and we don't have a valid instance of a user having access to the billing project but not the underlying resources. | ||
| case Failure(e: ApiException) if e.getCode == HttpStatus.SC_FORBIDDEN || e.getCode == HttpStatus.SC_NOT_FOUND => |
There was a problem hiding this comment.
I've started pushing this sort of logic down into the DAO layer in the similar BPM error handling code. The motivation is to keep the HTTP level concerns in the DAO (i.e., StatusCodes.NOT_FOUND etc.) and just return Option[foo] and make the upper levels deal with that interface. What do you think about doing that here?
There was a problem hiding this comment.
I think that is a better method in the long term, with how we're looking to structure our code, and separate our testing concerns.
I'll give it a try, and see what kind of issues it creates in the immediate term.
Ticket: https://broadworkbench.atlassian.net/browse/WOR-1403
Continue deletion if either the landing zone or billing profile is missing.
PR checklist
model/, then you should publish a new officialrawls-modeland updaterawls-modelin Orchestration's dependencies.