Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged

Conversation

birkof
Copy link
Contributor

@birkofbirkof commentedFeb 22, 2020
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
Tickets
LicenseMIT
Doc PR

Legacy tokens are a deprecated method of generating tokens for testing and development.

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

Copy link
Contributor

@OskarStarkOskarStark left a comment

Choose a reason for hiding this comment

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

Nice addition

@birkof
Copy link
ContributorAuthor

@chalasrnext milestone is due by May 31, 2020 and Slackwill not allow to generate a new token after May 5th, 2020, so this bridge will become unusable for newcomers.

What is the best approach for this?

@birkofbirkof changed the titleSend Slack messages using Incoming Webhooks[Notifier][Slack] Send messages using Incoming WebhooksFeb 25, 2020
@birkof
Copy link
ContributorAuthor

Updated SlackTransport to use Incoming Webhooks Slack app for sending messages.

@birkofbirkofforce-pushed thenotifier/SendWithIncomingWebhooks branch 2 times, most recently from29827ad to152e869CompareMarch 13, 2020 19:25
@birkof
Copy link
ContributorAuthor

Tests fixed

@birkof
Copy link
ContributorAuthor

I have removed the channel & username setup from DSN.
As specifiedhere, you cannot override the default channel (chosen by the user who installed your app), username, or icon when you're using Incoming Webhooks to post messages.

@birkofbirkofforce-pushed thenotifier/SendWithIncomingWebhooks branch fromb6eaea1 to2411765CompareMarch 23, 2020 09:48
@birkof
Copy link
ContributorAuthor

@Nyholm can you have a look on this? #SymfonyLive :)

Copy link
Member

@NyholmNyholm left a 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.

@fabpot
Copy link
Member

I'm taking over to finish this PR.

@fabpotfabpotforce-pushed thenotifier/SendWithIncomingWebhooks branch 3 times, most recently from96065bb to059026aCompareApril 12, 2020 13:44
@fabpotfabpotforce-pushed thenotifier/SendWithIncomingWebhooks branch from059026a to4b0807bCompareApril 12, 2020 13:45
@fabpot
Copy link
Member

New 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);
birkof and alejandrososa reacted with thumbs up emoji

@fabpot
Copy link
Member

Thank you@birkof.

@birkof
Copy link
ContributorAuthor

Thank you too,@fabpot!

@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
@xavierbriand
Copy link
Contributor

@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?
Webhooks management is laborious when notification need to reach individuals, or even any small number of channels.

Would this mean a different transport?

@fabpot
Copy link
Member

@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 :)

@birkof
Copy link
ContributorAuthor

@xavierbriand I think it is a misunderstood on your side, maybe because the title here.
This aims to replace legacy "Incoming Webhooks via web API" implementation with "Incoming Webhooks Slack app".

@birkofbirkof changed the title[Notifier][Slack] Send messages using Incoming Webhooks[Notifier][Slack] Send messages using Incoming Webhooks AppJun 5, 2020
@xavierbriand
Copy link
Contributor

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:

  • webhook (current implementation)slack://default/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX
  • web api:slack://xoxp-...@default/?channel=...

Having a username could be the trigger to switch from one method to the other.
Or using a different scheme together (e.g. slack-api) with one factory and two transports?

@birkof tell me what you think, I'll submit a PR.

@Nyholm
Copy link
Member

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. =)

@birkof
Copy link
ContributorAuthor

IMHO and how@Nyholm said, we shouldn't rely anymore on discontinued API.

fabpot added a commit that referenced this pull requestAug 16, 2020
…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
Copy link

Thank you for adding this! 🎉

@PythooonUser
Copy link

PythooonUser commentedJul 15, 2021
edited
Loading

I see no mention about this in the docs, however. It still refers to the old token authentication/setup.

@norkunas
Copy link
Contributor

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
Copy link

Thanks! Yeah, I found the corresponding PR#37138. Wasn't sure about the reasons for this revert, but now I know :)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@NyholmNyholmNyholm requested changes

@fabpotfabpotfabpot approved these changes

@OskarStarkOskarStarkOskarStark approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

@dunglasdunglasAwaiting requested review from dunglas

@jderussejderusseAwaiting requested review from jderusse

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

10 participants
@birkof@fabpot@xavierbriand@Nyholm@PythooonUser@norkunas@OskarStark@nicolas-grekas@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp