ENV variables should be reusable. about node-convict

[twitchard]

Gah I'm so sorry. I was writing a description, but then thought better of my idea halfway through writing it. Didn't realize the issue had been created.

What it was: I was a little surprised to see

Error: 'foo.enable_debug' reuses an env variable: ENABLE_DEBUG.

when I tried to do

var config = convict({
    foo: {
        enable_debug: {
            doc: "Enable debug for the foo service",
            format: Boolean,
            default: true,
            env: "ENABLE_DEBUG"
        }
    },
    bar: {
        enable_debug: {
            doc: "Enable debug for the bar service",
            format: Boolean,
            default: true,
            env: "ENABLE_DEBUG"
        }
    },
})

What I initially expected was for this to be accepted, and both foo.enable_debug and bar.enable_debug to be set to the value of process.env.ENABLE_DEBUG if it was set. In a sense, that seems more natural. (And for my use case, more convenient.) But after thinking about it, I see the merits of enforcing one setting per env variable. Somebody might be surprised if you changed one environment variable and this affected multiple settings.

Anyway, if you are not opposed to the behavior I have described, I would gladly add a pull request. If not, it would probably be a good thing to document in the README.md. (only one setting allowed per env variable)

[madarche]

Fixed by #189

[vladikoff]

what does this mean?

[jcollum]

Any movement on this? I think env variable reuse should be an overridable flag.

[madarche]

@twitchard I didn't know about this behavior: sure at least this should be documented in the README and should also be unit tested (I haven't found any test about this behavior - just a quick search).

@twitchard @jcollum I'm personaly not against and opt-in making it possible to reuse env variables. A PR on this could be accepted if the code is clean, doesn't add to the complexity of Convict, and with a matching unit test. At the moment the only time we pass options is in the validate method. So, to be accepted, a PR should provide a nice/easy/intuitive way to pass this option. So should we move this env variable reuse enforcement/check only in the validate method? Please elaborate on this point. Thanks

[madarche]

You might be interested by new options available for the validate method that has just landed https://github.com/mozilla/node-convict#configvalidateoptions with #182 as it seems a good place to deal with options. I'm not saying it's the way to go, just that this is a possible choice. Cheers.