Support custom ENVs #1520

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
@dschmidt
Contributor

dschmidt commented Jul 29, 2014

Hey,

so I started trying to implement the proposal from https://gist.github.com/rwjblue/51f1df4b21a23466ae47
To make sure I understood it right and to show up my problems I'm submitting this in a really early hacky stage.

Instead of adding methods to the EmberApp object I created a config model that is passed to the respective configuration module.

See the corresponding config files here: https://github.com/dschmidt/ember-cli-sass-demo/tree/customenvs/config

Going to add my questions as comments to the code below

Best regards,
Dominik

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Jul 29, 2014

Owner

I was told the config should not be cached so it can be reread on every build / FS change.
So I'm not caching tests/hinting here either. Makes sense?

I was told the config should not be cached so it can be reread on every build / FS change.
So I'm not caching tests/hinting here either. Makes sense?

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Jul 29, 2014

Owner

This went all to config files

This went all to config files

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Jul 29, 2014

Owner

I could not find were this was (still) used.

I could not find were this was (still) used.

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Jul 29, 2014

Owner

Not sure how to handle this... if options/config is not supposed to be cached, where would I handle this? I can't make this part of the config without giving it access to this.project(.root) and without cluttering up the API.

For now I have added the defaults where the values are consumed.

Not sure how to handle this... if options/config is not supposed to be cached, where would I handle this? I can't make this part of the config without giving it access to this.project(.root) and without cluttering up the API.

For now I have added the defaults where the values are consumed.

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Jul 29, 2014

Owner

Does not quite make sense to get this value from the config right now, explained above.

Does not quite make sense to get this value from the config right now, explained above.

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Jul 29, 2014

Owner

I did not find where the options are passed to anything at all. So this is probably noop right now and suffers from the same problem as the jshintrc issue above. If the config is not cached, how would I set default values there?

I did not find where the options are passed to anything at all. So this is probably noop right now and suffers from the same problem as the jshintrc issue above. If the config is not cached, how would I set default values there?

lib/broccoli/ember-app.js
var jshintedApp = jshintTrees(preprocessedApp, {
- jshintrcPath: this.options.jshintrc.app,
+ jshintrcPath: config.get('hinting', 'js', 'app') || this.project.root,

This comment has been minimized.

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

should we support ember's path support? get('hinting.js.app')

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

should we support ember's path support? get('hinting.js.app')

This comment has been minimized.

@dschmidt

dschmidt Jul 30, 2014

Contributor

Had the same thought when writing that line! 👍

@dschmidt

dschmidt Jul 30, 2014

Contributor

Had the same thought when writing that line! 👍

This comment has been minimized.

@stefanpenner

stefanpenner Aug 13, 2014

Contributor

^^ get('hinting.js.app') would be great, but we can add it in a follow up PR

@stefanpenner

stefanpenner Aug 13, 2014

Contributor

^^ get('hinting.js.app') would be great, but we can add it in a follow up PR

lib/models/config.js
+
+
+function Config() {
+ this.options = {};

This comment has been minimized.

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

Object.create(null) for these, so that we don't collide with prototype methods

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

Object.create(null) for these, so that we don't collide with prototype methods

lib/models/config.js
+// getter
+Config.prototype.get = function(group, key) {
+ var argc = arguments.length,
+ options = this.options;

This comment has been minimized.

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

1 var per assigment

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

1 var per assigment

lib/models/config.js
+'use strict';
+
+var isObject = require('lodash-node/modern/objects/isObject');
+var isUndefined = require('lodash-node/modern/objects/isUndefined');

This comment has been minimized.

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

whitespace

@stefanpenner

stefanpenner Jul 30, 2014

Contributor

whitespace

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 30, 2014

Contributor

Scheint gut Bruder.
I like that you have separated the config object out

Contributor

stefanpenner commented Jul 30, 2014

Scheint gut Bruder.
I like that you have separated the config object out

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Jul 30, 2014

Contributor

My most important question before I proceed:
If I don't cache the config object where/how am I supposed to set/add defaults like for jshintrc above?

Contributor

dschmidt commented Jul 30, 2014

My most important question before I proceed:
If I don't cache the config object where/how am I supposed to set/add defaults like for jshintrc above?

@dschmidt

This comment has been minimized.

Show comment
Hide comment
Contributor

dschmidt commented Aug 12, 2014

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 13, 2014

Contributor

@dschmidt thanks for the reminder I have been traveling. Will review this evening.

Contributor

stefanpenner commented Aug 13, 2014

@dschmidt thanks for the reminder I have been traveling. Will review this evening.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 13, 2014

Contributor

@dschmidt this looks fantastic, only tiny feedbacks^

I think for the first pass we can ignore the caching stuff, subsequent work and introduce this.

Contributor

stefanpenner commented Aug 13, 2014

@dschmidt this looks fantastic, only tiny feedbacks^

I think for the first pass we can ignore the caching stuff, subsequent work and introduce this.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 13, 2014

Contributor

Looks good. Happy to defer the caching issue for now.

Contributor

rwjblue commented Aug 13, 2014

Looks good. Happy to defer the caching issue for now.

dschmidt added some commits Aug 14, 2014

Merge remote-tracking branch 'upstream/master' into customenvs
Conflicts:
	lib/broccoli/ember-app.js
	lib/models/project.js
	tests/helpers/mock-project.js
foo
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Aug 26, 2014

Contributor

can I provide any help/feedback here (sorry I have been out of town with little time to provide feedback here)

Contributor

stefanpenner commented Aug 26, 2014

can I provide any help/feedback here (sorry I have been out of town with little time to provide feedback here)

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Aug 29, 2014

Contributor

Yup, you can. I am pretty clueless about what's up with tests ... they are hanging for me and having odd issues that I really don't know how to debug.

So if you could walk me through debugging it, that would be rad. Currently I am still at the ownCloud sprint, but could work on it early next week.

Contributor

dschmidt commented Aug 29, 2014

Yup, you can. I am pretty clueless about what's up with tests ... they are hanging for me and having odd issues that I really don't know how to debug.

So if you could walk me through debugging it, that would be rad. Currently I am still at the ownCloud sprint, but could work on it early next week.

@dschmidt

This comment has been minimized.

Show comment
Hide comment
@dschmidt

dschmidt Sep 28, 2014

Contributor

I'm still aware of this PR, just been travelling a lot last month and haven't got around to it. Will get back to it soon hopefully, sorry.

Contributor

dschmidt commented Sep 28, 2014

I'm still aware of this PR, just been travelling a lot last month and haven't got around to it. Will get back to it soon hopefully, sorry.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 28, 2014

Contributor

No worries im in the same boat

Contributor

stefanpenner commented Sep 28, 2014

No worries im in the same boat

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
Contributor

stefanpenner commented Sep 29, 2014

@joshsmith

This comment has been minimized.

Show comment
Hide comment
@joshsmith

joshsmith Oct 2, 2014

I know you guys are busy so don't want to appear too greedy for your time, just wondering what the status of this and when we might realistically see something. I'm close to deploying from staging to production and wanted to know where things stand.

I know you guys are busy so don't want to appear too greedy for your time, just wondering what the status of this and when we might realistically see something. I'm close to deploying from staging to production and wanted to know where things stand.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 2, 2014

Contributor

unsure both domme and myself have been busy + traveling so this has lingered :(

Contributor

stefanpenner commented Oct 2, 2014

unsure both domme and myself have been busy + traveling so this has lingered :(

@jakecraige

This comment has been minimized.

Show comment
Hide comment
@jakecraige

jakecraige Oct 2, 2014

Member

@joshsmith I personally have no problems deploying staging vs. production. I guess it depends exactly what config you need to change. Custom environments for config/environment.js currently work just fine. You just use whatever environment name you want and do an if within the environment.js file. This seems to just be adding a new feature to help differentiate between "configuration" and "environment" vars.

Member

jakecraige commented Oct 2, 2014

@joshsmith I personally have no problems deploying staging vs. production. I guess it depends exactly what config you need to change. Custom environments for config/environment.js currently work just fine. You just use whatever environment name you want and do an if within the environment.js file. This seems to just be adding a new feature to help differentiate between "configuration" and "environment" vars.

@joshsmith

This comment has been minimized.

Show comment
Hide comment
@joshsmith

joshsmith Oct 2, 2014

Ah, thanks @jakecraige. I only really need to change ENV vars.

Ah, thanks @jakecraige. I only really need to change ENV vars.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Oct 2, 2014

Contributor

Submitted #2164 to make adding additional environments (as either production or development styles) really simple.

Contributor

rwjblue commented Oct 2, 2014

Submitted #2164 to make adding additional environments (as either production or development styles) really simple.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 2, 2014

Contributor

I will attempt some time this Saturday to help get this to completion.

Contributor

stefanpenner commented Oct 2, 2014

I will attempt some time this Saturday to help get this to completion.

@joshsmith

This comment has been minimized.

Show comment
Hide comment
@joshsmith

joshsmith Oct 2, 2014

Thanks a lot @stefanpenner. While I know there's probably not much I can do, let me know if I can do anything to help.

Thanks a lot @stefanpenner. While I know there's probably not much I can do, let me know if I can do anything to help.

@joshsmith

This comment has been minimized.

Show comment
Hide comment
@joshsmith

joshsmith Oct 2, 2014

And thanks for attempt at temporary fix @rwjblue.

And thanks for attempt at temporary fix @rwjblue.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 2, 2014

Contributor

@joshsmith if u can get that PR over the finish line that would be handy.

Contributor

stefanpenner commented Oct 2, 2014

@joshsmith if u can get that PR over the finish line that would be handy.

@soulcutter

This comment has been minimized.

Show comment
Hide comment
@soulcutter

soulcutter Oct 23, 2014

What still needs to be done? I was able to make the tests pass with a fairly trivial change from config/environment -> config/development in project-test, but it's a little hard for me to follow what might still need attention.

What still needs to be done? I was able to make the tests pass with a fairly trivial change from config/environment -> config/development in project-test, but it's a little hard for me to follow what might still need attention.

@jakecraige

This comment has been minimized.

Show comment
Hide comment
@jakecraige

jakecraige Nov 5, 2014

Member

I'm a bit late chiming in here but it's better late than never.

First, since we're in ember-land I think we should make this very ember like and pretty much clone Ember.get, Ember.set, Ember.setProperties API.

This also means config.get,config.set and config.isEnabled should use the . syntax to get/set properties. This allows for arbitrary nesting or properties which currently isn't allowed. It's fixed 2 levels deep right now. A real example of this that wouldn't work: config.isEnabled('ENV.EmberENV.FEATURES.query-params)`

My idea is the API look like:

config.set(keyPath, value);
config.get(keyPath);
config.isEnabled(keyPath);
config.setProperties(keyPath, hash);

Thoughts?

Member

jakecraige commented Nov 5, 2014

I'm a bit late chiming in here but it's better late than never.

First, since we're in ember-land I think we should make this very ember like and pretty much clone Ember.get, Ember.set, Ember.setProperties API.

This also means config.get,config.set and config.isEnabled should use the . syntax to get/set properties. This allows for arbitrary nesting or properties which currently isn't allowed. It's fixed 2 levels deep right now. A real example of this that wouldn't work: config.isEnabled('ENV.EmberENV.FEATURES.query-params)`

My idea is the API look like:

config.set(keyPath, value);
config.get(keyPath);
config.isEnabled(keyPath);
config.setProperties(keyPath, hash);

Thoughts?

@jakecraige jakecraige referenced this pull request in poetic/ember-cli-cordova Nov 12, 2014

Open

first pass at multi environment support #91

@jakecraige jakecraige referenced this pull request Dec 22, 2014

Closed

ember-cli google hangout #2789

0 of 2 tasks complete
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 23, 2015

Contributor

we still intend to solve this, but master has diverged pretty far. I am going to close this, but we will sort this out.

Contributor

stefanpenner commented Feb 23, 2015

we still intend to solve this, but master has diverged pretty far. I am going to close this, but we will sort this out.

@raido

This comment has been minimized.

Show comment
Hide comment
@raido

raido Sep 6, 2015

Contributor

What's the status of this?

Contributor

raido commented Sep 6, 2015

What's the status of this?

@Kuzirashi

This comment has been minimized.

Show comment
Hide comment

Status?

@trcarden

This comment has been minimized.

Show comment
Hide comment
@trcarden

trcarden Jan 27, 2016

@stefanpenner is there any update on this feature? We are trying to put our app up but we need staging environment support

@stefanpenner is there any update on this feature? We are trying to put our app up but we need staging environment support

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jan 27, 2016

Contributor

Please use the existing 3 env. And configure those further with environment variables

Contributor

stefanpenner commented Jan 27, 2016

Please use the existing 3 env. And configure those further with environment variables

@trcarden

This comment has been minimized.

Show comment
Hide comment
@trcarden

trcarden Jan 27, 2016

Ah, thats pretty obvious in retrospect. For others that come across this i found a decent SO post here to help: http://stackoverflow.com/questions/26403334/how-to-pass-api-keys-in-environment-variables-to-ember-cli-using-process-env @stefanpenner thank you!

Ah, thats pretty obvious in retrospect. For others that come across this i found a decent SO post here to help: http://stackoverflow.com/questions/26403334/how-to-pass-api-keys-in-environment-variables-to-ember-cli-using-process-env @stefanpenner thank you!

@Senthe

This comment has been minimized.

Show comment
Hide comment
@Senthe

Senthe Nov 16, 2016

Two years later - is support for staging never going to be done? Documentation implies otherwise.

Honestly I just encountered this issue today and can't understand why isn't it added? Am I really supposed to pass variables from bash if I need them for staging, like the @trcarden post above suggests? I'm really lost. There was an issue opened to document it (#5400) but documentation was never done.

Senthe commented Nov 16, 2016

Two years later - is support for staging never going to be done? Documentation implies otherwise.

Honestly I just encountered this issue today and can't understand why isn't it added? Am I really supposed to pass variables from bash if I need them for staging, like the @trcarden post above suggests? I'm really lost. There was an issue opened to document it (#5400) but documentation was never done.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Nov 16, 2016

Contributor

@zackthehuman can you paste in the snippet that you wrote?

Contributor

nathanhammond commented Nov 16, 2016

@zackthehuman can you paste in the snippet that you wrote?

@zackthehuman

This comment has been minimized.

Show comment
Hide comment
@zackthehuman

zackthehuman Nov 16, 2016

@nathanhammond I think this is what you're referring to.

I sometimes build my addon's dummy app in order to use it like a demo/api documentation. For certain reasons I am unable to use the production build, so I want to use the development build, but I need to override some settings when I'm building the "apidoc" mode.

// environment.js

if(process.argv.indexOf('--apidoc') !== -1) {
    console.log('This is an apidoc build so the environment settings will be adjusted.');
    ENV.locationType = 'hash';

    // We want to force the country code to be "us".
    ENV.i18n.requestIpCountryCode = 'us';
}

@nathanhammond I think this is what you're referring to.

I sometimes build my addon's dummy app in order to use it like a demo/api documentation. For certain reasons I am unable to use the production build, so I want to use the development build, but I need to override some settings when I'm building the "apidoc" mode.

// environment.js

if(process.argv.indexOf('--apidoc') !== -1) {
    console.log('This is an apidoc build so the environment settings will be adjusted.');
    ENV.locationType = 'hash';

    // We want to force the country code to be "us".
    ENV.i18n.requestIpCountryCode = 'us';
}
@meliborn

This comment has been minimized.

Show comment
Hide comment
@meliborn

meliborn Nov 25, 2016

Is there a better way how to add staging environment like production and development? I need to set API_HOST for each environment in my app, but I can't wait another 2 year for support this feature.

meliborn commented Nov 25, 2016

Is there a better way how to add staging environment like production and development? I need to set API_HOST for each environment in my app, but I can't wait another 2 year for support this feature.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Nov 25, 2016

Contributor

@meliborn See the comment above yours. That represents a semi-official recommendation for how to toggle different information on a per-build basis. You'd use that code and something like:

ember build -prod --api-host=asdfasdfasdf

Contributor

nathanhammond commented Nov 25, 2016

@meliborn See the comment above yours. That represents a semi-official recommendation for how to toggle different information on a per-build basis. You'd use that code and something like:

ember build -prod --api-host=asdfasdfasdf

@meliborn

This comment has been minimized.

Show comment
Hide comment

@nathanhammond wrong link?

@wayne-o

This comment has been minimized.

Show comment
Hide comment
@wayne-o

wayne-o Nov 25, 2016

wayne-o commented Nov 25, 2016

@meliborn

This comment has been minimized.

Show comment
Hide comment
@meliborn

meliborn Nov 25, 2016

@wayne-o could you throw me an example please? Can't imagine how to use it without if-env block

@wayne-o could you throw me an example please? Can't imagine how to use it without if-env block

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Nov 25, 2016

Contributor

@meliborn Corrected link. Literally the comment above yours provides a workaround you can use, and my comment demonstrates how you could use it.

Contributor

nathanhammond commented Nov 25, 2016

@meliborn Corrected link. Literally the comment above yours provides a workaround you can use, and my comment demonstrates how you could use it.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Nov 25, 2016

Contributor

Also, @meliborn

but I can't wait another 2 year for support this feature

This type of communication is not constructive in an open source community. This implies a demand. Our work on Ember is a labor of love. Very few of us are paid to work directly on Ember projects, and if we are those efforts are strongly directed by those who are paying. We're real human beings with hobbies and pastimes and friends and things we do other than this. We strive to be friendly and helpful in spite of this, but I wanted to provide a bit of feedback so you understand that it's frustrating and hurtful to read comments like that. We'd love infinite time to address all things. I assure you. But we must prioritize.

Contributor

nathanhammond commented Nov 25, 2016

Also, @meliborn

but I can't wait another 2 year for support this feature

This type of communication is not constructive in an open source community. This implies a demand. Our work on Ember is a labor of love. Very few of us are paid to work directly on Ember projects, and if we are those efforts are strongly directed by those who are paying. We're real human beings with hobbies and pastimes and friends and things we do other than this. We strive to be friendly and helpful in spite of this, but I wanted to provide a bit of feedback so you understand that it's frustrating and hurtful to read comments like that. We'd love infinite time to address all things. I assure you. But we must prioritize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment