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.

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

Notify tests fails to slack #1082

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Notify tests fails to slack #1082

merged 1 commit into from
Aug 21, 2023

Conversation

noaKurman
Copy link
Contributor

No description provided.

@linear
Copy link

linear bot commented Aug 15, 2023

ELE-1158 open source daily test run fails - alert on slack

Definition of done:

When the dbt-data-reliability daily run fails it'd send an alerts to slack to notify the failure

@github-actions
Copy link
Contributor

👋 @noaKurman
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@noaKurman noaKurman marked this pull request as ready for review August 16, 2023 10:55
@noaKurman noaKurman force-pushed the ele-1158-notify-tests-fails branch 2 times, most recently from 19da411 to cbcc3a2 Compare August 16, 2023 12:19
@noaKurman
Copy link
Contributor Author

General comment - I know there's code duplication here, which I might could have exported to a separated workflow, but then I'd need to use "wait for" which is much slower, so for now this feels good enough

@noaKurman noaKurman requested a review from elongl August 21, 2023 06:53
@noaKurman noaKurman force-pushed the ele-1158-notify-tests-fails branch 3 times, most recently from a7aa03e to ed0a32f Compare August 21, 2023 11:14
@@ -0,0 +1,39 @@
name: Test all warehouse platforms
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be Notify Slack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops

notify:
name: Notify Slack Failure
runs-on: ubuntu-latest
if: ${{ inputs.result == 'failure' }}
Copy link
Member

@elongl elongl Aug 21, 2023

Choose a reason for hiding this comment

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

The caller should choose when to call this job.
If someone called Notify Slack it should run.
Would also change the name to be Notify Slack and not Notify Slack Failure to remain generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm coloring the message, so if we'll want to add a "success" message we'll add another job with a different if statement. For now doesn't feel like a big deal in the implementation

Copy link
Member

Choose a reason for hiding this comment

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

If you're not going to change it, then I think we should rename this workflow and step to notify_slack_failure and remove the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I might add a success job. thats what I'm saying

.github/workflows/notify_slack.yml Show resolved Hide resolved
with:
result: "failure"
run_id: ${{ github.run_id }}
workflow_name: "Test elementary GitHub action"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
workflow_name: "Test elementary GitHub action"
workflow_name: "Test Elementary GitHub action"

Forgive my OCD 😅

.github/workflows/test-all-warehouses.yml Show resolved Hide resolved
@noaKurman noaKurman merged commit 550d459 into master Aug 21, 2023
2 checks passed
@noaKurman noaKurman deleted the ele-1158-notify-tests-fails branch August 21, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants