[GH-1592] Notify Slack watchers on Terra submission creation #591

okotsopoulos · 2022-03-15T18:17:19Z

Purpose

https://broadinstitute.atlassian.net/browse/GH-1592
https://broadinstitute.atlassian.net/browse/GH-1635 Fix unrelated failing tests

When a Terra submission is created, WFL emits a Slack message with a link to the submission as well as the source snapshot.

Changes

executor

Call the Slack notifier when we create a Terra submission
Refactoring to pass around snapshot object rather than refetch it from TDR
Refactoring to generate Terra submission comment when creating submission

Testing

with-redefs-fn to with-redefs in the test files I touched for better readability
Fix unrelated Firecloud integration test failures: they were referencing an old workflow whose metadata just got archived

Manual Verification

The system test wfl.system.v1-endpoint-test/test-workload-sink-outputs-to-tdr generates a Slack notification when it creates its submission, but its links break when the test cleans up its temporary resources:

https://broadinstitute.slack.com/archives/C026PTM4XPA/p1647364194127229

I launched a workload off of a local WFL with a fixed snapshot:

{
  "project": "wfl-dev/CDC_Viral_Sequencing_okotsopo_GH-1592",
  "labels": [
    "hornet:test"
  ],
  "watchers": [
    ["slack", "C026PTM4XPA", "#hornet-slack-app-testing"]
  ],
  "source": {
    "name": "TDR Snapshots",
    "snapshots": ["7c70fb93-34df-49c9-8ef7-26f03fe48816"]
  },
  "executor": {
    "name": "Terra",
    "workspace": "wfl-dev/CDC_Viral_Sequencing_okotsopo_GH-1592",
    "methodConfiguration": "wfl-dev/sarscov2_illumina_full",
    "fromSource": "importSnapshot"
  },
  "sink": {
    "name": "Terra Workspace",
    "workspace": "wfl-dev/CDC_Viral_Sequencing_okotsopo_GH-1592",
    "entityType": "flowcell",
  "fromOutputs": {
…
    },
  "identifier": "run_id",
    "skipValidation": true
  }
}

Saw the submission launch Slack message for the original submission as well as its retry. The links in these messages work since the snapshot and workspace were not deleted, I clicked through them and confirmed that they work.

System Tests

Passed:

wm111-e35:wfl okotsopo$ make TARGET=system
export CPCACHE=/Users/okotsopo/wfl/api/.cpcache;            \
	export WFL_WFL_URL=http://localhost:3000; \
	clojure  -M:parallel-test wfl.system.v1-endpoint-test | \
	tee /Users/okotsopo/wfl/derived/api/system.log
WARNING: Specified path is external to project: ../derived/api/src
WARNING: Specified path is external to project: ../derived/api/resources

Ran 33 tests containing 374 assertions.
0 failures, 0 errors.
api system finished on Tue Mar 15 13:20:09 EDT 2022
docs system finished on Tue Mar 15 13:20:10 EDT 2022
functions/aou system finished on Tue Mar 15 13:20:10 EDT 2022
functions/sg system finished on Tue Mar 15 13:20:10 EDT 2022
helm system finished on Tue Mar 15 13:20:10 EDT 2022
ui system finished on Tue Mar 15 13:20:10 EDT 2022

Review Instructions

Advise hiding whitespace in review, especially for changes to test code.
Take a look at the Slack messages mentioned in "Manual Verification".
The meat of this PR is found in executor/create-submission!

okotsopoulos · 2022-03-15T18:24:56Z

Test failures due to unrelated bug:

https://broadinstitute.atlassian.net/browse/GH-1635

api/src/wfl/executor.clj

Referencing a new unarchived workflow.

okotsopoulos · 2022-03-15T20:21:01Z

api/src/wfl/executor.clj

+  ([{:keys [executor] :as workload}
+    user-comment-note
+    reference
+    [_type snapshot   :as _source-object]]


Heads up with some historical context:

We get this from the source queue, originally I thought this would be a snapshot but instead it's a pair:

[:datarepo/snapshot <snapshot>]

I think the original intention was to think of a future where we submitted something-that-isn't-a-snapshot (like a row in a Terra entity table). But I elected not to fuss much with it.

tbl3rd · 2022-03-15T20:58:21Z

api/src/wfl/executor.clj

+   (->> (get-in reference [:attributes :snapshot])
+        datarepo/snapshot
+        (conj [:datarepo/snapshot])
+        (create-submission! workload user-comment-note reference))))


OK, but why combine the 2 arities here?
They seem different.
Maybe create-submission and create-retry?
(And then we wouldn't need the user-comment-note parameter.)

I thought about this but I was wary about the potential for unintended deviation. Besides the contents of the comment, the operations performed in either case are the same (alter method config, submit method config, emit Slack message). No hard feelings if you elect to fuss with this, of course.

tbl3rd · 2022-03-15T21:04:37Z

api/src/wfl/service/datarepo.clj

+(defn snapshot-url
+  "Return a link to `snapshot` in TDR UI."
+  [{:keys [id] :as _snapshot}]
+  (datarepo-url "snapshots/details" id))


Probably spozed to be (datarepo-url "snapshots" "details" id), but OK.

I wondered about that, but I did see other usage patterns like this:

(def ^:private repository "API URL for Data Repo API." (partial datarepo-url "api/repository/v1"))

tbl3rd · 2022-03-15T21:07:09Z

api/test/wfl/unit/executor_test.clj

+       [executor/workflow-finished-slack-msg mock-workflow-finished-slack-msg
+        slack/notify-watchers                (constantly nil)]
+        (is (= records (#'executor/notify-on-workflow-completion workload records))
+            "Should return all passed-in records")))))


Just … craazy.

tbl3rd · 2022-03-15T21:09:08Z

api/test/wfl/integration/firecloud_test.clj

-         ~'wdl {:path    "pipes/WDL/workflows/assemble_refbased.wdl"
-                :release "master"
-                :repo    "viral-pipelines"}]
-     ~@body))


GH-1592 GH-1635 Notify Slack watchers on Terra submission creation (#591) GH-1593 Notify Slack watchers on failed TDR snapshot jobs (#590) GH-1618 Add workload info to executor logs (#589) GH-1633 Remove WFL_SLACK_ENABLED feature switch (#587)

[GH-1592] Notify Slack watchers on Terra submission creation

478cc96

okotsopoulos commented Mar 15, 2022

View reviewed changes

api/src/wfl/executor.clj Show resolved Hide resolved

okotsopoulos commented Mar 15, 2022

View reviewed changes

api/src/wfl/executor.clj Show resolved Hide resolved

[GH-1635] Fix failing Firecloud integration tests

ae010a4

Referencing a new unarchived workflow.

okotsopoulos commented Mar 15, 2022

View reviewed changes

okotsopoulos marked this pull request as ready for review March 15, 2022 20:30

okotsopoulos requested a review from tbl3rd March 15, 2022 20:30

tbl3rd self-assigned this Mar 15, 2022

tbl3rd approved these changes Mar 15, 2022

View reviewed changes

Tighten up whitespace

8c28932

okotsopoulos merged commit 705fa7e into develop Mar 21, 2022

okotsopoulos deleted the okotsopo/GH-1592-slack-submission-launch branch March 21, 2022 19:08

okotsopoulos mentioned this pull request Mar 21, 2022

[GH-1634] Release WFL v0.17.0 #592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-1592] Notify Slack watchers on Terra submission creation #591

[GH-1592] Notify Slack watchers on Terra submission creation #591

okotsopoulos commented Mar 15, 2022 •

edited

okotsopoulos commented Mar 15, 2022

okotsopoulos Mar 15, 2022

tbl3rd Mar 15, 2022

okotsopoulos Mar 21, 2022

tbl3rd Mar 15, 2022

okotsopoulos Mar 21, 2022

tbl3rd Mar 15, 2022

tbl3rd Mar 15, 2022

[GH-1592] Notify Slack watchers on Terra submission creation #591

[GH-1592] Notify Slack watchers on Terra submission creation #591

Conversation

okotsopoulos commented Mar 15, 2022 • edited

Purpose

Changes

Manual Verification

System Tests

Review Instructions

okotsopoulos commented Mar 15, 2022

okotsopoulos Mar 15, 2022

Choose a reason for hiding this comment

tbl3rd Mar 15, 2022

Choose a reason for hiding this comment

okotsopoulos Mar 21, 2022

Choose a reason for hiding this comment

tbl3rd Mar 15, 2022

Choose a reason for hiding this comment

okotsopoulos Mar 21, 2022

Choose a reason for hiding this comment

tbl3rd Mar 15, 2022

Choose a reason for hiding this comment

tbl3rd Mar 15, 2022

Choose a reason for hiding this comment

okotsopoulos commented Mar 15, 2022 •

edited