krakenjs / confit

Environment-aware configuration.
Other
61 stars 26 forks source link

Support deep property changes in env and argv overrides #52

Closed zetlen closed 8 years ago

zetlen commented 8 years ago

Confit allows command-line arguments and environment variables to take precedence over config files. However, as observed in https://github.com/krakenjs/confit/issues/42, it does not support setting complex properties. This change uses the existing object traversal code that confit already provides in its JavaScript API to read command-line arguments and environment variables. It enables this:

$ node app.js --a:b:c=d

The resulting overridden config value would be:

{
  "a": {
    "b": {
      "c": "d"
    }
  }
}
jasisk commented 8 years ago

I support the intended behavior however I'd prefer a different implementation.

I think a design I'd feel more comfortable with would be:

  1. break out the object traversal logic from config.set into a separate method
  2. call that method in the factory before merging the env and argv convenience methods into the store

@pvenkatakrishnan, what do you think?

zetlen commented 8 years ago

Thanks @jasisk, that's good advice. I updated the PR to do as you've described; a .toJSON method on Config objects is no longer necessary.

osukaa commented 8 years ago

Is this getting merged sometime soon? I'm having a similar issue but bourne shell uses colon for ENV variable expansion so it seems it would be better to define another character to split ENV variables.

zetlen commented 8 years ago

@osukaa I'd like to know too! Also, my tests found this to work fine in Bash; it doesn't seem to invoke the expansion meaning unless you're inside a ${ ... }, and it doesn't invoke the builtin : either...

jasisk commented 8 years ago

Thought about this a bit tonight. Realized this is already possible. --a.b.c=d will do what you want.

osukaa commented 8 years ago

@jasisk you mean overwriting nested properties in the config.json?

jasisk commented 8 years ago

Yup. You can access nested properties from command line arguments by separating them with a period. Note this only applies to command line arguments and not environment variables.

osukaa commented 8 years ago

@jasisk what could be a solution for env variables? But thinking of it, could be a bit weird

zetlen commented 8 years ago

I swear this didn't work before, @jasisk, but I just tried it again on our local kraken app, and it works now. Witchcraft!

jasisk commented 8 years ago

what could be a solution for env variables? But thinking of it, could be a bit weird

something similar to this PR, if we wanted a code change. The other option is something like this:

// config.json
{
  "DB_PASSWORD": "default value",
  "something": {
    "deeply": {
      "nested": {
        "db": {
          "user": "scruffmcgruff",
          "password": "config:DB_PASSWORD"
        }
      }
    }
  }
}
$ export DB_PASSWORD=60652
$ node thisUsesConfit.js
> // deeply.nested.db.password === "60652"
osukaa commented 8 years ago

Thanks!

theoutlander commented 7 years ago

FYI for others running into an issue like mine.

The original request was for

node app.js --a:b:c=d

But, the solution supports dots instead of colons:

node app.js --a.b.c=d

That makes more sense. I was just struggling for a while wondering why the override wasn't working as I kept trying colon separators.

tomalex0 commented 6 years ago

When i try to override c with new value for below config structure node app.js --a.b.c=g , property e is getting lost. Is this expected ?

Current

{
  "a": {
    "b": {
      "c": "d"
      "e" : "f"
    }
  }
}

Expected after node app.js --a.b.c=g

{
  "a": {
    "b": {
      "c": "g"
      "e" : "f"
    }
  }
}