Skip to content
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

fix(notifications): include icon in slack legacy url #1303

Merged
merged 2 commits into from
Aug 1, 2022
Merged

fix(notifications): include icon in slack legacy url #1303

merged 2 commits into from
Aug 1, 2022

Conversation

Choromanski
Copy link
Contributor

Modified parsing of discord URLs:

  • The previous implementation assumed that the discord URL had a trailing path argument in the URL path
    • Ex: https://discordapp.com/api/webhooks/WEBHOOK_ID/TOKEN/slack
  • However discord generates webhook URLs without the trailing path argument
    • Ex: https://discordapp.com/api/webhooks/WEBHOOK_ID/TOKEN
  • Seeing as the /slack argument is never parsed by watchtower or sent to shoutrrr it should not be a requirement
  • This PR addresses this issue by supporting URLs of both types:
    • https://discordapp.com/api/webhooks/WEBHOOK_ID/TOKEN/slack and https://discordapp.com/api/webhooks/WEBHOOK_ID/TOKEN

Added support for the --notification-slack-icon-url argument when using a discord webhook:

  • Previously this argument did nothing when using a discord webhook URL
  • Now it is parsed as the Avatar in the shoutrrr discord config

Added two tests for my additions

  • One to test the parsing of the WEBHOOK_ID and TOKEN when there is no trailing path argument in the discord webhook URL
  • One to test the parsing of the --notification-slack-icon-url argument with a discord webhook URL
    • This also tests the parsing of the --notification-slack-identifier argument with a discord webhook URL (not previously tested)

@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #1303 (37f2b52) into main (e983beb) will increase coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1303      +/-   ##
==========================================
+ Coverage   63.65%   63.86%   +0.21%     
==========================================
  Files          23       23              
  Lines        1527     1536       +9     
==========================================
+ Hits          972      981       +9     
  Misses        465      465              
  Partials       90       90              
Impacted Files Coverage Δ
pkg/notifications/slack.go 95.12% <100.00%> (+0.25%) ⬆️
pkg/filters/filters.go 83.63% <0.00%> (+2.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e983beb...37f2b52. Read the comment docs.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

The previous implementation assumed that the discord URL had a trailing path argument in the URL path
Ex: https://discordapp.com/api/webhooks/WEBHOOK_ID/TOKEN/slack

Yes, this is because that is the slack API emulation endpoint for discord. Using the regular discord webhook API with a slack client would not work.
Proper discord support is added through shoutrrr, and this is only a helper that creates a shoutrrr URL from the arguments. It should behave as the original implementation.

Added support for the --notification-slack-icon-url

This was actually an unintended regression, so this should probably be added.

@Choromanski
Copy link
Contributor Author

Looking through the parsing of the hookURL in the GetURL function the /slack endpoint is never parsed and therefore makes no difference when it is present or not. I however have not looked through the shoutrrr source and would not be surprised if shoutrrr is adding the /slack endpoint back in.

I made this change to help simplify discord notification integration. IMHO when you are given a webhook URL you shouldn't have to modify the URL to use it.

The lack of documentation when it comes to using discord notifications requires digging into the source to determine that the webhook URL needs an extra endpoint (actually any endpoint works ex. /anything). Just trying to make it more friendly to individuals not willing to dig into the source.

I do plan on adding documentation for discord notifications in the next week.

I have tested this PR both with and without the /slack endpoint and can verify that it functions the same either way.

@piksel
Copy link
Member

piksel commented Jun 9, 2022

The lack of documentation when it comes to using discord notifications requires digging into the source to determine that the webhook URL needs an extra endpoint [...]

That's because the discord-over-slack-API is not recommended, nor supported. We only added the helper so that it wouldn't break the current users use-cases.

I however have not looked through the shoutrrr source and would not be surprised if shoutrrr is adding the /slack endpoint back in.

No, it doesn't, because it's got native discord support. It's not using the slack-emulation in the discord API.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@piksel piksel changed the title Discord notification enhancements fix(notifications): include icon in slack legacy url Aug 1, 2022
@piksel piksel merged commit 489356a into containrrr:main Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants