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

DOCS: Shoutrrr added list of notification services #3242

Merged
merged 9 commits into from
Dec 19, 2024

Conversation

cafferata
Copy link
Collaborator

@cafferata cafferata commented Dec 12, 2024

The documentation for notifications has been updated with several improvements, including prioritizing Shoutrrr as the preferred notification service, renaming “notification types” to “notification services,” and providing a JSON configuration example for Shoutrrr. Additionally, the full list of available notification services was added, and the “Future work” section was removed since Shoutrrr is now implemented. The introduction was rewritten to reflect these updates, and the JSON code example was enhanced with proper syntax highlighting.

You can check out a preview of these changes online at this link. I’d love to hear your thoughts and feedback!

cc: @hmoffatt

@hmoffatt
Copy link
Contributor

hmoffatt commented Dec 12, 2024

Good idea @cafferata . I considered adding the full list of services when I submitted the feature, but wasn't sure how much we wanted to duplicate their documentation (especially as they made add more services), but worth it I think.

Is it confusing that Slack/Mattermost and Teams are listed under both Shoutrrr and separately?

Minor bug: the section on Slack ends with a comma, possibly mid-sentence:

Configure slack_url to this webhook. Mattermost works as well, as they share the same api,

@tlimoncelli
Copy link
Contributor

Brilliant!

Yes, we should do this. Let's encourage people to use Shoutrrr.

If there are any methods that are supported by both the native dnscontrol and Shoutrrr, let's mark the native one as "legacy" and recommend the Shoutrrr version in the future. (In fact, maybe in the future the native methods should just call Shoutrrr?)

@cafferata
Copy link
Collaborator Author

Good idea @cafferata . I considered adding the full list of services when I submitted the feature, but wasn't sure how much we wanted to duplicate their documentation (especially as they made add more services), but worth it I think.

Thanks, @hmoffatt, for thinking along and sharing your perspective—it’s greatly appreciated!
I realize my GitHub pull request may not have been entirely comprehensive. The greatest value I see in including the complete Shoutrrr services list is that DNSControl users can find a notification platform directly in the DNSControl documentation without needing prior knowledge of Shoutrrr.

An example of a search term that currently doesn’t work but will after merging the GitHub pull request is:

This demonstrates how including the full Shoutrrr services list will make it easier for users to find relevant notification platforms directly from the DNSControl documentation.

Minor bug: the section on Slack ends with a comma, possibly mid-sentence:

Thanks for pointing that out, @hmoffatt—it’s much appreciated! However, this bug wasn’t introduced in my changes. See:

Additionally, there’s a second GitHub issue open where we’re discussing the potential removal of these sections entirely. See:

If there are any methods that are supported by both the native dnscontrol and Shoutrrr, let's mark the native one as "legacy" and recommend the Shoutrrr version in the future. (In fact, maybe in the future the native methods should just call Shoutrrr?)

Thanks for the suggestion, Tom—it’s a great idea! I propose we merge this GitHub pull request as is and continue discussing the details of your suggestion in GitHub issue #3243. Does everyone agree with this approach?

@hmoffatt
Copy link
Contributor

Both sound good, in terms of improved documentation and simplified code. (You still need to support Bonfire separately I guess.)

@cafferata
Copy link
Collaborator Author

That’s what I suspect as well. Hence my open question in GitHub issue #3243 about how @tlimoncelli envisions this, given that Bonfire is specific to Stack Overflow.

Copy link
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

Thanks for updating this file! I have a few minor suggestions.

One more suggestion: Change If you want to use the X integration, you need to to To use the X integration, (it is shorter and avoids using second person pronouns)

documentation/notifications.md Outdated Show resolved Hide resolved
documentation/notifications.md Outdated Show resolved Hide resolved
documentation/notifications.md Outdated Show resolved Hide resolved
@cafferata cafferata force-pushed the docs/notification-shoutrrr branch from 525e451 to f0255fe Compare December 19, 2024 14:44
@cafferata
Copy link
Collaborator Author

Resolved the merge conflict with #3248.

@cafferata
Copy link
Collaborator Author

One more suggestion: Change If you want to use the X integration, you need to to To use the X integration, (it is shorter and avoids using second person pronouns)

No problem. Please note that these texts are present in the main branch and were not introduced from this GitHub pull request. This, and the other, textual suggestions have been implemented in commit e5740df

@tlimoncelli
Copy link
Contributor

No problem. Please note that these texts are present in the main branch and were not introduced from this GitHub pull request. This, and the other, textual suggestions have been implemented in commit e5740df

Sounds good!

I've lost context on some of these PRs. Please be explicit about their status (ready for review, ready for merge). I don't want to jump the gun.

Thanks!
Tom (and happy holidays!)

@cafferata
Copy link
Collaborator Author

Please be explicit about their status (ready for review, ready for merge).

If my pull request is not ready for merge then it will be marked as a GitHub draft pull request. So to make it extremely expliciet: This pull request is ready for a (re)review and to be merged 🚀

@tlimoncelli
Copy link
Contributor

Thanks!

@tlimoncelli tlimoncelli merged commit 71539d4 into StackExchange:main Dec 19, 2024
6 checks passed
@cafferata cafferata deleted the docs/notification-shoutrrr branch December 19, 2024 16:58
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.

3 participants