Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Notifier][Slack] Send messages using Incoming Webhooks App#35828
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.
Nice addition
src/Symfony/Component/Notifier/Bridge/Slack/SlackWebhookTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Notifier/Bridge/Slack/SlackWebhookTransport.php OutdatedShow resolvedHide resolved
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.
src/Symfony/Component/Notifier/Bridge/Slack/SlackWebhookTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Notifier/Bridge/Slack/SlackWebhookTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Notifier/Bridge/Slack/SlackWebhookTransport.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
@chalasr What is the best approach for this? |
Updated SlackTransport to use Incoming Webhooks Slack app for sending messages. |
29827ad
to152e869
CompareTests fixed |
I have removed the channel & username setup from DSN. |
b6eaea1
to2411765
Compare@Nyholm can you have a look on this? #SymfonyLive :) |
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.
Thank you.
There are some BC breaks. But that is fine since the component is experimental and the API discontinued.
I added some small comments. The code looks fine but I need to look at the API docs and run the code to make sure it is working properly.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Notifier/Bridge/Slack/SlackTransportFactory.php OutdatedShow resolvedHide resolved
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.
I'm taking over to finish this PR. |
96065bb
to059026a
Compare059026a
to4b0807b
CompareNew usage: useSymfony\Component\Notifier\Message\ChatMessage;useSymfony\Component\EventDispatcher\EventDispatcher;useSymfony\Component\Notifier\Transport;$dispatcher =newEventDispatcher();// https://hooks.slack.com/services/T64B39MPG//BABMSGW9C/ZOhzOOo726qazXhJVxRBs2yP$slack = Transport::fromDsn('slack://default/T64B39MPG//BABMSGW9C/ZOhzOOo726qazXhJVxRBs2yP',$dispatcher);$message =newChatMessage('Hello!');$slack->send($message);// change the recipient$options->recipient('T64B39MPG/BMNM4FUA1/98mBsq2VHu1r5gOWG3D2wUdK');$slack->send($message); |
Thank you@birkof. |
Thank you too,@fabpot! |
@birkof@fabpot, I don't understand the rationale behind using webhooks instead of the web API here. Was it to avoid setting up a Slack app, and use to use theIncomning Webhooks app? But it is part of Slack'slegacy custom integration. So if we're left with having to create a Slack app anyway, why use webhooks over the web API? Would this mean a different transport? |
@xavierbriand TBH, there is no reasons on my part. Diving into Slack docs is just too tedious and time consuming for me. So, I think I'm just trusting people willing to fix the mess :) If you think there is a better solution, please, tell us what that would be :) |
@xavierbriand I think it is a misunderstood on your side, maybe because the title here. |
My confusion probably comes from the fact that 5.0 was implementingchat.postMessage and working as expected with current OAuth access tokens. Have the notifier using Slack apps webhook is a totally valid case. I wonder what would be a way to us either or. Could that be express in the DSN and then the transporter could infer the expected intent? From 5.0 and 5.1 implementations:
Having a username could be the trigger to switch from one method to the other. @birkof tell me what you think, I'll submit a PR. |
I understand. But I believe Slack was discontinuing that API. That is why we were forced to change things. Im sorry if it caused any issues. Feel free to open a new issue or discuss with the community in Slack. Let's not ping people in this thread anymore. =) |
IMHO and how@Nyholm said, we shouldn't rely anymore on discontinued API. |
…nstead of WebHooks (xavierbriand)This PR was squashed before being merged into the 5.2-dev branch.Discussion----------[Notifier][Slack] Use Slack Web API chat.postMessage instead of WebHooks| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets || License | MIT| Doc PR |**TL;DR**: Revert changes introduced in 5.1 by#35828: Slack's Web API is much more flexible and easier to operate than Slack WebHooks are.---### ContextA valid choice was made to switch from Slack's Web API to Slack's WebHooks for a wrong reason: according to#35828, this was in reaction to the deprecation of Slack's Legacy Tokens.The author cites:> Legacy tokens are a deprecated method of generating tokens for testing and development.[As far as I was able to understand](#35828 (comment)), from the author perspective, this deprecation was amalgamated with the (perceived) deprecation of the Web API itself.According to [Slack documentation](https://api.slack.com/legacy/custom-integrations/legacy-tokens) cited by the author:> If you were using a legacy token to make calls with the Web API, you'll need to generate a new one for your new Slack app.**So Slack changing its credentials' policy didn't warrant changing how to use Slack's Web API.**---### Why Web API > WebHook?While using Slack WebHooks as the underlying transport method for notification isn't inherently a bad choice, it does–on top of [the reasons cited by the author](#35828)–come with a major limitation: the need to setup (manually or programmatically) a Webhook per channel/recipient.The Web API, with it's underlying OAuth Access Token, is much more flexible and allows for more diverse use case. Notifications can be sent on behalf of users for instance. Slack can also limit the use of the access tokens to a list of IP addresses and ranges.**E.g. I want to notify the person who triggers a CD pipeline if the latter fails.** \Assuming a team of 10 developer, using Slack WebHooks, I would need to setup 10 WebHooks, and manages as many secret in my Symfony app. Furthermore, I would need to create new WebHook each time a new member were to join the team. \With the Web API, I would only need to manage a single access token for the whole Slack workspace, regardless of who as access to this workspace.Slack's Web API is much more flexible and easier to operate than Slack WebHooks are.Commits-------bb614c2 [Notifier][Slack] Use Slack Web API chat.postMessage instead of WebHooks
PythooonUser commentedJul 15, 2021
Thank you for adding this! 🎉 |
PythooonUser commentedJul 15, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I see no mention about this in the docs, however. It still refers to the old token authentication/setup. |
It has been reverted to allow to send to different recipients instead of a single one which is pre-selected with a webhook. |
PythooonUser commentedJul 15, 2021
Thanks! Yeah, I found the corresponding PR#37138. Wasn't sure about the reasons for this revert, but now I know :) |
Uh oh!
There was an error while loading.Please reload this page.
As tokens will became deprecated on May 5th, 2020 we should use Incoming Webhooks app for using this bundle.
Usage:
slack://hooks.slack.com/services/xxx/xxx/xxx