-
Notifications
You must be signed in to change notification settings - Fork 1k
Improve automation for env vars for Data Connect init. #8684
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
Improve automation for env vars for Data Connect init. #8684
Conversation
Created using spr 1.3.6-beta.1
src/init/features/project.ts
Outdated
projectMetaData = await projectChoicePrompt(options); | ||
if (!projectMetaData) { | ||
return; | ||
const projectEnvVar = utils.envOverride("GCLOUD_PROJECT", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defer to @joehan on this~
Is this a reasonable env name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it probably should be GOOGLE_CLOUD_PROJECT or FIREBASE_PROJECT
GoogleCloudPlatform/nodejs-getting-started#251 GCLOUD_PROJECT is a deprecated CF3 env var - obviously not gonna affect this, but I'd prefer to keep consistency accross products when possible
We also have some code in place that populates GCLOUD_PROJECT to support very old firebase-functions versions, so it'll be nice to avoid any interactions with that by using GOOGLE_CLOUD_PROJECT instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I've changed it to FIREBASE_PROJECT
. Later maybe we can make it a drop-in replacement of --project
. Or not. The script can work either way
if (cwdPlatformGuess !== Platform.NONE) { | ||
// If a platform can be detected or a connector is chosen via env var, always | ||
// setup SDK. FDC_CONNECTOR is used for scripts under https://firebase.tools/. | ||
if (cwdPlatformGuess !== Platform.NONE || envOverride("FDC_CONNECTOR", "")) { | ||
await sdk.doSetup(setup, config); | ||
} else { | ||
logBullet( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we can change this to a confirmation~
We can do it separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but probably switch the env var
src/init/features/project.ts
Outdated
projectMetaData = await projectChoicePrompt(options); | ||
if (!projectMetaData) { | ||
return; | ||
const projectEnvVar = utils.envOverride("GCLOUD_PROJECT", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it probably should be GOOGLE_CLOUD_PROJECT or FIREBASE_PROJECT
GoogleCloudPlatform/nodejs-getting-started#251 GCLOUD_PROJECT is a deprecated CF3 env var - obviously not gonna affect this, but I'd prefer to keep consistency accross products when possible
We also have some code in place that populates GCLOUD_PROJECT to support very old firebase-functions versions, so it'll be nice to avoid any interactions with that by using GOOGLE_CLOUD_PROJECT instead
Created using spr 1.3.6-beta.1
Description
See CHANGELOG.md
Scenarios Tested
Sample Commands
firebase init
firebase init dataconnect