-
Notifications
You must be signed in to change notification settings - Fork 4
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
Rawls attempts to fetch "rawls" workspaces from WSM in addition to "MC" #2825
Conversation
core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala
Show resolved
Hide resolved
// some GCP workspaces, like those with linked snapshots, have a stub WSM workspace | ||
val wsmContext = | ||
try | ||
wsmService.fetchAggregatedWorkspace(workspaceContext, ctx) |
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.
do we need a test for this specific call or does that exist somewhere else already? (I'd assume so but wanted to check)
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.
im still trying to figure that out
core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala
Show resolved
Hide resolved
) | ||
|
||
Await.result( | ||
services.mcWorkspaceService.createMultiCloudWorkspace(workspaceRequest, new ProfileModel().id(UUID.randomUUID())), |
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 think this is the right way to go about it. From what I saw, looks like I either have to create a workspace this way or call services.workspaceService.createWorkspace
, otherwise calling both throws a "workspace' already exists" error.
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.
👍, with optional syntax suggestions - thank you!
val wsmContext = | ||
try | ||
wsmService.fetchAggregatedWorkspace(workspaceContext, ctx) | ||
catch { | ||
case e: AggregateWorkspaceNotFoundException => | ||
// return workspace with no WSM information for gcp workspace | ||
if (workspaceContext.workspaceType == WorkspaceType.RawlsWorkspace) { | ||
AggregatedWorkspace(workspaceContext, | ||
Some(workspaceContext.googleProjectId), | ||
azureCloudContext = None, | ||
policies = List.empty | ||
) | ||
} else { | ||
// bubble up an MC workspace exception | ||
throw e | ||
} | ||
} |
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.
This code is correct and in good shape, and I would not block the PR on changes here. However, the use of try/catch
is Java-idiomatic, where the use of Try/Success/Failure
is Scala-idiomatic. To be a good Scala citizen, you'd have something like this:
val wsmContext = | |
try | |
wsmService.fetchAggregatedWorkspace(workspaceContext, ctx) | |
catch { | |
case e: AggregateWorkspaceNotFoundException => | |
// return workspace with no WSM information for gcp workspace | |
if (workspaceContext.workspaceType == WorkspaceType.RawlsWorkspace) { | |
AggregatedWorkspace(workspaceContext, | |
Some(workspaceContext.googleProjectId), | |
azureCloudContext = None, | |
policies = List.empty | |
) | |
} else { | |
// bubble up an MC workspace exception | |
throw e | |
} | |
} | |
val wsmContext = | |
Try(wsmService.fetchAggregatedWorkspace(workspaceContext, ctx)) match { | |
case Success(found) => found | |
case Failure(notFound: AggregateWorkspaceNotFoundException) | |
if workspaceContext.workspaceType == WorkspaceType.RawlsWorkspace => | |
// return workspace with no WSM information for gcp workspace | |
AggregatedWorkspace(workspaceContext, | |
Some(workspaceContext.googleProjectId), | |
azureCloudContext = None, | |
policies = List.empty | |
) | |
case Failure(x) => throw x // bubble up an MC workspace exception | |
} |
val response = readWorkspace.convertTo[WorkspaceResponse] | ||
|
||
response.workspace.name shouldBe workspaceName | ||
response.azureContext shouldEqual None | ||
response.workspace.cloudPlatform shouldBe Some(WorkspaceCloudPlatform.Gcp) | ||
response.policies should not be empty | ||
val policies: List[WorkspacePolicy] = response.policies.get | ||
policies should not be empty | ||
val policy: WorkspacePolicy = policies.head | ||
policy.name shouldBe wsmPolicyInput.getName | ||
policy.namespace shouldBe wsmPolicyInput.getNamespace | ||
val additionalData = policy.additionalData | ||
additionalData.length shouldEqual 2 | ||
additionalData.head.getOrElse("pair1Key", "fail") shouldEqual "pair1Val" | ||
additionalData.tail.head.getOrElse("pair2Key", "fail") shouldEqual "pair2Val" |
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 also won't block the PR on any changes here; this code is fine. You could streamline a lot of the assertions by defining the expected result and checking for that, though:
val response = readWorkspace.convertTo[WorkspaceResponse] | |
response.workspace.name shouldBe workspaceName | |
response.azureContext shouldEqual None | |
response.workspace.cloudPlatform shouldBe Some(WorkspaceCloudPlatform.Gcp) | |
response.policies should not be empty | |
val policies: List[WorkspacePolicy] = response.policies.get | |
policies should not be empty | |
val policy: WorkspacePolicy = policies.head | |
policy.name shouldBe wsmPolicyInput.getName | |
policy.namespace shouldBe wsmPolicyInput.getNamespace | |
val additionalData = policy.additionalData | |
additionalData.length shouldEqual 2 | |
additionalData.head.getOrElse("pair1Key", "fail") shouldEqual "pair1Val" | |
additionalData.tail.head.getOrElse("pair2Key", "fail") shouldEqual "pair2Val" | |
val response = readWorkspace.convertTo[WorkspaceResponse] | |
response.workspace.name shouldBe workspaceName | |
response.azureContext shouldEqual None | |
response.workspace.cloudPlatform shouldBe Some(WorkspaceCloudPlatform.Gcp) | |
val expectedPolicy: WorkspacePolicy = new WorkspacePolicy("test_name", | |
"test_namespace", | |
List( | |
Map("pair1Key" -> "pair1Val"), | |
Map("pair2Key" -> "pair2Val") | |
) | |
) | |
response.policies should contain(List(expectedPolicy)) |
Ticket: https://broadworkbench.atlassian.net/browse/AJ-1750
Currently, Rawls only returns policies for Azure workspaces, and an empty list for GCP ("rawls") workspaces. This is because GCP workspaces usually don't have a corresponding entry in WSM. However, GCP workspaces with linked snapshots do, and we need to try and retrieve that to display policies in the UI.
This PR attempts to make the WSM fetch call for all workspaces, and returns a default entry if it doesn't exist. It's a similar method to what we're doing in the snapshot code here.
PR checklist
model/
, then you should publish a new officialrawls-model
and updaterawls-model
in Orchestration's dependencies.