Skip to content

[Notifier] Add better example for Slack DSN #14599

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

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Nov 23, 2020

Slacks API documentation is a mess!

This small change would make it more clear that one should use their webhook URL's parameters.

@@ -149,6 +149,7 @@ Chatters are configured using the ``chatter_transports`` setting:

# .env
SLACK_DSN=slack://default/ID
SLACK_DSN=slack://default/XXXXXXXXX/XXXXXXXXX/XXXXXXXXXXXXXXXXXXXXXXXX
Copy link
Member

Choose a reason for hiding this comment

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

This might be better than the current situation ... but to me it's the same. How can anybody understand what XXXX means? Maybe we need to add a super short comment before each SLACK_DSN example explaining what's the ID and those XXXXX. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this comment is helpful (PR updated)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about something like this:

# .env
# when using the Slack channel ID
SLACK_DSN=slack://default/ID
# when using Slack webhooks (https://hooks.slack.com/services/T.../B.../X...)
SLACK_DSN=slack://default/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX

I don't know if the ID is the channel ID; it's only an example. Also, I think we should use the same pattern as the Slack API docs for the different webhook parts. See https://api.slack.com/messaging/webhooks

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I think slack://default/ID belongs to a deprecated API that we've removed support for. But Im not sure.

If Im correct, then slack://default/ID cannot be used..

Copy link
Contributor

Choose a reason for hiding this comment

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

It took me hours to get it running using an incoming Webhoook like this:

https://github.com/sonata-project/dev-kit/blob/master/.env.dist

Copy link
Member

Choose a reason for hiding this comment

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

The ID here refers to the T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX bit, which is the confusing part here.

(and btw, I agree that the Notifier docs are quite a mess. The component evolved a lot after its initial launch, the original document structure no longer fits the broad set of chatters and their different features)

@javiereguiluz
Copy link
Member

javiereguiluz commented Nov 25, 2020

Tobias thanks for improving this. Hopefully it will avoid the confusion that many have faced so far with these docs.

@javiereguiluz javiereguiluz merged commit 1b6b814 into symfony:5.1 Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants