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

Fix for 557: Provide request headers in various payload events #562

Merged
merged 6 commits into from Aug 23, 2017

Conversation

@bodawei
Copy link
Contributor

bodawei commented Jul 24, 2017

This is a draft fix for #557 . In particular, this demonstrates what I have in mind there. Happy to discuss this in more detail.

@bodawei

This comment has been minimized.

Copy link
Contributor Author

bodawei commented Jul 27, 2017

I noticed that some of the Travis CI build failed. I don't get these same problems on my local system (notably, one of these seems to be missing some module (!), and others are about a "leak" of WebAssembly). Any insights into what is up with this?

@bodawei bodawei changed the title Draft fix for 557: Provide request headers in various payload events Fix for 557: Provide request headers in various payload events Jul 28, 2017
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Aug 5, 2017

Yes. An update to lab fixes the memory leak issue (it's not really a leak btw, just a global variable). Missing modules I'm not sure about... I haven't seen that locally or in any of my Travis runs. There are still failing tests because, surprise surprise, things changed in the stream API so it doesn't act the same in the 4 versions of node good is tested against.

@bodawei

This comment has been minimized.

Copy link
Contributor Author

bodawei commented Aug 7, 2017

I looked closer and noticed that all the errors in this build are the same as the ones from #553. Given that that is merely a .md file change, seems like it's not likely my change caused these. So, this change seems good to go to me. Let me know!

Copy link
Contributor

arb left a comment

Also, rebase in master, tests should be green now.

@@ -143,6 +147,10 @@ class RequestLog {
this.method = request.method;
this.path = request.path;
this.config = internals.extractConfig(request);

if (reqOptions.headers) {
this.headers = request.raw.req.headers;

This comment has been minimized.

Copy link
@arb

arb Aug 10, 2017

Contributor

Just to be safe incase this isn't there, use hoek.reach

@@ -1,7 +1,7 @@
{
"name": "good",
"description": "Server and process monitoring plugin",
"version": "7.2.0",
"version": "7.3.0",

This comment has been minimized.

Copy link
@arb

arb Aug 10, 2017

Contributor

Undo.

Deej Burrowes added 4 commits Aug 14, 2017
Deej Burrowes
…again. hopefully it won't be cancelled by something else this time
@arb arb modified the milestone: 7.3.0 Aug 23, 2017
@arb
arb approved these changes Aug 23, 2017
@arb arb merged commit 549662a into hapijs:master Aug 23, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.