AJ-1749 Remove import-service from firecloud-orchestration #1355

jladieu · 2024-05-10T19:16:58Z

Remove import service. Once https://github.com/broadinstitute/terra-helmfile/pull/5556 is live, cWDS will effectively field 100% of import traffic and all import-service backend resources will be deactivated.

Process:

Rename ImportServiceTypes -> LegacyServiceTypes (IDE refactor only)
Move LegacyFileTypes to CwdsDao (Manual refactor)
Rename ImportServiceDAO methods to match CwdsDao (IDE refactor only)
Make MockCwdsDao like MockImportServiceDao (manual)
Eliminate importServiceDao.getJobV1 (manual)
Eliminate importServiceDao.listJobsV1 (manual)
Remove importV1 from ImportService: and then make all the tests
still pass for vaguely the same reasons as before, except using cWDS.
Prune references to ImportService
Make supportedFormats consistent

Related PRs:

terra-helmfile: https://github.com/broadinstitute/terra-helmfile/pull/5538
rawls: AJ-1797: Eliminate import-service integration from Rawls rawls#2866

I, the developer opening this PR, do solemnly pinky swear that:

~~I've followed the instructions if I've made any changes to the API, especially if they're breaking changes~~ (N/A)
~~I've updated the RC_XXX release ticket with any manual steps required to release this change~~ (N/A)
~~I've updated the FISMA documentation if I've made any security-related changes, including auth, encryption, or auditing~~ (N/A)

In all cases:

Get two thumbsworth of review and PO signoff if necessary
Verify all tests go green
Squash and merge; you can delete your branch after this unless it's for a hotfix. In that case, don't delete it!
Test this change deployed correctly and works on dev environment after deployment

jladieu · 2024-05-10T19:18:24Z

src/test/scala/org/broadinstitute/dsde/firecloud/dataaccess/MockRawlsDAO.scala

