-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Move webhooks to core #2376
Move webhooks to core #2376
Conversation
…stServeAction and this can be used
Do you plan to submit a documentation pull request before merging? It looks like there is quite a lot of updates needed there |
I guess I probably should 🙂 |
… Ubuntu due to a race condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two tiny queries but basically seems fine
ofType(WebhookTransformer.class).values().stream().collect(Collectors.toUnmodifiableList()); | ||
|
||
final Webhooks webhooks = new Webhooks(webhookTransformers, options.getProxyTargetRules()); | ||
loadedExtensions.put(webhooks.getName(), webhooks); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would of course overwrite any existing extension of name webhook
, but I guess you had thought of that and are OK with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea is that you shouldn't have to do it yourself any more.
It takes the same network rules as the proxy from the options object and there's a new extension point for WebhookTransformer
so no need to new it up yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably misunderstanding, but it looks like the WebHook will be sent twice per matched request? Once by this path:
Webhooks.doAction(ServeEvent, Admin, Parameters) (org.wiremock.webhooks)
StubRequestHandler.triggerPostServeActions(ServeEvent) (com.github.tomakehurst.wiremock.http)
StubRequestHandler.afterResponseSent(ServeEvent, Response) (com.github.tomakehurst.wiremock.http)
and once by this:
Webhooks.triggerWebhook(ServeEvent, Parameters) (org.wiremock.webhooks)
Webhooks.afterComplete(ServeEvent, Parameters) (org.wiremock.webhooks)
ServeEventListener.onEvent(RequestPhase, ServeEvent, Parameters) (com.github.tomakehurst.wiremock.extension)
StubRequestHandler.triggerListeners(RequestPhase, ServeEvent)(2 usages) (com.github.tomakehurst.wiremock.http)
StubRequestHandler.afterResponseSent(ServeEvent, Response) (com.github.tomakehurst.wiremock.http)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see - it's just two different ways of configuring the WebHook.
It looks like withServeEventListener
is now the preferred way, and withPostServeAction
is deprecated. And if so, should we document that there is a new preferred way and add an @Deprecated
annotation to MappingBuilder.withPostServeAction
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - the class is deprecated but not the method, so I'll do that in another PR.
… fail on Ubuntu due to a race condition" This reverts commit 3ad4d67.
…al memory races when reading/writing to it
…urns out they are legitimately async
Implements #2373.
Removes the webhooks extension as a separate sub-project and moves it to the core codebase.
Although it remains implemented as an extension, it is enabled at startup automatically so there's no need to explicitly enable it.
It now also implements
ServeEventListener
in addition toPostServeAction
so both the old and new forms can be used.