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

[ntfy] Add dedicated service plugin ntfy #638

Merged
merged 6 commits into from
Apr 26, 2023
Merged

[ntfy] Add dedicated service plugin ntfy #638

merged 6 commits into from
Apr 26, 2023

Conversation

amotl
Copy link
Member

@amotl amotl commented Apr 20, 2023

About

While the previous integration of an adapter for ntfy was based on Apprise, this patch adds a dedicated service plugin instead.

In that way, mqttwarn will have better control about all the details. In a specific case (see references about Frigate), the current implementation had to be improved, in order to address attachments on the local filesystem, and upload them to the ntfy instance. This was not possible beforehand.

Documentation

You can inspect the documentation about the new plugin in its rendered version at mqttwarn notifier catalog » ntfy.

Details

Currently, Apprise does not offer to upload attachments from local files, and ntfy-wrapper only works on ntfy.sh, but not with on-premise installations. For those reasons, our implementation is solely based on requests now, and supports both options. It is thoroughly covered by corresponding software tests (100%).

References

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #638 (609d79f) into main (079ceeb) will increase coverage by 1.65%.
The diff coverage is 99.18%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
+ Coverage   42.44%   44.10%   +1.65%     
==========================================
  Files          83       84       +1     
  Lines        3854     3970     +116     
==========================================
+ Hits         1636     1751     +115     
- Misses       2218     2219       +1     
Flag Coverage Δ
unittests 44.10% <99.18%> (+1.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mqttwarn/services/apprise_multi.py 95.45% <ø> (ø)
mqttwarn/services/apprise_single.py 95.34% <ø> (ø)
mqttwarn/services/apprise_util.py 100.00% <ø> (ø)
mqttwarn/services/ntfy.py 99.13% <99.13%> (ø)
mqttwarn/commands.py 100.00% <100.00%> (ø)
mqttwarn/core.py 92.09% <100.00%> (ø)
mqttwarn/model.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

targets = {
'test': {
'url': 'https://ntfy.sh/testdrive',
'attach': 'https://unsplash.com/photos/spdQ1dVuIHw/download?w=320',
Copy link
Member Author

Choose a reason for hiding this comment

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

attach is the standard ntfy field to attach a file from a URL.

targets = {
'test': {
'url': 'https://ntfy.sh/testdrive',
'attachment': '/tmp/ntfy-attachment-{slot}-{label}.png',
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've introduced the attachment option/field to signal to mqttwarn that it should attach a local file to the ntfy notification, in order to support @sevmonster's use case.

However, I do not like the naming yet, as it's easy to confuse with the attach field. We should choose another name. I am leaning towards just using file instead. Do you have any other suggestions?

Copy link
Member Author

@amotl amotl Apr 21, 2023

Choose a reason for hiding this comment

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

I've renamed the option to file with 40e2efd, as proposed. I think it's a better choice to avoid confusion.

Comment on lines 1777 to 1783
:::{important}
[ntfy publishing options] outlines different ways to marshal data to the ntfy
HTTP API. mqttwarn is using HTTP headers for serializing values, because the
HTTP body will already be used for the attachment file. Because of this, you
are not able to use UTF-8 characters within your message text, they will be
replaced by placeholder characters like `?`.
:::
Copy link
Member Author

@amotl amotl Apr 21, 2023

Choose a reason for hiding this comment

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

mqttwarn is using HTTP headers for serializing values. Because of this, you are not able to use UTF-8 characters within your message text, they will be replaced by placeholder characters like ?.

@binwiederhier outlined a potential solution for this slight drawback, and figured it would be trivial to implement on behalf of ntfy, see caronc/apprise#866 (comment). 🌻

Choose a reason for hiding this comment

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

As I said in the other comment; ntfy server does support UTF-8 in headers, so if you send valid UTF-8 in headers it'll work. But because the HTTP spec does not officially support UTF-8, your libraries or language may not support it.

The suggested RFC encoding would fix this, obviously.

Copy link
Member Author

@amotl amotl Apr 21, 2023

Choose a reason for hiding this comment

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

Hi Philipp.

ntfy server does support UTF-8 in headers, so if you send valid UTF-8 in headers it'll work.

Ah. Thanks for mentioning it once again.

But because the HTTP spec does not officially support UTF-8, your libraries or language may not support it.

Right, the Python client libraries would currently already croak on that detail, raising a corresponding exception. I will see if a workaround can be applied in form of a monkey patch or such.

But we agree it's dangerous, because there may be HTTP intermediaries in between, which might scramble those headers, right?

Encoding the headers using RFC 2047 would fix this, obviously.

I will very much appreciate such an improvement to ntfy. Until it will converge, I will eventually look into the monkey patch solution, or just let it sit like it is.

Thank you so much for your swift responses, and keep up the spirit.

With kind regards,
Andreas.

Choose a reason for hiding this comment

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

Works wonderfully: binwiederhier/ntfy#707

Choose a reason for hiding this comment

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

It's merged and will be in the next release

Copy link
Member Author

Choose a reason for hiding this comment

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

That was fast. Thank you very much!

Copy link
Member Author

@amotl amotl Apr 23, 2023

Choose a reason for hiding this comment

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

I've adjusted the client-side implementation correspondingly with 609d79f. Thank you again!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi again,

at 609d79f#r110220447, you've commented:

I didn't do tags. Only title and message. Though tags are probably a good idea too.

This has already been added with binwiederhier/ntfy@59a5077713 now - thank you so much! I've complemented this improvement with b67e390, where RFC 2047 encoding will now only be applied to specific fields, title, message, and tags.

When looking through the available fields once more, I only discovered that the Action's label value might be another candidate for being encoded with RFC 2047, in order to use UTF-8 characters on the action buttons, when using HTTP header transport mode. But please consider this only as a discovery on my end, I personally do not have a need for that.

Thanks again for the quick turnaround on this matter.

With kind regards,
Andreas.

Copy link
Collaborator

@jpmens jpmens left a comment

Choose a reason for hiding this comment

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

Looks good to me

@caronc
Copy link

caronc commented Apr 22, 2023

Just seeing this now, in regards to this:

address attachments on the local filesystem, and upload them to the ntfy instance. This was not possible beforehand.

Apprise does support local attachments to Ntfy already. I just wanted to chime in should i need to fix a bug of my own. Is this not working anymore?

Also, in another thread, i saw a discussion about renaming attach to file in the ntfy payload. Is this the case? Or will both keys work for backwards compatibility? Or most likely i just misinterpreted what was being said here🙂

@amotl

This comment was marked as duplicate.

@amotl
Copy link
Member Author

amotl commented Apr 23, 2023

Dear Chris,

thank you for chiming in. I've responded over at caronc/apprise#866 (comment), because I think it fits the context better, and otherwise the relevant information would be missing from that discussion. TLDR; a) I don't think Apprise currently supports local attachments, but only remote attachments of the ntfy API. b) Using a file field is exclusively related to the mqttwarn integration, nothing changed on the ntfy API.

With kind regards,
Andreas.

Only apply the encoding to the values of ntfy option fields `title`,
`message`, and `tags`. Strip non-ASCII characters from all others.
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.

5 participants