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.

Sign up for GitHub

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

Jump to bottom

Allow to use NEEO_SERVER_IP, NEEO_SERVER_BASEURL, NEEO_SERVER_PORT as Environment Variable #171

Closed
wants to merge 3 commits into from
Closed

Allow to use NEEO_SERVER_IP, NEEO_SERVER_BASEURL, NEEO_SERVER_PORT as Environment Variable #171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ecd578
Allow to use BASEURL or ADAPTERIPADRESS as Environment Variable & Ada…
splattner Dec 30, 2018
76d6ed4
Allow ADAPTERPORT to be specied as env variable
splattner Dec 30, 2018
b8b1299
Make it consistent with neeo/cli
splattner Dec 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cli/devicecontroller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@ function storeSdkServerConfiguration(brain, sdkOptions, devices) {
const { serverPort, serverName } = sdkOptions;
serverConfiguration = {
brain,
port: serverPort || 6336,
port: serverPort || process.env.NEEO_SERVER_PORT || 6336,
name: serverName || 'default',
adapterIpAddress: process.env.NEEO_SERVER_IP,
baseurl: process.env.NEEO_SERVER_BASEURL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why the baseurl variable is needed here? it doesn't seem used below and the url is generated from the IP address.

Copy link
Author

Choose a reason for hiding this comment

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

in https://github.com/NEEOInc/neeo-sdk/blob/master/lib/device/index.js#L48 its only generated when conf.baseurl is not set. With this change, you can set conf.baseurl with an ENV

Generating the baseURL by IP Address might not always work (e.g. behind NAT), I even would suggest to not build at all a baseURL that needs to be send to the Brain. But thats an other Topic and probably a bit more tricky.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh OK makes sense, I didn't see it in the PR and I was confused, my bad.

Copy link
Author

Choose a reason for hiding this comment

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

it's not in the PR :) it was already there. np 👍

devices,
};
}
Expand Down
2 changes: 1 addition & 1 deletion lib/device/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ function generateAdapterName(conf) {
}

function generateBaseUrl(conf) {
const ipaddress = validation.getAnyIpAddress();
const ipaddress = conf.adapterIpAddress || validation.getAnyIpAddress();
const baseUrl = 'http://' + ipaddress + ':' + conf.port;
debug('Adapter baseUrl %s', baseUrl);
return baseUrl;
Expand Down
4 changes: 4 additions & 0 deletions test/unit/cli/devicecontroller_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ describe('./cli/devicecontroller.js', function() {
},
port: 6336,
name: 'default',
adapterIpAddress: undefined,
baseurl: undefined,
devices: [device],
});
});
Expand All @@ -95,6 +97,8 @@ describe('./cli/devicecontroller.js', function() {
},
port: 6336,
name: 'default',
adapterIpAddress: undefined,
baseurl: undefined,
devices: [device],
});
});
Expand Down