search

NEEOInc / neeo-sdk

NEEO Brain SDK
https://neeoinc.github.io/neeo-sdk/
MIT License
48 stars 17 forks source link

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

Closed splattner closed 10 months ago

splattner commented 5 years ago

My suggestion for #170

This allows to set the full BASEURL via Environment Variable or just the ADAPTERIPADDRESS and then let the SDK build the baseurl.

splattner commented 5 years ago

I guess I should move part of this into the neeo-sdk-toolkit?

splattner commented 5 years ago

I also added ADAPTERPORT as Env Variable, so I can set this when running multiple Docker Container with NEEO SDK Devices

splattner commented 5 years ago

ok, I just figured out, that I probably should use the neeo-sdk-toolkit to run multiple Devices in one Container. But the Problem with the wrong IP Address taken for the baseURL still exists there. Therefore my 2 PRs:

MichaelKohler commented 5 years ago

Can you send us also the Dockerfile you are using so we can reproduce this? Also, how do you run Docker and on which OS?

splattner commented 5 years ago

Sure, check https://github.com/splattner/neeo-driver-googlecast

FROM node:latest

WORKDIR /app
COPY package.json /app
RUN npm install

COPY . /app
EXPOSE 6336

CMD ["npm","start"]

or by using the newer @neeo/cli

[...]
CMD ["npx","neeo-cli","start"]

I run Docker on my Ubuntu 18.04 and on my Synology NAS

docker build -t splattner/neeo-driver-googlecast . docker run --rm --name neeo-driver-googlecast --network host -e "BRAINIP=192.168.10.211" -e "DEBUG=*" splattner/neeo-driver-googlecast

The Problem is generateBaseUrl in lib/device/index.js or better getAnyIpAddress in lib/device/validation/iphelper.js which does return the Containers eth0 Adress when run in Bridge configuration (which is obviously not reachable from the Brain) or returns the first non internal IPv4 Address when run in Host-Network Configuration (in my case this was vboxnet0 which is also not reachable from the Brain)

with my PR's you can use NEEO_SERVER_IP to override the getAnyIpAddress Function or use NEEO_SERVER_BASEURL to completly override the baseURL and don't use the generateBaseUrl Function.

pfiaux commented 5 years ago

I see the use case for this, that makes sense. As you mentioned if we go this way it's more important to add this in the neeo-sdk-toolkit/cli since it will replace this neeo-sdk cli (which will eventually be removed).

I'm wondering if we need both adapterIpAddress and baseurl, only 1 might be enough and will keep the interface more concise.

The end goal for you is mainly to use the SDK within docker, correct?

splattner commented 5 years ago

I see the use case for this, that makes sense. As you mentioned if we go this way it's more important to add this in the neeo-sdk-toolkit/cli since it will replace this neeo-sdk cli (which will eventually be removed).

See https://github.com/NEEOInc/neeo-sdk-toolkit/pull/5 I made the modifications there, but it requires some changes from this PR (the change in lib/device/index.js) In the neeo-sdk-toolkit the ENV Handling is anyway nicer! Part of this PR is just for Backwards Compatibilty.

I'm wondering if we need both adapterIpAddress and baseurl, only 1 might be enough and will keep the interface more concise.

  • the IP is easier to use (don't need to build URL), but for some edge cases having both is more flexible
  • From the SDK perspective, they overlap adding complexity to the API, possible confusion over which to use when, and we'll have to maintain both (it's easier to add than remove without breaking changes)

I agree, but as the baseURL was already there as a SDKOption, I wanted to leave it as it is (and just add the Possibility to set it via ENV). The Reason for adding the The NEEO_SERVER_IP is that the generateBaseURL uses the correct Port for building the URL, whereas if you use NEEO_SERVER_BASEURL you have to be aware of the correct Port.

  • As a third alternative we might also be able to improve iphelper to cover the docker use case (and use neither)

The end goal for you is mainly to use the SDK within docker, correct?

Yes, at least for me it is, but also on Devices with multiple Network Interface (e.g. from a Viritualization Network, Virtualbox or similar)