- Notifications
You must be signed in to change notification settings - Fork925
docs: enable Slack notifications#14830
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Overall looks great!
I think@EdwardAngert should have a look as well, if possible.
Thanks for putting this together!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| receiver.router.post("/webhook", async (req, res) => { | ||
| try { | ||
| if (!req.body) { | ||
| return res.status(400).send("Error: request body is missing"); |
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 we fail the webhook receiver, the webhook will be retried.
In this case, I think we should just log but let the receiver return successfully.
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're right, but I'd like to keep the logic short. TBH I wanted to remove the HTTP request entity validation at all to indicate the communication flow with Slack API.
Also, this is rather a prevention mechanism for potential internet crawlers or scanners trying to send crappy requests.
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 we can still demonstrate the correct behaviour, but it's not a big deal.
Also, this is rather a prevention mechanism for potential internet crawlers or scanners trying to send crappy requests.
By that logic, shouldn't the requests succeed so the crawlers don't know their request was invalid?
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.
By that logic, shouldn't the requests succeed so the crawlers don't know their request was invalid?
That's a long story; frankly, it depends on the crawler implementation, and HTTP200 may encourage them to perform further scans :)
I'll defer to platform admins to take a decision on this 👍 .
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| receiver.router.post("/webhook", async (req, res) => { | ||
| try { | ||
| if (!req.body) { | ||
| return res.status(400).send("Error: request body is missing"); |
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 we can still demonstrate the correct behaviour, but it's not a big deal.
Also, this is rather a prevention mechanism for potential internet crawlers or scanners trying to send crappy requests.
By that logic, shouldn't the requests succeed so the crawlers don't know their request was invalid?
a3ffab6 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes:#14740
Preview:link
This PR adds a subpage to Coder docs with a guide on integrating Coder with MS Teams notifications.