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

Add support for JSON array/object notation in env variables #98

Closed
wants to merge 6 commits into from

Conversation

Legogris
Copy link

@Legogris Legogris commented Oct 3, 2017

In the current version, if an array is defined in an rc, there is no way to override it from env variables. This adds support for this. New version number since this risks breaking existing configurations.

@azetilov
Copy link

azetilov commented Oct 3, 2017

Hi,

A couple of reasons I had not to create this PR myself:

  1. This is a breaking change - some clients might not expect strings to be parsed, some have logic in place to parse manually
  2. It seems that parsing only works for env variables, not covering command-line arguments - need to check/test this to avoid inconsistency
  3. I'm not sure maintainer do wants to have this functionality in place

Solutions:

  1. Make this feature optional
  2. Test and add command-line arguments parsing as well
  3. Would be good to get this feedback

Thanks,
Anatolii

@Legogris
Copy link
Author

Legogris commented Oct 3, 2017

Good point on the command-line arguments.
I guess the rest is ultimately up to @dominictarr, but my 5 cents:

  1. Should be OK as long as there is a major version bump and notes in the release notes/changelog on this. One could always escape strings.

@dominictarr
Copy link
Owner

can you describe your use case and why this is a problem?
I'd generally recommend avoiding arrays in rc configs (since merging the configs doesn't make much sense)
and making it use JSON in envvars for everyone is a big change just because you want arrays.

@dominictarr
Copy link
Owner

I'm gonna close this. I maintain a very high bar for changes to rc and that is what keeps it very simple and thus useful, changes of this degree need to be very strongly backed up by a use case. I suggest just parsing after loading your config.

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