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

Added option to specify host address to bind servers to #37

Merged
merged 5 commits into from Jan 24, 2017

Conversation

Projects
None yet
3 participants
@tejashah88
Copy link
Member

commented Jan 21, 2017

This resolves issue #5.

README.md Outdated
port: 80,
// The host address in which the server should bind to. If not specified,
// this argument will be ignored.
host_address: undefined,

This comment has been minimized.

Copy link
@dblock

dblock Jan 21, 2017

Collaborator

I think the convention for this would be to be called host, cause you can also put a host name here, right?

This comment has been minimized.

Copy link
@dblock

dblock Jan 21, 2017

Collaborator

Should we just default this to 127.0.0.1 and avoid binding without a host name and therefore preventing remote access by default?

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 21, 2017

Author Member

I guess one thing to note is that is it a breaking change to some extent. If we look at the node docs for .listen here (express's app.listen does call http.listen), when you don't specify a hostname, it will try to find any IPv4/IPv6 address.

What we could do is either add a note about the recommended way to setup the server bindings or maybe deprecate the old style method in favor of the new one.

This comment has been minimized.

Copy link
@dblock

dblock Jan 22, 2017

Collaborator

For the purposes of this PR I agree not to change it, opened #38.

index.js Outdated
//TODO: add separate option to specify specific host address for HTTPS server to bind to ???
if (typeof config.host_address === 'string') {
self.httpsInstance = httpsServer.listen(config.httpsPort, config.host_address);
self.log("Listening on HTTPS port " + config.httpsPort + " on address " + config.host_address);

This comment has been minimized.

Copy link
@dblock

dblock Jan 21, 2017

Collaborator

Nitpick: maybe should just say something like "Listing on https://host:port", someone reading the log can just copy paste it into a browser.

index.js Outdated
self.log("Listening on HTTPS port " + config.httpsPort + " on address " + config.host_address);
} else {
self.httpsInstance = httpsServer.listen(config.httpsPort);
self.log("Listening on HTTPS port " + config.httpsPort);

This comment has been minimized.

Copy link
@dblock

dblock Jan 21, 2017

Collaborator

This would say "Listening on https://127.0.0.1:port or localhost ...

index.js Outdated
} else {
self.httpInstance = self.express.listen(config.port);
self.log("Listening on HTTP port " + config.port);
}

This comment has been minimized.

Copy link
@dblock

dblock Jan 21, 2017

Collaborator

Reading this, are we creating both an http and https endpoint? Shouldn't be just https if that's enabled?

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 23, 2017

Author Member

Not really, since the initial check already checks if https is enabled, and if so, will then proceed to initialize httpsInstance here. Otherwise, only the httpInstance should contain a server instance.

it("mounts hello world app (HTTP only)", function() {
testServer = alexaAppServer.start({
port: 3000,
host_address: "127.0.0.1",

This comment has been minimized.

Copy link
@dblock

dblock Jan 23, 2017

Collaborator

This should be host. The test isn't actually checking that these values were used, start the server with these and then compare them from the express instance.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 24, 2017

Author Member

I was going to test it via referring to the actual address (like http://127.0.0.1:3000 and https://127.0.0.1:6000) the express server is hosting to, since it's not possible to check for the IP from the express instance.

certificate: 'cert.cer'
});

return request(testServer.express)

This comment has been minimized.

Copy link
@dblock

dblock Jan 23, 2017

Collaborator

Just like above.

index.js Outdated
@@ -149,11 +151,13 @@ var appServer = function(config) {

self.start = function() {
// Instantiate up the server
//TODO: add i18n support (i18n-node might be a good look)

This comment has been minimized.

Copy link
@dblock

dblock Jan 23, 2017

Collaborator

Nitpick: personally I don't find these TODOs useful, because we have Github issues. If you want them there, maybe add a link to the github issue opened for it?

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 24, 2017

Author Member

I initially put the TODO's so that if anyone else is trying to fix any current issues, they know where in the code to look for. I like the idea of adding the links to the issues, so thanks!

index.js Outdated
try { // These two lines below can fail it the certs were generated incorrectly. But we can continue startup without HTTPS
var httpsServer = https.createServer(credentials, self.express); // create the HTTPS server

//TODO: add separate option to specify specific host address for HTTPS server to bind to ???

This comment has been minimized.

Copy link
@dblock

dblock Jan 23, 2017

Collaborator

Tiny annoying nitpick. Can we add a space after //, so // TODO:, hurts me every time :)

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Jan 24, 2017

Author Member

No problem!

@coveralls

This comment has been minimized.

Copy link

commented Jan 24, 2017

Coverage Status

Coverage increased (+0.2%) to 95.679% when pulling 76fec61 on tejashah88:master into f2bfb23 on alexa-js:master.

var alexaAppServer = require("../index");

// used so that testing https requests will work properly
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';

This comment has been minimized.

Copy link
@dblock

dblock Jan 24, 2017

Collaborator

I'll merge, however it feels like this should be set per-test, or in a beforeEach and then reset back in an afterEach.

@dblock dblock merged commit e2954da into alexa-js:master Jan 24, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 95.679%
Details
@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 24, 2017

Merged, thanks.

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