bazwilliams / node-upnp-subscription

Node library to manage UPNP subscriptions, renewals, expiry and unsubscribe
MIT License
4 stars 1 forks source link

User should be able to specify a port for the HTTP server to listen on. #6

Open taylorzane opened 8 years ago

taylorzane commented 8 years ago

Issue

A user should be able to specify a port number as an env variable (PORT) to the library in order to always get the same HTTP port.

Pros

Implement handling to accept an env variable PORT that will be the port that the HTTP server listens on.

Optionally, provide a way to permit (or deny) the "auto-port-search" behavior through another env variable ({PERMIT,DENY}_AUTO_FIND_PORT=true).

For clarification, the "auto-port-search" behavior will be enabled when no PORT env variable is passed, to provide backwards compatibility with previous versions of the library.


NOTE: I am completely willing to make this implementation, and I may start prior to getting a response, as it is somewhat critical to my using of this library in a Docker container. But I would like to hear opinions on the best way to implement this.

taylorzane commented 8 years ago

I made an initial attempt (6e1a27) to solve this problem. So far it solves all problems, as laid out in the initial issue. I won't make a pull request until we've at least discussed this issue, in order to lay out how we want it to officially work.

A question that may arise, that I'll answer preemptively, is the double defined env variables (ones with and without the library name prepended in all caps). I've done this so that one can easily specify the variables if running this as a single instance (i.e. from the command line) or so that it may be run using an environment configuration file, as to prevent any collisions with other variables.

bazwilliams commented 8 years ago

Hi,

Sorry for the delay in responding! Yes I agree this would be useful, I do use this library inside another Docker, but have avoided the issue through the use of --net=host.

I'd prefer a route which is either: a) Request a port, the application throws an exception if busy. b) Don't request a port, the application picks a port using the port library.

I'm struggling to think of a situation where you'd have a preferred port but happy to use another.

My first thought was along the line of an init() method which could take a configuration object. e.g

const Subscription = require('node-upnp-subscription')({ port: 54317 });

let sub = new Subscription(uri);
taylorzane commented 8 years ago

You're right. There isn't really any case that I could think of that would be acceptable for a "preferred, but okay choose at random" port. I just figured I'd cover all bases. I can remove that check from the code.

An init method could work. I'd have to see how that ties into dealing with the async nature of the port finding.

bazwilliams commented 8 years ago

I think the init will work with the async code you already put in place. The subscriptions wouldn't start until the server is up.

Reason behind init() is because I'd rather not force environment variable configuration at library level. Most 12 factor code I've written creates a config object from environmental variables using the 12 factor library which provides additional checks.

taylorzane commented 8 years ago

Fair enough, I need to look into the 12f design principles a bit more. But what you're saying makes sense. I'll go ahead and make those changes today. Is there anything else you can thing of before I turn this into a PR?