Move 'location' from firebase.json to dataconnect.yaml #7373
Conversation
| @@ -43,6 +40,9 @@ export async function readDataConnectYaml(sourceDirectory: string): Promise<Data | |||
|
|
|||
| function validateDataConnectYaml(unvalidated: any): DataConnectYaml { | |||
| // TODO: Add validation | |||
| if (!unvalidated["location"]) { | |||
| throw new FirebaseError("Missing required field 'location' in dataconnect.yaml"); | |||
| } | |||
| return unvalidated as DataConnectYaml; | |||
There was a problem hiding this comment.
OOC, Q: do we want to validate other fields as well?
Is there a way to make json schema do it for us?
There was a problem hiding this comment.
That's a good point, and we defiitely should leverage the json schema here. Adding a todo for now.
src/dataconnect/prompts.ts
Outdated
| @@ -21,3 +21,10 @@ export async function promptDeleteConnector( | |||
| utils.logLabeledSuccess("dataconnect", `Connector ${connectorName} deleted`); | |||
| } | |||
| } | |||
|
|
|||
| export function locationInFirebaseJsonWarning() { | |||
There was a problem hiding this comment.
Is this used? I saw the same string in src/config.ts
There was a problem hiding this comment.
Oops, this was accidentally left in, removing.
| `--config_dir=${args.configDir}`, | ||
| `--connector_id=${args.connectorId}`, | ||
| ]; | ||
| const res = childProcess.spawnSync(commandInfo.binary, cmd, { encoding: "utf-8" }); | ||
|
|
||
| logger.debug(res.stderr); | ||
| if (res.error) { |
There was a problem hiding this comment.
I think we would want to remove this check.
--logtostderr above will re-direct all logs to stderr. Probably negligence on my end.
Have we tested it?
There was a problem hiding this comment.
This does pipe all logs to stderr - however, it is the stderr of the childprocess, so it is not visible to the end user unless we do this.. This pipes that to the main logger at debug level, so that we can at least use it for debugging issues.
We could alternatively log at info level if we want these to always be displayed, which looks like:
Its a bit noisy, but not too bad overall.
There was a problem hiding this comment.
Oh, thanks for the test run!
The info logs here looks helpful~ I was worrying if if (res.error) would be true in case of any stderror. Doesn't seem to be that case.
| config.set("dataconnect.location", info.locationId); | ||
| } else { | ||
| // Remove location if it was previously set. | ||
| config.set("dataconnect", { source: dir }); |
There was a problem hiding this comment.
Q: why this branch?
Isn't this equivalent to always do config.set("dataconnect", { source: dir });?
| `--config_dir=${args.configDir}`, | ||
| `--connector_id=${args.connectorId}`, | ||
| ]; | ||
| const res = childProcess.spawnSync(commandInfo.binary, cmd, { encoding: "utf-8" }); | ||
|
|
||
| logger.debug(res.stderr); | ||
| if (res.error) { |
There was a problem hiding this comment.
Oh, thanks for the test run!
The info logs here looks helpful~ I was worrying if if (res.error) would be true in case of any stderror. Doesn't seem to be that case.
Description
Previously, location was provided as part of firebase.json:
Now, it is provided via dataconnect.yaml
Scenarios Tested
Tested the following cases using an emulator built from HEAD:
I also tested the cases where someone still has the old way of setting location - we show a warning if location is set in firebase.json, and error out if it is missing in dataconnect.yaml:
