CTM-279: copy fix over from PR #7826

[LizBaldo]

Duplicate of this PR #7826

Making a copy here so we don't merge from a different fork, and can use the full CI suite.

[sberss]

Apologies, I missed the test that checks the output copy command. The following patch fixes it:

diff --git a/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala b/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala
index 9c1a9c770..16fec6627 100644
--- a/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala
+++ b/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala
@@ -471,7 +471,6 @@ class AwsBatchJobSpec extends TestKitSuite with AnyFlatSpecLike with Matchers wi
     val job = generateJobWithS3InOut
     val postscript =
       s"""
-         |set -e
          |# (re-)add tags to include added volumes:
          |if [[ "false" == "true" ]]; then
          |  echo "*** TAGGING RESOURCES ***"
@@ -487,10 +486,16 @@ class AwsBatchJobSpec extends TestKitSuite with AnyFlatSpecLike with Matchers wi
          |if [ -f "/tmp/scratch/hello-stdout.log" ]; then _s3_delocalize_with_retry "/tmp/scratch/hello-stdout.log" "${job.jobPaths.standardPaths.output}"; fi
          |
          |echo "DELOCALIZATION RESULT: $$DELOCALIZATION_FAILED"
+         |rc=$$(head -n 1 /tmp/scratch/hello-rc.txt 2>/dev/null) || rc=1
          |if [[ $$DELOCALIZATION_FAILED -eq 1 ]]; then
          |  echo '*** DELOCALIZATION FAILED ***'
-         |  echo '*** EXITING WITH RETURN CODE 1***'
-         |  exit 1
+         |  if [[ "$$rc" -eq 0 ]]; then
+         |    echo '*** EXITING WITH RETURN CODE 1 ***'
+         |    exit 1
+         |  else
+         |    echo "*** EXITING WITH ORIGINAL RETURN CODE $$rc ***"
+         |    exit $$rc
+         |  fi
          |else
          |  echo '*** COMPLETED DELOCALIZATION ***'
          |fi

[LizBaldo]

Apologies, I missed the test that checks the output copy command. The following patch fixes it:

diff --git a/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala b/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala
index 9c1a9c770..16fec6627 100644
--- a/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala
+++ b/supportedBackends/aws/src/test/scala/cromwell/backend/impl/aws/AwsBatchJobSpec.scala
@@ -471,7 +471,6 @@ class AwsBatchJobSpec extends TestKitSuite with AnyFlatSpecLike with Matchers wi
     val job = generateJobWithS3InOut
     val postscript =
       s"""
-         |set -e
          |# (re-)add tags to include added volumes:
          |if [[ "false" == "true" ]]; then
          |  echo "*** TAGGING RESOURCES ***"
@@ -487,10 +486,16 @@ class AwsBatchJobSpec extends TestKitSuite with AnyFlatSpecLike with Matchers wi
          |if [ -f "/tmp/scratch/hello-stdout.log" ]; then _s3_delocalize_with_retry "/tmp/scratch/hello-stdout.log" "${job.jobPaths.standardPaths.output}"; fi
          |
          |echo "DELOCALIZATION RESULT: $$DELOCALIZATION_FAILED"
+         |rc=$$(head -n 1 /tmp/scratch/hello-rc.txt 2>/dev/null) || rc=1
          |if [[ $$DELOCALIZATION_FAILED -eq 1 ]]; then
          |  echo '*** DELOCALIZATION FAILED ***'
-         |  echo '*** EXITING WITH RETURN CODE 1***'
-         |  exit 1
+         |  if [[ "$$rc" -eq 0 ]]; then
+         |    echo '*** EXITING WITH RETURN CODE 1 ***'
+         |    exit 1
+         |  else
+         |    echo "*** EXITING WITH ORIGINAL RETURN CODE $$rc ***"
+         |    exit $$rc
+         |  fi
          |else
          |  echo '*** COMPLETED DELOCALIZATION ***'
          |fi

Thanks! I was about to write some tests this morning, much appreciated!

[aednichols]

I need to think about this some more.

Edit for clarity:
The PR clearly does address the user's test case, but there could be wider implications of removing set -e that we should take some time to think through. It's possible there are other users who rely on the current behavior.

[sberss]

The PR clearly does address the user's test case, but there could be wider implications of removing set -e that we should take some time to think through. It's possible there are other users who rely on the current behavior.

The other way to solve this without touching the set -e is to just move the lines to delocalise stderr, stdout and rc to the beginning of the delocalize block. That way these files will always be delocalized, even if other outputs are missing.

The proposed PR felt a bit nicer, but you're right that it may have implications for others that are currently dependant on the existing behaviour. I think moving those 3 copies to the top should be safe for everybody.