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

supporting multiple subdomains #676

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TeleMediaCC
Copy link

Please create a new release with my supporting multiple subdomains patch
Thanx!


NODE_ENV.forEach(function (env) {
// Backward compatibility
baseNames.push(subdomain, subdomain + "-" + env);
Copy link
Contributor

Choose a reason for hiding this comment

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

so for a machine named server.foo.bar.com, we used to have server-prod.json added to the list, if hostname != 'server'.

If I recall how this code works, each entry earlier in the list is overridden by entries later on, and if so then this is not queueing them in the correct order.

I would expect files that specialize by domain to load in the following order:
com-prod.json
bar-com-prod.json
foo-bar-com-prod.json
server-foo-bar-com.prod.json

I don't know when server-prod.json should load in that sequence, but in the original code it applies last so that probably should be preserved, and I don't think you have the loop structured properly to insist upon that.

I'm wondering what use case you're covering here. Why would I have two machines with the same name in the same environment with different domains? Pet servers with isolation per customer? I think that's still covered by the ordering I proposed above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also wondering about the use-case. Please update the pull request description to explain in what cases this would be used and why the existing features of supporting multiple config files don't already address the case well.

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

Successfully merging this pull request may close these issues.

None yet

3 participants