Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Updating environment variable name for mongodb with the new one for Heroku #1413

Merged
merged 1 commit into from
Jul 31, 2016

Conversation

lirantal
Copy link
Member

Updating environment variable name for mongodb with the new one for Heroku (according to https://devcenter.heroku.com/articles/mongolab)
Fixes #1411

@lirantal lirantal added this to the 0.5.0 milestone Jul 27, 2016
@lirantal lirantal self-assigned this Jul 27, 2016
@codydaig
Copy link
Member

LGTM

@ilanbiala
Copy link
Member

LGTM.

@mleanos
Copy link
Member

mleanos commented Jul 28, 2016

👍

@mleanos
Copy link
Member

mleanos commented Jul 28, 2016

I looked into this a little further, and realized that this might end up being a breaking change. I have an app deployed on Heroku, and the environment variable I'm using is "MONGOLAB_URI", and I'm not having any issues connecting to my mLab instance. However, if this change is introduced and I update my application from upstream, there will be a mismatch & my app won't be able to connect to mLab if I don't update the config variable on Heroku. The fix in that case would be simple & if I'm merging from upstream (with this change) I should be aware that I'll have to update my setting on Heroku.

The issue that this is addressing might not be an issue at all. Just as long as the name used on Heroku's environment config settings matches up with what our application uses, then there shouldn't be a problem. If someone is using Heroku's MongoDB plugin, then there may be a mismatch if they don't set an environment variable that matches the name that our framework uses. So they should add this setting to their Heroku env config vars.

My concern here is that we can't account for every variation of naming convention that's used by all possible deployment environments. For instance, what if AWS uses a different name for their MongoDB connection strings?

To me the name we use is irrelevant, and it's up to the developer to configure their app to accommodate whichever environment they are deploying to; to ensure their MEANJS app matches up with their deployment environment.

@lirantal
Copy link
Member Author

@mleanos I understand what you're trying to say but we aren't accommodating every possible config, but rather just herokus, and they indeed changed their config variable from the old one to the new one so I don't see an issue here. (I also validated with other repositories performed this change)

Unless you really think there's something else to consider here I don't see why not to make this change.

@mleanos
Copy link
Member

mleanos commented Jul 29, 2016

@lirantal I'm fine with this change. I just wanted to express my point of view. To me there isn't an impact if we do, or do not, make this change. So lets roll with this to keep our naming consistent with Heroku's.

@lirantal
Copy link
Member Author

Cool, merging.

@lirantal lirantal merged commit 64392b1 into meanjs:master Jul 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants