From c785a0ae7e87c63b255032d5874e751a91a4c990 Mon Sep 17 00:00:00 2001 From: Janet Gainer-Dewar Date: Mon, 14 Feb 2022 10:24:06 -0500 Subject: [PATCH] Include optional strings in backend_parameters [BT-551] (#6673) * Support differentiation between empty string and null in backend_parameters * Pass optional string values through in backend_parameters * Whitespace * Update supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesRuntimeAttributesSpec.scala Co-authored-by: Miguel Covarrubias Co-authored-by: Miguel Covarrubias --- .../impl/tes/TesRuntimeAttributes.scala | 12 +++++++++--- .../cromwell/backend/impl/tes/TesTask.scala | 4 ++-- .../impl/tes/TesRuntimeAttributesSpec.scala | 19 ++++++++++++++++--- .../backend/impl/tes/TesTaskSpec.scala | 6 +++--- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesRuntimeAttributes.scala b/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesRuntimeAttributes.scala index 1a744f5da16..103f8e51967 100644 --- a/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesRuntimeAttributes.scala +++ b/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesRuntimeAttributes.scala @@ -9,6 +9,7 @@ import eu.timepit.refined.api.Refined import eu.timepit.refined.numeric.Positive import wom.RuntimeAttributesKeys import wom.format.MemorySize +import wom.types.WomStringType import wom.values._ case class TesRuntimeAttributes(continueOnReturnCode: ContinueOnReturnCode, @@ -19,7 +20,7 @@ case class TesRuntimeAttributes(continueOnReturnCode: ContinueOnReturnCode, memory: Option[MemorySize], disk: Option[MemorySize], preemptible: Boolean, - backendParameters: Map[String, String]) + backendParameters: Map[String, Option[String]]) object TesRuntimeAttributes { @@ -55,12 +56,17 @@ object TesRuntimeAttributes { def makeBackendParameters(runtimeAttributes: Map[String, WomValue], keysToExclude: Set[String], - config: TesConfiguration): Map[String, String] = { + config: TesConfiguration): Map[String, Option[String]] = { if (config.useBackendParameters) runtimeAttributes .filterKeys(k => !keysToExclude.contains(k)) - .collect { case (key, strValue: WomString) => (key, strValue.value)} + .flatMap( _ match { + case (key, WomString(s)) => Option((key, Option(s))) + case (key, WomOptionalValue(WomStringType, Some(WomString(optS)))) => Option((key, Option(optS))) + case (key, WomOptionalValue(WomStringType, None)) => Option((key, None)) + case _ => None + }) else Map.empty } diff --git a/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesTask.scala b/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesTask.scala index 3ebb1b82ac5..b4e9655b3c7 100644 --- a/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesTask.scala +++ b/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesTask.scala @@ -238,7 +238,7 @@ object TesTask { .workflowOptions .get(TesWorkflowOptionKeys.WorkflowExecutionIdentity) .toOption - .map(TesWorkflowOptionKeys.WorkflowExecutionIdentity -> _) + .map(TesWorkflowOptionKeys.WorkflowExecutionIdentity -> Option(_)) .toMap val disk :: ram :: _ = Seq(runtimeAttributes.disk, runtimeAttributes.memory) map { @@ -298,7 +298,7 @@ final case class Resources(cpu_cores: Option[Int], disk_gb: Option[Double], preemptible: Option[Boolean], zones: Option[Seq[String]], - backend_parameters: Option[Map[String, String]]) + backend_parameters: Option[Map[String, Option[String]]]) final case class OutputFileLog(url: String, path: String, diff --git a/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesRuntimeAttributesSpec.scala b/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesRuntimeAttributesSpec.scala index 210ac12ca15..6314ed30d3a 100644 --- a/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesRuntimeAttributesSpec.scala +++ b/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesRuntimeAttributesSpec.scala @@ -75,7 +75,7 @@ class TesRuntimeAttributesSpec extends AnyWordSpecLike with CromwellTimeoutSpec val runtimeAttributes = Map("docker" -> WomString("ubuntu:latest"), "preemptible" -> WomString("yes")) assertFailure(runtimeAttributes, "Expecting preemptible runtime attribute to be a Boolean or a String with values of 'true' or 'false'") } - + "validate a valid continueOnReturnCode entry" in { val runtimeAttributes = Map("docker" -> WomString("ubuntu:latest"), "continueOnReturnCode" -> WomInteger(1)) val expectedRuntimeAttributes = expectedDefaultsPlusUbuntuDocker.copy(continueOnReturnCode = ContinueOnReturnCodeSet(Set(1))) @@ -160,13 +160,26 @@ class TesRuntimeAttributesSpec extends AnyWordSpecLike with CromwellTimeoutSpec "turn unknown string attributes into backend parameters" in { val runtimeAttributes = Map("docker" -> WomString("ubuntu:latest"), "foo" -> WomString("bar")) - val expectedRuntimeAttributes = expectedDefaults.copy(backendParameters = Map("foo" -> "bar")) + val expectedRuntimeAttributes = expectedDefaults.copy(backendParameters = Map("foo" -> Option("bar"))) assertSuccess(runtimeAttributes, expectedRuntimeAttributes, tesConfig = mockTesConfigWithBackendParams) } "exclude unknown non-string attributes from backend parameters" in { val runtimeAttributes = Map("docker" -> WomString("ubuntu:latest"), "foo" -> WomInteger(5), "bar" -> WomString("baz")) - val expectedRuntimeAttributes = expectedDefaults.copy(backendParameters = Map("bar" -> "baz")) + val expectedRuntimeAttributes = expectedDefaults.copy(backendParameters = Map("bar" -> Option("baz"))) + assertSuccess(runtimeAttributes, expectedRuntimeAttributes, tesConfig = mockTesConfigWithBackendParams) + } + + + "turn populated optional unknown string attributes into backend parameters" in { + val runtimeAttributes = Map("docker" -> WomString("ubuntu:latest"), "foo" -> WomOptionalValue(WomString("bar"))) + val expectedRuntimeAttributes = expectedDefaults.copy(backendParameters = Map("foo" -> Option("bar"))) + assertSuccess(runtimeAttributes, expectedRuntimeAttributes, tesConfig = mockTesConfigWithBackendParams) + } + + "turn unpopulated optional unknown string attributes into backend parameters" in { + val runtimeAttributes = Map("docker" -> WomString("ubuntu:latest"), "foo" -> WomOptionalValue.none(WomStringType)) + val expectedRuntimeAttributes = expectedDefaults.copy(backendParameters = Map("foo" -> None)) assertSuccess(runtimeAttributes, expectedRuntimeAttributes, tesConfig = mockTesConfigWithBackendParams) } } diff --git a/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesTaskSpec.scala b/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesTaskSpec.scala index 347049714c9..b71a9ddefd5 100644 --- a/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesTaskSpec.scala +++ b/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesTaskSpec.scala @@ -30,21 +30,21 @@ class TesTaskSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matchers wit it should "create the correct resources when an identity is passed in WorkflowOptions" in { val wd = workflowDescriptorWithIdentity(Option("abc123")) TesTask.makeResources(runtimeAttributes, wd) shouldEqual - Resources(None, None, None, Option(false), None, Option(Map(TesWorkflowOptionKeys.WorkflowExecutionIdentity -> "abc123")) + Resources(None, None, None, Option(false), None, Option(Map(TesWorkflowOptionKeys.WorkflowExecutionIdentity -> Option("abc123"))) ) } it should "create the correct resources when an empty identity is passed in WorkflowOptions" in { val wd = workflowDescriptorWithIdentity(Option("")) TesTask.makeResources(runtimeAttributes, wd) shouldEqual - Resources(None, None, None, Option(false), None, Option(Map(TesWorkflowOptionKeys.WorkflowExecutionIdentity -> "")) + Resources(None, None, None, Option(false), None, Option(Map(TesWorkflowOptionKeys.WorkflowExecutionIdentity -> Option(""))) ) } it should "create the correct resources when no identity is passed in WorkflowOptions" in { val wd = workflowDescriptorWithIdentity(None) TesTask.makeResources(runtimeAttributes, wd) shouldEqual - Resources(None, None, None, Option(false), None, Option(Map.empty[String, String]) + Resources(None, None, None, Option(false), None, Option(Map.empty[String, Option[String]]) ) } }