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

Inject request with specified the remote client IP address #36

Merged
merged 5 commits into from Aug 4, 2015

Conversation

@matteobaglini
Copy link
Contributor

matteobaglini commented Jun 25, 2015

fixes #35 issue

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Jun 25, 2015

@matteobaglini thanks for your PR. request.info is something that's found on the hapi request object.

The request in this module is supposed to simulate node's http.IncomingMessage. The ip address should therefore be on req.connection.remoteAddress.

@matteobaglini

This comment has been minimized.

Copy link
Contributor Author

matteobaglini commented Jul 9, 2015

@mtharrison I changed the implementation following your suggestion.

@mtharrison

This comment has been minimized.

Copy link

mtharrison commented on lib/index.js in 2c48bc0 Jul 17, 2015

I prefer remoteAddress as the option name.

This comment has been minimized.

Copy link

mtharrison replied Jul 20, 2015

127.0.0.1 would be a better default value.

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Jul 17, 2015

@matteobaglini Would you be able to update the readme too with this new option?

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Jul 17, 2015

@hueniverse This is going to break a bunch of hapi's tests, so am I right in assuming we:

  • Bump the major version of shot
  • Upgrade hapi's shot version and fix the tests over there
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 18, 2015

Why does it break existing tests? Why is it setting a default value of ''?

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Jul 20, 2015

It's due to a check here that bails out of processing a request on a stopped server if there's a connection property.

I'm guessing that checking for connection was a way of checking if the request is coming from Shot. A better way might be for us to use Shot.isInjection(request).

@matteobaglini

This comment has been minimized.

Copy link
Contributor Author

matteobaglini commented Jul 27, 2015

@mtharrison after you, or @hueniverse, released a new Shot version, I can help to fix Hapi's broken tests with a PR.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Jul 27, 2015

If that's the only change required, I would not consider it a breaking change.

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Jul 29, 2015

@hueniverse ah ok. I just spotted that hapi is linked to a specific version of Shot via the shrinkwrap. I wrongly assumed that releasing this under 1.x.x would break everyone's hapi.

In that case, we're good to release Shot 1.6.0, and update hapi later?

mtharrison added a commit that referenced this pull request Aug 4, 2015
Inject request with specified the remote client IP address
@mtharrison mtharrison merged commit c16f4cd into hapijs:master Aug 4, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mtharrison mtharrison added this to the 1.6.0 milestone Aug 4, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 4, 2015

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.