Skip to content

Proposal: reduce usage of environment variables for configuration. #1715

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

Closed
drew-gross opened this issue May 7, 2016 · 11 comments
Closed

Proposal: reduce usage of environment variables for configuration. #1715

drew-gross opened this issue May 7, 2016 · 11 comments

Comments

@drew-gross
Copy link
Contributor

I read this recently: http://12factor.net/config

It suggests not using environment variables for anything that isn't going to change between development and production environments.

Probably in practise that would mean these would stay as env vars:

PARSE_SERVER_APPLICATION_ID
PARSE_SERVER_MASTER_KEY
PORT
PARSE_SERVER_DATABASE_URI
PARSE_SERVER_COLLECTION_PREFIX
PARSE_SERVER_URL
PARSE_PUBLIC_SERVER_URL
PARSE_SERVER_CLIENT_KEY
PARSE_SERVER_JAVASCRIPT_KEY
PARSE_SERVER_REST_API_KEY
PARSE_SERVER_DOT_NET_KEY
PARSE_SERVER_PUSH
PARSE_SERVER_FILE_KEY
PARSE_SERVER_FACEBOOK_APP_IDS

And these would become set in code/command line flags only:

PARSE_SERVER_DATABASE_OPTIONS
PARSE_SERVER_CLOUD_CODE_MAIN
PARSE_SERVER_OAUTH_PROVIDERS
PARSE_SERVER_ENABLE_ANON_USERS
PARSE_SERVER_ALLOW_CLIENT_CLASS_CREATION
PARSE_SERVER_FILES_ADAPTER
PARSE_SERVER_EMAIL_ADAPTER
PARSE_SERVER_VERIFY_USER_EMAILS
PARSE_SERVER_APP_NAME
PARSE_SERVER_CUSTOM_PAGES
PARSE_SERVER_MAX_UPLOAD_SIZE
PARSE_SERVER_SESSION_LENGTH
VERBOSE
PARSE_SERVER_REVOKE_SESSION_ON_PASSWORD_RESET
PARSE_SERVER_LIVE_QUERY_OPTIONS
PARSE_SERVER_LOGGER_ADAPTER
PARSE_SERVER_MOUNT_PATH

Then for future settings we would decided on a case-by-case basis whether to add env vars for them or not.

Thoughts?

@flovilmart
Copy link
Contributor

Guys using pm2 will likely cry and hate us. But I'm ok with it :)

@blacha
Copy link
Contributor

blacha commented May 7, 2016

PARSE_SERVER_ALLOW_CLIENT_CLASS_CREATION would be something that would change between dev / production.

PARSE_SERVER_VERIFY_USER_EMAILS is something I would also have off in dev

@drew-gross
Copy link
Contributor Author

Personally I would recommend having allow client class creation not change between dev and prod. If you are working on a full stack feature where you add a class in your server, and some client code that uses it, and then you deploy both your client and your server, things will break. I don't have a perfect solution for prototyping full stack features within an app that is out of prototyping, but I don't think allowing client class creating only in dev is the right solution.

My experience working on Parse.com tells me that turning off email verification is dev is a bad idea, as it makes it very hard to work on email related features. What I would recommend to most people is to have a separate set of keys for your email service for dev.

@steven-supersolid
Copy link
Contributor

We create the server with a config object and pass in our own environment variables but the variables that we have set match the top group.

Suggestions to remove:
PARSE_SERVER_COLLECTION_PREFIX - Would this vary between dev and prod?
PARSE_SERVER_URL - Is the use case for this being able to run cloud code on a different server? If so, can see why we need to keep.

Suggestion to keep:
PARSE_SERVER_DATABASE_OPTIONS - I think this could be useful e.g. to change connection pooling options for different environments.

@drew-gross
Copy link
Contributor Author

Collection prefix can be used to have multiple apps in the same database, which is something you may want to do in dev but not prod. I don't know if anyone is actually doing that kind of setup in practice.

Server URL is pretty much always going to be localhost. Ideally we would remove it completely, but the JS SDK needs it for now. I would definitely be open to removing that one.

Good point on database options, that one should probably stay as an env var.

@bohemima
Copy link
Contributor

bohemima commented May 8, 2016

Changing this out of the blue will cause a lot of problems with early adopters, I suggest not adding any more configuration options using environment variables and for next major version remove them altogether.

@drew-gross
Copy link
Contributor Author

Yep, this would definitely be a breaking change, so we would need to version appropriately.

@steven-supersolid
Copy link
Contributor

Very minor but should we consider renaming PARSE_PUBLIC_SERVER_URL to PARSE_SERVER_PUBLIC_URL? Then all variables would start with PARSE_SERVER and would be grouped if sorted alphabetically (like in the Heroku dashboard). They will be grouped anyway because of the PARSE_ prefix so not a biggie.

@drew-gross
Copy link
Contributor Author

That is a good point and we probably should have done that at the start, but I don't think thats a good enough reason to do a breaking change.

@cleever
Copy link

cleever commented Aug 2, 2016

You guys probably know about it.

But, I recommend to take a look into a great tool called dotenv.
https://github.com/motdotla/dotenv

@montymxb
Copy link
Contributor

Closing this out as it's been some time and we haven't made any strides towards this recently. If there's still an interest let us know and we'll open this back up.

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

No branches or pull requests

7 participants