search

node-config / node-config

Node.js Application Configuration
Other
6.27k stars 498 forks source link

Add a warning when HOSTNAME is "default" ? #644

Open mathieugalle opened 3 years ago

mathieugalle commented 3 years ago

Hi,

I'm submitting a feature request / surprising behavior report. So, here's the story :

I was working locally with a local-dev NODE_ENV, everything was working as expected : node-config was using the local-dev.json file values. Because this micro-service runs on a docker image, I tried to run my project locally in a Docker image. So, after building the image, I simply launched as usual : docker run -e "NODE_ENV=local-dev" mycompany/project:master

But it seems that on Windows, when using "Docker toolbox" with Virtualbox (because of some old incompatibility between docker and Virtualbox), docker sets the docker image HOSTNAME as default. I checked this with docker exec.

And when I finally tested with docker run --hostname not-default -e "NODE_ENV=local-dev" mycompany/project:master, node-config was indeed using my local-dev.json

I did not know at first that hostnames were used to set the configuration file used by node-config, and with a higher priority. It took some time to understand this problem. And there is no mention anywhere on the Docker side that the default HOSTNAME was default.

So, I am describing this problem here in case it could help someone.

I think that having some kind of warning when the HOSTNAME is default could have helped here, but I don't know if this kind of warning is common or not. After all, everything worked correctly here, and my setup (using docker toolbox) is now obsolete.

What do you think ? I can propose a pull-request if people think it's worth it.

Thanks for reading !

Please tell us about your environment:

markstos commented 3 years ago

I agree a warning in this case could be helpful.

lorenwest commented 3 years ago

Agreed. I'd support a PR with this warning. It seems like an edge case so it won't pollute a lot of people's logs. Once per program execution, so low impact even if we missed something.

jdmarshall commented 1 year ago

Did the parser rework alter this behavior?

markstos commented 1 year ago

@jdmarshall I'm not sure what you are referring to, but you are welcome to check.

jdmarshall commented 1 year ago

Well we run in docker quite a bit and I don’t recall running into this problem with docker desktop. The parsing rework collects the names and then loads them, so I’m wondering if that has changed the load order, since with the list you can see if you’re referring to something that is already going to be loaded.

jdmarshall commented 1 year ago

While working on something else this week I found that on OS X, docker desktop sets host name to the container ID. This does seem to be peculiar to Windows, or perhaps the specific configuration Mathieu mentions.

I think this problem could be fixed in https://github.com/node-config/node-config/blob/4c1c6193903bdb8e477792bec9dd852f7508dbcf/lib/config.js#L725

And while I’m looking at this function , I’m not entirely sure the sort() is appropriate to the semantics of load precedence here. Won’t that cause the incorrect load order for some machine and environment names?

One option would be to use a set for locatedFiles, but I’m not sure what behavior people would expect if hostname or environment name is default or local.