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.

Sign up for GitHub

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

Jump to bottom

feat(notifications): Adds ntfy.sh notification option #787

Merged
merged 1 commit into from
Feb 19, 2023
Merged

feat(notifications): Adds ntfy.sh notification option #787

merged 1 commit into from
Feb 19, 2023

Conversation

blueberryapple
Copy link
Contributor

@blueberryapple blueberryapple commented Feb 10, 2023 •
edited by crazy-max
Loading

Description

Adds notification option for ntfy.sh

Context

fixes #569

@blueberryapple blueberryapple changed the title feat(notifications): Adds ntfh.sh notification option feat(notifications): Adds ntfy.sh notification option Feb 10, 2023
crazy-max
crazy-max requested changes Feb 10, 2023
View reviewed changes
Copy link
Owner

@crazy-max crazy-max left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your contrib, that looks great!

We should stay closed to the API spec of the notifier when naming inputs in Diun. Otherwise changes look good.

Also see my last comment about using a JSON representation. Can be done in follow-up.

internal/notif/ntfy/client.go Outdated Show resolved Hide resolved
internal/model/notif_ntfy.go Outdated Show resolved Hide resolved
internal/config/fixtures/config.validate.yml Outdated Show resolved Hide resolved
internal/config/fixtures/config.test.yml Outdated Show resolved Hide resolved
internal/config/config_test.go Outdated Show resolved Hide resolved
docs/notif/ntfy.md Outdated Show resolved Hide resolved
internal/model/notif_ntfy.go Outdated Show resolved Hide resolved
docs/config/index.md Outdated Show resolved Hide resolved
docs/notif/ntfy.md Outdated Show resolved Hide resolved
internal/notif/ntfy/client.go Outdated Show resolved Hide resolved
@blueberryapple
Copy link
Contributor Author

blueberryapple commented Feb 11, 2023 •
edited
Loading

Okay I addressed all the comments. @crazy-max this should be ready for review again

@crazy-max
Copy link
Owner

Looks great, thanks! Can you squash your commits please? I only use merge commit in this repo.

@blueberryapple
Copy link
Contributor Author

Looks great, thanks! Can you squash your commits please? I only use merge commit in this repo.

Okay the commits have been squashed

crazy-max
crazy-max reviewed Feb 19, 2023
View reviewed changes
docs/notif/ntfy.md Outdated Show resolved Hide resolved
crazy-max
crazy-max approved these changes Feb 19, 2023
View reviewed changes
Copy link
Owner

@crazy-max crazy-max 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!

@crazy-max crazy-max merged commit aaadc5d into crazy-max:master Feb 19, 2023
@adamantike
Copy link
Contributor

I have moved to the edge Docker tag to use this new feature. Looking forward to having this available in a new release tag! :)

@crazy-max crazy-max mentioned this pull request Jun 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazy-max crazy-max crazy-max approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ntfy.sh notifier
3 participants
@blueberryapple @crazy-max @adamantike