Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[IA-4175] delete disk wsm dao removal #4752

Merged
merged 6 commits into from
Aug 27, 2024
Merged

[IA-4175] delete disk wsm dao removal #4752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d7895e
delete disk done
Aug 19, 2024
2ff709d
compiling but unlikely tests work with current mocks
Aug 20, 2024
7d64181
test fixes
Aug 20, 2024
1225d94
final test fixes
Aug 20, 2024
ae75d76
cleanup
Aug 20, 2024
630f026
feedback
Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 8 additions & 69 deletions http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/dao/HttpWsmDao.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,12 @@ import org.broadinstitute.dsde.workbench.leonardo.dao.LandingZoneResourcePurpose
WORKSPACE_BATCH_SUBNET
}
import org.broadinstitute.dsde.workbench.leonardo.dao.WsmDecoders._
import org.broadinstitute.dsde.workbench.leonardo.dao.WsmEncoders._
import org.broadinstitute.dsde.workbench.leonardo.db.WsmResourceType
import org.broadinstitute.dsde.workbench.leonardo.util.AppCreationException
import org.broadinstitute.dsde.workbench.model.TraceId
import org.broadinstitute.dsde.workbench.openTelemetry.OpenTelemetryMetrics
import org.http4s._
import org.http4s.circe.CirceEntityDecoder._
import org.http4s.circe.CirceEntityEncoder._
import org.http4s.client.Client
import org.http4s.client.dsl.Http4sClientDsl
import org.http4s.headers.{`Content-Type`, Authorization}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to document this file with a Header explaining that this is a legacy DAO that should not be modified other than eventually removing the Landing Zone Logic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not seeing a commit with these headers / comments. Are these coming as part of another PR? It could be part of this PR honestly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added them to the interface WsmDao instead of this concrete implementation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry could you point me to it? I can't find it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops apparently I needed to force-push 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha no worries, I see it now, looks good 👍

Expand All @@ -31,6 +29,10 @@ import org.typelevel.log4cats.StructuredLogger

import java.util.UUID

/**
* This is the legacy WsmDAO. It remains because there is some specific logic around models retrieved from WSM
* It SHOULD NOT be added to. Favor usage of WsmClientProvider, the auto-generated client.
*/
class HttpWsmDao[F[_]](httpClient: Client[F], config: HttpWsmDaoConfig)(implicit
logger: StructuredLogger[F],
F: Async[F],
Expand All @@ -40,6 +42,8 @@ class HttpWsmDao[F[_]](httpClient: Client[F], config: HttpWsmDaoConfig)(implicit

val defaultMediaType = `Content-Type`(MediaType.application.json)

// This remains in the legacy dao because of the custom logic around landing zones
// We should migrate to the generated client when possible
override def getLandingZoneResources(billingProfileId: BillingProfileId, userToken: Authorization)(implicit
ev: Ask[F, AppContext]
): F[LandingZoneResources] =
Expand Down Expand Up @@ -252,52 +256,8 @@ class HttpWsmDao[F[_]](httpClient: Client[F], config: HttpWsmDaoConfig)(implicit
)(onError)
} yield resOpt.fold(List.empty[LandingZoneResourcesByPurpose])(res => res.resources)

override def deleteDisk(request: DeleteWsmResourceRequest, authorization: Authorization)(implicit
ev: Ask[F, AppContext]
): F[Option[DeleteWsmResourceResult]] =
deleteHelper(request, authorization, "disks")

// TODO: Next up for removal, IA-4175
override def getCreateVmJobResult(request: GetJobResultRequest, authorization: Authorization)(implicit
ev: Ask[F, AppContext]
): F[GetCreateVmJobResult] =
for {
ctx <- ev.ask
res <- httpClient.expectOr[GetCreateVmJobResult](
Request[F](
method = Method.GET,
uri = config.uri
.withPath(
Uri.Path
.unsafeFromString(
s"/api/workspaces/v1/${request.workspaceId.value.toString}/resources/controlled/azure/vm/create-result/${request.jobId.value}"
)
),
headers = headers(authorization, ctx.traceId, false)
)
)(onError)
} yield res

override def getDeleteDiskJobResult(request: GetJobResultRequest, authorization: Authorization)(implicit
ev: Ask[F, AppContext]
): F[GetDeleteJobResult] =
for {
ctx <- ev.ask
res <- httpClient.expectOr[GetDeleteJobResult](
Request[F](
method = Method.GET,
uri = config.uri
.withPath(
Uri.Path
.unsafeFromString(
s"/api/workspaces/v1/${request.workspaceId.value.toString}/resources/controlled/azure/disks/delete-result/${request.jobId.value}"
)
),
headers = headers(authorization, ctx.traceId, false)
)
)(onError)
} yield res

// This remains in the legacy dao because of the custom logic around storage containers
// We should migrate to the generated client when possible
override def getWorkspaceStorageContainer(workspaceId: WorkspaceId, authorization: Authorization)(implicit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be removed as well? My understanding was that only the Landing Zone related calls would remain in this legacy DAO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This some pretty specific business logic we do here that dependent on our current decoding I'd rather not redo. I'll comment both.

ev: Ask[F, AppContext]
): F[Option[StorageContainerResponse]] = for {
Expand Down Expand Up @@ -345,27 +305,6 @@ class HttpWsmDao[F[_]](httpClient: Client[F], config: HttpWsmDaoConfig)(implicit
)(onError)
} yield resp

private def deleteHelper(req: DeleteWsmResourceRequest, authorization: Authorization, resource: String)(implicit
ev: Ask[F, AppContext]
): F[Option[DeleteWsmResourceResult]] =
for {
ctx <- ev.ask
res <- httpClient.expectOptionOr[DeleteWsmResourceResult](
Request[F](
method = Method.POST,
uri = config.uri
.withPath(
Uri.Path
.unsafeFromString(
s"/api/workspaces/v1/${req.workspaceId.value.toString}/resources/controlled/azure/${resource}/${req.resourceId.value.toString}"
)
),
entity = req.deleteRequest,
headers = headers(authorization, ctx.traceId, true)
)
)(onError)
} yield res

private def onError(response: Response[F])(implicit ev: Ask[F, AppContext]): F[Throwable] =
for {
context <- ev.ask
Expand Down
Loading
Loading