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

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

Open
mathieugalle opened this issue Mar 18, 2021 · 6 comments
Open

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

mathieugalle opened this issue Mar 18, 2021 · 6 comments

Comments

@mathieugalle
Copy link

mathieugalle commented Mar 18, 2021

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:

  • node-config version: 3.3.6
  • node-version: 10
  • docker toolbox 18.03
@markstos
Copy link
Collaborator

I agree a warning in this case could be helpful.

@lorenwest
Copy link
Collaborator

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
Copy link
Contributor

Did the parser rework alter this behavior?

@markstos
Copy link
Collaborator

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

@jdmarshall
Copy link
Contributor

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
Copy link
Contributor

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

util.locateMatchingFiles = function(configDirs, allowedFiles) {

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants