vbauer / manet

Website screenshot service powered by Node.js, SlimerJS and PhantomJS
MIT License
575 stars 102 forks source link

Configured host and port has priority over cloud ENV one #56

Closed jpraus closed 8 years ago

jpraus commented 8 years ago

Configured host and port has priority over cloud ENV one, this is imp…ortat if you want to run manet service as part of another service inside one container.

This change would currently break all manet instanced which are left with default config and are running inside cloud container. Proposed solution is to either comment HOST and PORT in default.yaml config to always load from cloud env first for current users or to introduce some config property that would force to use configured HOST and PORT instead of cloud-env one.

Why do I need this? Because I need to run manet inside a docker container which is a service on its own. And with current implementation it always maps out via mapped port. But I need my service to be mapped out and manet service to be hidden.

vbauer commented 8 years ago

I guess that I understand this problem.. but we can try to solve it in another way. At the current moment Manet has the following steps/priorities to load configuration parameters:

  1. Configuration file (low-priority)
  2. Environment parameters and command line arguments
  3. Cloud ENV parameters to work with IP & PORT.

config.js:

function load() {
    const confPath = defaultConfigPath(),
          config = nconf.argv()
            .env()
            .file({
                file: confPath,
                format: {
                    parse: yaml.safeLoad,
                    stringify: yaml.safeDump,
                }
            })
            .get();

    config.cache = Math.max(config.cache, 0);
    config.storage = path.resolve(config.storage || os.tmpdir());
    config.host = cloudEnv.get(ENV_IP, config.host);
    config.port = cloudEnv.get(ENV_PORT, config.port);
    config.path = confPath;

    return config;
}

I hope we can modify above code to something like this (I need to dig in nconf lib to find a way how to do it):

config = nconf.argv()
  .env()
  .cloudEnv() <- Something like this
  .file()

It will allow to override PORT & HOST arguments via CLI parameters. I think it should be enough. Am I right?

vbauer commented 8 years ago

I've just thought about another moment.. Maybe it will be better just to remove cloudEnv support. Anyway Procfile has configured port parameter.

UPD: I think we'll use you solution for now and maybe remove cloudEnv in future. Thank you for contribution!