add ntfy plugin #524
add ntfy plugin #524
Conversation
|
Fantastic! |
|
This is a great first start guys! 🚀 I can see you're still fine-tuning your details. I'll pull my half of my promise and write the test cases once you're comfortable with what you've got. Please feel free to ask any questions you want; if you're stuck on how to do something, it's likely been done in another Notification service (so feel free to steal/copy paste). I can also help guide you to solve anything too if needed. Since this is web based and allows you to host your own server (with https/http), you might see a lot of commonality in Chris |
|
@caronc thanks for that! I was looking through a few of the other plugins and wondering which ones were probably closest. I'd never heard of RocketChat so I'll definitely take a look in there. |
Add a plugin for the [Ntfy](https://ntfy.sh) notification service. Supports topics, emojis, priorities, and tags.
particledecay
force-pushed
the
add_ntfy_plugin
branch
from
295e22d to
2573171
Compare
|
Ok this seems to work with topic, priorities, emojis, and tags. Good enough for a first pass I think, sorry for the delay 😄 |
Codecov Report
@@ Coverage Diff @@
## master #524 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 102 107 +5
Lines 13419 13905 +486
Branches 2465 2600 +135
==========================================
+ Hits 13419 13905 +486
Continue to review full report at Codecov.
|
apprise/plugins/NotifyNtfy.py
Outdated
| schema = 'https' if self.secure else 'http' | ||
|
|
||
| # Use default host if one is not defined | ||
| host = self.default_host |
There was a problem hiding this comment.
I don't know where else to comment, but this does not work as a host, but it should: ntfys://--joey, or ntfys://joey--
There was a problem hiding this comment.
What spec are you looking at for that? That would be a host of joey-- with no topic, which doesn't seem valid. In fact, as a shortcut, I currently have that going to the default ntfy.sh host with a topic of joey-- instead.
Either way, as a hostname it would be invalid according to RFC 952.
There was a problem hiding this comment.
It's not a hostname. It's a ntfy topic name.
ntfys://joey-- translates to https://ntfy.sh/joey-- which is a valid topic name. The spec is my regex of allowed topic names: https://github.com/binwiederhier/ntfy/blob/main/server/server.go#L58
I think you are right though that apprise is using a URI parser which doesn't recognize this as a valid host and doesn't allow it, since joey-- in the previous example would be parsed as a hostname.
There was a problem hiding this comment.
Hmm. I felt like it would be a nice shortcut, but it inadvertently restricts topic names to conform to hostname standards. I'll see if I can fix it, otherwise we may have to be explicit about host 😕
|
@caronc Hey I just tested this and it works well for the basic use case. How do we get this rolling and merged? What else do we need to do? :-D |
Add the ability to specify a 'click' option, 'delay', and 'email' for forwarding in ntfy plugin. All three are done via URL params.
|
@binwiederhier I added |
|
@particledecay For "upload this file" kind of attachments (https://ntfy.sh/docs/publish/#attach-local-file), we can skip that for now I think and tackle that in round two. |
|
You guys are really doing a great job; this is fantastic.. If you want to manipulate the headers, you could make it custom (or does the upstream plugin require it?)... For example for the # Below would set the header:
# X-Delay: value
#
# Assuming our {hostname} is localhost
# Assuming our {port} is 8080
apprise -vv -t "Test Message Title" -b "Test Message Body" \
"ntfy://localhost:8080/path/?+X-Delay=value"
# Multiple headers just require more entries defined:
# Below would set the headers:
# X-Delay: abcdefg
# X-Tags: awesome,sauce
#
# Assuming our {hostname} is localhost
# Assuming our {port} is 8080
apprise -vv -t "Test Message Title" -b "Test Message Body" \
"ntfy://localhost:8080/path/?+X-Delay=abcdefg&+X-Tags=awesome,sauce"Food for thought anyway... i can help you implement this as well while i add testing. Otherwise if you want to add support for something like |
Add the ability to attach files via URL to ntfy messages.
|
@binwiederhier added the @caronc is your offer to build tests still on the table? I think this is ready for testing. |
|
Of course, i wouldn't go back on my word. Is the foundation of what you're building what you want? |
|
@particledecay , i added some unit-tests (not done yet) and also re-factored your code a little bit.
Anyway; i still have a lot of test cases to add, but you can see a basis of what i've done. I'd be curious on your feedback. Chris |
|
That looks fantastic. Thank you both for working on this. :-D One question:
Could you explain to me where the ambiguity is? I thought |
|
To address your ambiguity question:
|
|
One question i have actually is that the code parses the JSON returned from a post to the ntfy server, but doesn't do anything with what it gets, was this intended? |
|
I can only comment on UX, not on the Python. I'll defer to @particledecay for the Python pieces. Multiple topicsIn ntfy itself I avoided having one message posted to multiple topics, because it messages with the rate limiting, but I think the way you implemented it (multiple POSTs) is fine. It seems to work nicely too. In ntfy, it is possible to SUBSCRIBE (but not publish) to multiple topics using the comma syntax: Topic names must match
|
|
Hey @particledecay, I'm very eager to get apprise support out, but I've been backing off intentionally to let you take the reigns. It has been very quiet though lately, so I was wondering if you wanted to let me work with @caronc to finish things up? My python fu is not strong, but I am pretty sure I'll manage, with the countless examples that apprise has. |
|
@binwiederhier Got to 100% test coverage today; i think we're ready for a merge once you approve that it works okay for you.
Yup, done and fully supported in this last few chunks of code i've pushed. So even a mix such as
Honestly don't worry about the Give it a try and let me know if you still have concerns; if not we can merge this into the master branch. great work all! |
| 'attach': { | ||
| 'name': _('Attach'), | ||
| 'type': 'string', | ||
| }, |
There was a problem hiding this comment.
We're missing Filename, which is the displayname/filename; it's fine if it's missing for now, but it should be easy to add.
See https://ntfy.sh/docs/publish/#attach-file-from-a-url and https://ntfy.sh/docs/publish/#list-of-all-parameters
There was a problem hiding this comment.
X-Filename has been added in the last check for Attachments. but for local file attachments as identified here. This still needs to be done.
I would advise maybe reviewing what is there and see if the rest is okay, and then we can start adding the --attach support to Apprise to push uploaded content next.
|
I read the code and tried everything out. Looks great. Apart from the one nitpick and the missing |
|
I made a small change to Same source though. Glad you're happy, I'll merge what we have and we can isolate any new bugs you find and fix them separately. A least you have a core foundation to work with now. The the only part of the code source as it is now that doesn't make sense is that after a post is successful to your API, a JSON response is checked, but nothing is done with the results (in Apprise). Can we drop this code completely? Or should we take action on specific return values? See here |
Excuse my ignorance, but would this be to attach local files (https://ntfy.sh/docs/publish/#attach-local-file)? Like
Yes you can remove that entirely, no problem. The error message is often useful though, and some errors do return a Example: |
|
Ship it ;-) |
|
Done! 🙂 |
Add a plugin for the Ntfy notification service.
Description:
Related issue (if applicable): #520
New Service Completion Status
keywordssection of thesetup()declaration%global common_descriptionChecklist
flake8)