@@ -271,7 +273,7 @@ class MockRawlsDAO extends RawlsDAO {
    WorkspaceDetails(
      namespace = "namespace",
      name = "name",
-      workspaceId = "workspaceId",
+      workspaceId = mockWorkspaceId,


Needed to switch this to a UUID to prevent an error attempting to initialize an actual UUID from the workspaceId

jladieu · 2024-05-10T19:18:42Z

src/test/scala/org/broadinstitute/dsde/firecloud/dataaccess/MockRawlsDAO.scala

 import scala.concurrent.ExecutionContext.Implicits.global
 import scala.concurrent.Future

 // Common things that can be accessed from tests
 object MockRawlsDAO {

+  val mockWorkspaceId = UUID.randomUUID().toString


Made available for a couple tests to refer to it.

jladieu · 2024-05-10T19:20:20Z

src/test/scala/org/broadinstitute/dsde/firecloud/webservice/WorkspaceApiServiceSpec.scala

        (Post(pfbImportPath, PFBImportRequest("https://bad.request.avro"))
          ~> dummyUserIdHeaders(dummyUserId)
          ~> sealRoute(workspaceRoutes)) ~> check {
-            status should equal(BadRequest)
-            responseAs[String] should include ("Bad request as reported by import service")
+            status should equal(InternalServerError) // TODO: bubble up 400 / BadRequest


The tests verify current behavior in prod, however these tests used to verify what happened in the interaction to ImportService so I left these TODOs in place to trigger a conversation over whether we should continue to strive to resurface errors, or if we're okay with just bubbling up as InternalServerError. If we're okay with leaving this as is, I could get rid of the TODOs.

My vote would be to deprecate Orchestration's import related endpoints entirely and move everything to talk directly to CWDS.

For these APIs, I would much rather bubble up the actual error/response code instead of wrapping everything in a 500. Since this current PR does not change behavior, I'm fine making that change in a follow-on.

For these APIs, I would much rather bubble up the actual error/response code instead of wrapping everything in a 500. Since this current PR does not change behavior, I'm fine making that change in a follow-on.

I plan to leave out any changes related to error handling from this PR, but wanted to highlight @nawatts 's suggestion here which is worth considering: #1355 (comment)

jladieu · 2024-05-10T19:21:29Z

src/test/scala/org/broadinstitute/dsde/firecloud/dataaccess/MockCwdsDAO.scala

+      workspaceId: String,
+      importRequest: AsyncImportRequest
+  )(implicit userInfo: UserInfo): GenericJob = {
+    importRequest.filetype match {


Copied this logic over from MockImportServiceDAO and then swapped this in wherever tests relied on it when interacting with ImportService.

jladieu · 2024-05-10T19:21:49Z

src/main/scala/org/broadinstitute/dsde/firecloud/dataaccess/CwdsDAO.scala

 import org.databiosphere.workspacedata.client.ApiException
 import org.databiosphere.workspacedata.model.GenericJob

+object LegacyFileTypes {


Pulled over from ImportServiceDAO which went away.

👍 for calling these out as for backwards compatibility

nawatts · 2024-05-13T13:29:37Z

src/test/scala/org/broadinstitute/dsde/firecloud/webservice/WorkspaceApiServiceSpec.scala

        (Post(pfbImportPath, PFBImportRequest("https://bad.request.avro"))
          ~> dummyUserIdHeaders(dummyUserId)
          ~> sealRoute(workspaceRoutes)) ~> check {
-            status should equal(BadRequest)
-            responseAs[String] should include ("Bad request as reported by import service")
+            status should equal(InternalServerError) // TODO: bubble up 400 / BadRequest


My vote would be to deprecate Orchestration's import related endpoints entirely and move everything to talk directly to CWDS.

calypsomatic · 2024-05-21T13:41:47Z

src/test/scala/org/broadinstitute/dsde/firecloud/EntityServiceSpec.scala

+        case (status, errorReport) =>
+          status shouldEqual StatusCodes.InternalServerError
+          errorReport.message should be("Unexpected error during async TSV import")
+      }
    }

    "should return error for (async=true) when import service returns sync error" in {


Test description should be "cwds" now, right?

Thanks for noticing this! This triggered an additional round of find/replace and I think I got them all this time.

calypsomatic · 2024-05-21T13:45:36Z

src/test/scala/org/broadinstitute/dsde/firecloud/dataaccess/MockCwdsDAO.scala

+      else if (importRequest.url.contains("good")) makeJob(workspaceId)
+      else
+        throw new ApiException(
+          EnhanceYourCalm.intValue,


Process: * Rename ImportServiceTypes -> LegacyServiceTypes (IDE refactor only) * Move `LegacyFileTypes` to `CwdsDao` (Manual refactor) * Rename ImportServiceDAO methods to match CwdsDao (IDE refactor only) * Make `MockCwdsDao` like `MockImportServiceDao` (manual) * Eliminate `importServiceDao.getJobV1` (manual) * Eliminate `importServiceDao.listJobsV1` (manual) * Remove `importV1` from ImportService: and then make all the tests still pass for vaguely the same reasons as before, except using cWDS. * Prune references to ImportService * Make supportedFormats consistent

* IDE: `{ImportService->Cwds}ListResponse` * IDE: `{ImportService->Cwds}Response` * Manual/IDE: Various methods/comments

calypsomatic · 2024-05-22T14:11:14Z

automation/README.md

@@ -38,7 +38,7 @@ TODO: update `render-local-env.sh` so it doesn't require manual code changes

 ##### Testing against your own FiaB



Not fully relevant to this PR, but FiaBs don't exist anymore

davidangb · 2024-05-22T14:32:29Z

local-dev/templates/firecloud-orchestration.conf.ctmpl

-  server = "https://terra-importservice-dev.appspot.com"
-  bucketName = "import-service-batchupsert-dev"
-  enabled = true
-}


do you have a corresponding terra-helmfile PR to remove this stanza?

github.com/broadinstitute/terra-helmfile/pull/5538

davidangb · 2024-05-22T14:38:44Z

src/test/scala/org/broadinstitute/dsde/firecloud/webservice/WorkspaceApiServiceSpec.scala

        (Post(pfbImportPath, PFBImportRequest("https://bad.request.avro"))
          ~> dummyUserIdHeaders(dummyUserId)
          ~> sealRoute(workspaceRoutes)) ~> check {
-            status should equal(BadRequest)
-            responseAs[String] should include ("Bad request as reported by import service")
+            status should equal(InternalServerError) // TODO: bubble up 400 / BadRequest


For these APIs, I would much rather bubble up the actual error/response code instead of wrapping everything in a 500. Since this current PR does not change behavior, I'm fine making that change in a follow-on.

jladieu commented May 10, 2024

View reviewed changes

jladieu requested a review from a team May 10, 2024 19:23

jladieu changed the title ~~AJ-1749 Remove import-service from firecloud-orchestration~~ DO_NOT_MERGE: AJ-1749 Remove import-service from firecloud-orchestration May 10, 2024

nawatts approved these changes May 13, 2024

View reviewed changes

jladieu mentioned this pull request May 14, 2024

AJ-1797: Eliminate import-service integration from Rawls broadinstitute/rawls#2866

Merged

8 tasks

davidangb mentioned this pull request May 17, 2024

AJ-1749: respect importService.enabled for listing jobs #1361

Merged

7 tasks

jladieu force-pushed the aj-1749-remove-import-service branch from 8dc16ff to 40d1baf Compare May 20, 2024 21:01

jladieu changed the title ~~DO_NOT_MERGE: AJ-1749 Remove import-service from firecloud-orchestration~~ AJ-1749 Remove import-service from firecloud-orchestration May 20, 2024

jladieu requested a review from a team May 20, 2024 21:03

nawatts approved these changes May 21, 2024

View reviewed changes

calypsomatic reviewed May 21, 2024

View reviewed changes

jladieu added 2 commits May 22, 2024 10:00

Rename various {ImportService->Cwds} references.

963d780

* IDE: `{ImportService->Cwds}ListResponse` * IDE: `{ImportService->Cwds}Response` * Manual/IDE: Various methods/comments

jladieu force-pushed the aj-1749-remove-import-service branch from 40d1baf to 963d780 Compare May 22, 2024 14:00

jladieu requested review from calypsomatic and nawatts May 22, 2024 14:05

calypsomatic reviewed May 22, 2024

View reviewed changes

calypsomatic approved these changes May 22, 2024

View reviewed changes

nawatts approved these changes May 22, 2024

View reviewed changes

davidangb approved these changes May 22, 2024

View reviewed changes

jladieu merged commit 681c7f5 into develop May 22, 2024
14 checks passed

jladieu deleted the aj-1749-remove-import-service branch May 22, 2024 16:15

davidangb mentioned this pull request May 23, 2024

AJ-1749: bubble up cwds error codes #1368

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AJ-1749 Remove import-service from firecloud-orchestration #1355

AJ-1749 Remove import-service from firecloud-orchestration #1355

jladieu commented May 10, 2024 •

edited

Loading

jladieu May 10, 2024

jladieu May 10, 2024

jladieu May 10, 2024

nawatts May 13, 2024

davidangb May 22, 2024

jladieu May 24, 2024

jladieu May 10, 2024

jladieu May 10, 2024

nawatts May 22, 2024 •

edited

Loading

nawatts May 13, 2024

calypsomatic May 21, 2024

jladieu May 22, 2024

calypsomatic May 21, 2024

calypsomatic May 22, 2024

davidangb May 22, 2024

jladieu May 22, 2024

davidangb May 22, 2024

		@@ -38,7 +38,7 @@ TODO: update `render-local-env.sh` so it doesn't require manual code changes

		##### Testing against your own FiaB

AJ-1749 Remove import-service from firecloud-orchestration #1355

AJ-1749 Remove import-service from firecloud-orchestration #1355

Conversation

jladieu commented May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nawatts May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jladieu commented May 10, 2024 •

edited

Loading

nawatts May 22, 2024 •

edited

Loading