-
Notifications
You must be signed in to change notification settings - Fork 189
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
Issue 1366 - Slack integrations [ Siblings Check ] #1530
Issue 1366 - Slack integrations [ Siblings Check ] #1530
Conversation
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.
It looks good, just a few nits. We'll need a teams/zoom session to test this.
tools/autodeployment/env.example
Outdated
# body: "text": "This is the message" | ||
# }); | ||
# This will send the following message to our Slack channel: "This is the message" | ||
SLACK_SEND_MESSAGE= |
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'm not sure of what this means. Is SLACK_SEND_MESSAGE
the text that will be sent to the channel?
Also, I don't think you need to show an example here, the URL to webhooks should be enough.
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.
Example requested by @humphd:
#1472 (comment)
SLACK_SEND_MESSAGE
is an URL that receives a fetch/post with the message as the body and publishes it on our slack channel. It's almost like an API KEY, everyone with this URL will be able to send messages to our slack channel.
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 still think the example should go in the docs, where all the env
vars are explained.
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.
Sure, you are right. We should add them to this doc too.
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.
If this is a URL, let's label it as such. I think @manekenpix's confusion is partly due to the name. What about SLACK_WEBHOOK_URL
instead?
tools/autodeployment/server.js
Outdated
// Server health check | ||
|
||
firstCheck(); |
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.
You could just call it healthCheck()
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.
Sure. It will make more sense in this scope.
process.DEPLOY_TYPE === 'production' | ||
? 'https://telescope.cdot.systems/health.' | ||
: 'https://dev.telescope.cdot.systems/health.'; |
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.
We could do here what @humphd suggested. We can make the servers check each other, so if the machine is down, we'd still get the status, but I think that you'd need to add a case in which the server never answers.
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 will sketch this feature. I think it will be better to understand it. But basically, we have the firstCheck
running every 2 min. If the firstCheck
fails, it will trigger the secondCheck
that stops the firstCheck
and performs a fetch every 10sec. If the secondCheck
fails for the first time, the message "SERVER DOWN" is sent to us on SLACK. Once the message is sent, the secondCheck continues to run (the message informing us about the crash is sent just once). When the server is back, secondCheck
sends the message "SERVER IS BACK", stop itself and triggers the firstCheck
.
Because we rely on the network, sometimes the firstCheck
can emit a false-negative when it fails due to network "issues" (timeout, etc.). That's why I created this system to double-check the server and try to avoid false-negative messages on our slack channel.
Sure. Let's do it. |
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.
Looking better and better. I agree, let's get on a call early this week and test it together. Let's co-ordinate on Slack when these changes are done and we can test for real.
tools/autodeployment/env.example
Outdated
# body: "text": "This is the message" | ||
# }); | ||
# This will send the following message to our Slack channel: "This is the message" | ||
SLACK_SEND_MESSAGE= |
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.
If this is a URL, let's label it as such. I think @manekenpix's confusion is partly due to the name. What about SLACK_WEBHOOK_URL
instead?
@@ -0,0 +1,81 @@ | |||
/* eslint-disable no-use-before-define */ | |||
/* eslint-disable no-unused-expressions */ |
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.
Are these really necessary? Can we not change your code to fit our linting style?
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.
Regarding the /* eslint-disable no-unused-expressions */
is solved.
But regarding /* eslint-disable no-use-before-define */
I have tried until I run out of ideas.
Because the firstCheck
calls the secondCheck
and vice versa.
const a = () => {
b(); -> LINTER: "b" was used before it was defined.
}
const b = () => {
a()
}
Any idea about this is welcome.
clearIntervalAsync(timerOne); | ||
secondCheck(); | ||
} | ||
}, 120000); |
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.
All these magic numbers should probably be something we can configure via env.
image_url: | ||
status === 200 | ||
? 'https://data.whicdn.com/images/186936913/original.jpg' | ||
: 'https://64.media.tumblr.com/090e86896948354262566ddb906e514e/tumblr_mxrsfhStH61r62zwpo1_500.jpg', |
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.
Should we pull images we depend on into our repo so we can rely on the URLs not going down in the future?
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.
Sure, this is just a test message.
This is very cool, might I add a suggestion? I know this is bare bones at the moment, but I was wondering if we can write the status of the server into a .txt file It might be a bit easier on the eyes, and it'll help archive the logs from the server |
Could you please elaborate more? |
I'm updating the images because I'm not sure about the license of the previous images. |
Fixes Seneca-CDOT#1815: Spotify playlists within an iframe are not sanitized
Thank you very much to everyone committing to this PR, your work is very appreciated. We will be coming back to this PR later, so we are closing it for now. |
in the server entry point. And REBASE
*fix env access on Siblings Check module *docs updated
order prod checking dev and vice versa
Sorry, I will reopen this PR for testing purposes. |
Okay tested. Sorry. |
Issue This PR Addresses
Type of Change
Description
This PR is part of the solution of #1366.
Siblings Checkers: DEV and PROD checking each other periodically.
We also have an internal checker:
Slack Integration: Docker Events
Checklist