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

Allow to use both lowercased and uppercased env variables #8960

Closed
wants to merge 2 commits into from

Conversation

mahnunchik
Copy link

Hi @kirrg001

It is for #8935

@kirrg001 kirrg001 self-assigned this Sep 1, 2017
@kirrg001
Copy link
Contributor

kirrg001 commented Sep 1, 2017

@mahnunchik Thanks! Could you please fix the lint error? Thanks :)

@kirrg001 kirrg001 changed the title Allow to use both lowercased and uppercased env variables, #8935 Allow to use both lowercased and uppercased env variables Sep 1, 2017
@kirrg001
Copy link
Contributor

kirrg001 commented Sep 1, 2017

I think the nconf implementation is a little bit confusing and not sure we should merge this.

e.g. you have a config key in Ghost e.g. useMinFiles

useMinFiles=true node index.js

nconf adds useminfiles and useMinFiles to the env object, see, which results in Ghost having two keys in the config object (config.get('useminfiles') and config.get('useMinFiles') works).

@mahnunchik
Copy link
Author

@kirrg001 it should not break the current code, but it will make it much more convenient to use environment variables.

@kirrg001
Copy link
Contributor

kirrg001 commented Sep 4, 2017

it should not break the current code

Yeah i know, but the concern i've raised is a about messing up our config object.

cc @ErisDS

@ErisDS
Copy link
Member

ErisDS commented Oct 18, 2017

I have been thinking about this. Using uppercase for environment variables is fairly standard, and therefore it would be really good if we could support this.

Although this option in nconf is intended to make this supported, it is actually broken in the next version bump 0.8.5, as tracked in indexzero/nconf#271.

I also agree that this option has an unintended side effect. Almost every config value in Ghost has a snakeCased key, meaning most of them would be stored in memory twice. This is an unnecessary waste.

I've added a comment here: indexzero/nconf#271 (comment) explaining this.

I believe the way forward here is to come up with a better option/implementation in nconf, that doesn't impact memory usage. Once that is supported I'd be more than happy to accept a PR that upgrades nconf and adds support to Ghost.

I hope that makes sense. Going to close this PR as we aren't going to merge this approach.

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

Successfully merging this pull request may close these issues.

3 participants