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

Improve request context setting #91

Merged
merged 3 commits into from
Apr 27, 2018

Conversation

martin308
Copy link
Contributor

This was raised by @tremlab, the context from the request is not available in the middleware due it being set after the middleware has already been run. This fixes this issue by moving the setting of the context out of the middleware and moving it to being set when the request is updated in the event object.

This was raised by @tremlab, the context from the request is not
available due it being set after the middleware has already been run.
This fixes this issue by moving the setting of the context out of the
middleware and moving it to being set when the request is updated in the
event object
@martin308 martin308 requested a review from a team April 19, 2018 17:13
fractalwrench
fractalwrench previously approved these changes Apr 20, 2018
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

Lgtm. If you have mazerunner setup for this notifier then it may also be worth adding a scenario, so we have some additional coverage that isn't a unit test.

Add this method back and add a comment around it not being used and liable to be removed in the next major version bump
@martin308 martin308 merged commit 9dc3ad5 into master Apr 27, 2018
@martin308 martin308 deleted the martin308/improve-request-context-setting branch April 27, 2018 17:25
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.

None yet

2 participants