-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: add runId to slack thread when sanities fail #270
Conversation
service/src/alertOnResult.js
Outdated
title: 'runId for sanity test:', | ||
text: message.runId, | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is perfectly fine to merge as is, though something I'd like to do when my larger PR is merged in is consolidate all the slack messages into a single message for improved readability and to reduce chance of rate limiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I don't love the thread either.
Created story: https://tophat.atlassian.net/browse/SERVINFRA-1246
You'll want to rebase |
7453d8e
to
d7db28b
Compare
service/src/alerts/index.ts
Outdated
}: { | ||
testFiles: Record<string, string> | ||
results: EnhancedAggregatedResult | ||
testVariables: Partial<Record<string, string>> | ||
runId: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runId: string, | |
runId: string |
L20 is a typescript type which uses semicolons or newlines as separators. So you can just remove the comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @noahnu 🙏 , test is passing now.
d7db28b
to
8b7ba05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM until we can consolidate the messages
https://tophat.atlassian.net/jira/software/c/projects/WEB/boards/247?modal=detail&selectedIssue=SERVINFRA-1209