ibroadfo / ember-cli-deploy-firebase

an ember-cli-deploy plugin to deploy to firebase hosting
MIT License
6 stars 13 forks source link

Add deployToken option for firebase for use with multiple accounts. #4

Closed mileszim closed 7 years ago

mileszim commented 7 years ago

Additionally, change fireBaseAppName to firebase.appName to fit with config hash. Does not break 'context.config.fireBaseAppName'

ibroadfo commented 7 years ago

I won't get a chance to test this till tomorrow pm but it looks good on a visual scan.

I wondered if we should default to using process.env.FIREBASE_TOKEN; thoughts?

mileszim commented 7 years ago

@ibroadfo I was thinking that would be a fine route to go, but it seems like the .env environments would start to get too confusing. In other words, FIREBASE_TOKEN is such a generic label it could mean anything, but FIREBASE_DEPLOY_TOKEN implies only one thing. If there is a benefit for FIREBASE_TOKEN my opinion is extremely weak so that's fine with me.

mileszim commented 7 years ago

Also FYI it works fine, I'm using it on a production project no sweat

ibroadfo commented 7 years ago

The reason I was asking about FIREBASE_TOKEN is that's what firebase-tools uses, and in fact deploying works with that environment variable set without this patch. (FIREBASE_TOKEN="foo" ember deploy production)

I'm not saying this isn't a useful thing to add! Just wanted to run this past you. :)

mileszim commented 7 years ago

Oooooohhh that might make it a lot simpler of a change. Lemme rework this and resubmit. Good catch :)

ibroadfo commented 7 years ago

👍 On Wed, 3 May 2017 at 02:25, Miles Zimmerman notifications@github.com wrote:

Oooooohhh that might make it a lot simpler of a change. Lemme rework this and resubmit. Good catch :)

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ibroadfo/ember-cli-deploy-firebase/pull/4#issuecomment-298804110, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAGDr-czriYa8_zBiBde5CKpeEDr53Nks5r19eIgaJpZM4NMkvv .

mileszim commented 7 years ago

Ok it's been updated to use the native FIREBASE_TOKEN env var if available. I have it behind the config/environment.js var in case organizing your token stuff there is more preferable, but either works. I'll sing the usual dev song: "Works on my machine". lemme know if you hit snags. Readme is also updated with both use cases

ibroadfo commented 7 years ago

Apologies for being a bit slow at handling this; I'm currently travelling so don't have reliable interwubs.

I think this is looking good! I noticed that the readme is mentioning config/environment.js where I'd expect it to be config/deploy.js; would that be right?

Hope this doesn't seem like nitpicking! :)

mileszim commented 7 years ago

@ibroadfo my b! changed to config/deploy.js, you were totally right about that since as one option I said to put the token in .env.deploy.production which would have only shown up in that file. Should be good to go I hope

ibroadfo commented 7 years ago

Thanks! This should be really useful for people.

ibroadfo commented 7 years ago

released! 😸