Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CZID-8390] Add sqs step notifications #116

Merged
merged 7 commits into from
Sep 25, 2023
Merged

[CZID-8390] Add sqs step notifications #116

merged 7 commits into from
Sep 25, 2023

Conversation

rzlim08
Copy link
Collaborator

@rzlim08 rzlim08 commented Aug 25, 2023 •
edited
Loading

  • Pins version of urllib3
  • Adds the same sqs miniwdl plugin as in the previous PR
  • Targets the existing queue
  • Ran black on the test/test_wdl.py file
  • Split the test message reception into stage notification and step notifications

@rzlim08 rzlim08 marked this pull request as ready for review August 25, 2023 21:14
@rzlim08 rzlim08 changed the title [CZID-8390] Add sqs step notifications part 2 [CZID-8390] Add sqs step notifications Sep 6, 2023
morsecodist
morsecodist suggested changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@morsecodist morsecodist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the plugin overall. My high level ask is a change to an SNS based approach that should actually be pretty similar to what you have and very slightly simpler. See my review comment.

miniwdl-plugins/sqs_notification/sqs_notification.py Outdated Show resolved Hide resolved
miniwdl-plugins/sqs_notification/sqs_notification.py Outdated
"TaskName": {"DataType": "String", "StringValue": run_id[-1]},
"ExecutionId": {
"DataType": "String",
"StringValue": "execution_id_to_be_passed_in",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this get populated? You should be able to pass this as an env variable in the step function definition.

Copy link
Collaborator Author

@rzlim08 rzlim08 Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we could also make this a WDL input & set it when the workflow starts. Either one is fine with me. We also want to use the Run Id here instead of the Execution Id

terraform/modules/swipe-sfn-batch-job/main.tf Outdated Show resolved Hide resolved
terraform/modules/swipe-sfn-batch-job/variables.tf Show resolved Hide resolved
terraform/modules/swipe-sfn-batch-job/variables.tf Outdated Show resolved Hide resolved
@rzlim08 rzlim08 merged commit 1e352bd into main Sep 25, 2023
3 checks passed
@rzlim08 rzlim08 deleted the rlim/add-sqs-step-take-2 branch September 25, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@morsecodist morsecodist morsecodist approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rzlim08 @morsecodist