Fixes #37667 - Provide meaningful error message on org/loc mismatch #78
Conversation
There was a problem hiding this comment.
We're running into this weird mismatch where webhooks are globally visible, but webhook templates are scoped into organizations and locations. This approach tips the scales towards the latter and treats this mismatch as an error state. An alternative could be to treat this as an expected state and silently do nothing instead of failing. Or we could lean more into global visibility point of view and disregard the template scoping.
@ofedoren opinions?
| @@ -4,6 +4,8 @@ module ForemanWebhooks | |||
| class EventSubscriber < ::Foreman::BaseSubscriber | |||
| def call(event) | |||
| ::Webhook.deliver(event_name: event.name, payload: event.payload) | |||
| rescue ::Foreman::Exception => exception | |||
There was a problem hiding this comment.
Without this, the stack trace appears in the logs. I don't think the stack trace is necessary.
There was a problem hiding this comment.
No strong opinion on this one, both ways are OK to me in this case.
In my opinion, providing an error message hinting on why the webhook failed to fire is much better than saying nothing and then users wondering what happened to their webhooks. |
adamlazik1
force-pushed
the
fix-37667-fix-error-message
branch
from
85014b4
to
a478414
Compare
|
Also 👮 |
There was a problem hiding this comment.
@ofedoren opinions?
It would certainly be more user friendly if we'd have unified scoping around webhooks and their templates. We could either introduce scoping for webhooks (that'd be another hot week within taxonomies' hell and will certainly introduce bugs or misbehaving, which I'd like to avoid unless necessary) or drop scoping from the templates. Not sure what it brings in terms of downsides, but if we'll unify, I'd vote for dropping.
But, the issue description is not about the idea of scoping, but more of adequate error propagation, which this PR solves well, let's stick with simplicity.
| @@ -4,6 +4,8 @@ module ForemanWebhooks | |||
| class EventSubscriber < ::Foreman::BaseSubscriber | |||
| def call(event) | |||
| ::Webhook.deliver(event_name: event.name, payload: event.payload) | |||
| rescue ::Foreman::Exception => exception | |||
There was a problem hiding this comment.
No strong opinion on this one, both ways are OK to me in this case.
adamlazik1
force-pushed
the
fix-37667-fix-error-message
branch
from
a478414
to
f8f11ef
Compare
Added meaningful error message when webhook template fails to render due to organization/location mismatch.
adamlazik1
force-pushed
the
fix-37667-fix-error-message
branch
from
f8f11ef
to
a2c5358
Compare
|
[test Redmine issues] |
|
Thank you @adamlazik1 & @ofedoren ! |
Added meaningful error message when webhook template fails to render due to organization/location mismatch.