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

Allow passing Accept and User-Agent headers in notification webhook #2595

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

lowlyocean
Copy link
Contributor

Closes #2573

Copy link
Collaborator

@zagrim zagrim left a comment

Choose a reason for hiding this comment

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

Looking purely at the code (and referring to my test configs for memory refresher) left me with a question WRT to existing configs. Otherwise looks ok.

motioneye/config.py Outdated Show resolved Hide resolved
@MichaIng MichaIng linked an issue Sep 20, 2022 that may be closed by this pull request
motioneye/webhook.py Outdated Show resolved Hide resolved
@MichaIng MichaIng added this to the v0.43.0 milestone Sep 20, 2022
motioneye/config.py Outdated Show resolved Hide resolved
motioneye/config.py Outdated Show resolved Hide resolved
motioneye/webhook.py Outdated Show resolved Hide resolved
Co-authored-by: MichaIng <micha@dietpi.com>
@lowlyocean lowlyocean force-pushed the webhook_headers branch 3 times, most recently from 1111f52 to 5c1c16c Compare September 20, 2022 18:59
@lowlyocean
Copy link
Contributor Author

lowlyocean commented Sep 20, 2022

Even with changes we just agreed on, this won't be truly backwards compatible if someone does an in-place upgrade and already has an on_event_start webhook configured. That's because this will think their existing config is passing Accept and User-agent. The only way I see around it is if we re-write motion_camera_dict_to_ui() to not use utils.split_semicolon(on_event_start) but instead use argparse and make use of optional arguments (i.e. --useragent) instead of using positional arguments as we do today.

That said, I'm not really willing/able to put that much effort in right now for such a rewrite. Maybe a stop-gap in the meantime will be to not surround optional Accept and User-agent with single quotes while appending to on_event_start

@MichaIng
Copy link
Member

MichaIng commented Sep 20, 2022

flake8 is not happy:

motioneye/config.py:1290:1: C901 'motion_camera_dict_to_ui' is too complex (70)
def motion_camera_dict_to_ui(data):

Since we do not want a refactor the function in this PR, I suggest to ignore this particular error for now:

def motion_camera_dict_to_ui(data):  # noqa: C901

That's because this will think their existing config is passing Accept and User-agent.

I do not see the issue yet: Event actions (stored as a single line string in the camera config) are split with semicolon, the fields of each action are split with space. So with old configs, the event action will have three fields only, which are parsed correctly. Since the new options are not defined (or empty), the headers won't be added. But best is to simply test it. I'll probably not find time until Thursday since I have an evening event at work tomorrow.

@lowlyocean
Copy link
Contributor Author

lowlyocean commented Sep 20, 2022

Maybe a stop-gap in the meantime will be to not surround optional Accept and User-agent with single quotes while appending to on_event_start

I've added a commit with this, and tested that it's now truly backwards compatible. Hopefully the commit sufficiently illustrates what the problem was. Also note the change from 3/5 -> 7/9 is intentional, and UI doesn't parse config file correctly otherwise

I've also hopefully now satisfied flake8 with your suggestion

@MichaIng MichaIng linked an issue May 18, 2023 that may be closed by this pull request
@MichaIng
Copy link
Member

As there was another potentially related report #2704, lets have another look at this.

Actually, to not clutter the GUI too much, does it make sense to first try to pass a common user agent header by default?

@lowlyocean was the Accept header really required in your case? I generally would try to do things like curl, as it is to common for API requests and I never faced any issues without manually modifying the default headers it passes. And it does not send an Accept header, how should it even know what response to expect.

@lowlyocean
Copy link
Contributor Author

@lowlyocean was the Accept header really required in your case? I generally would try to do things like curl, as it is to common for API requests and I never faced any issues without manually modifying the default headers it passes. And it does not send an Accept header, how should it even know what response to expect.

Whether or not Accept (or other particular headers) are required may depend on the backend service. In my case, the webhooks are sent to an instance of node-red/node-red and Accept was required for that use case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Motion detection not calling webhook Webhook not being executed
3 participants