Repository navigation
Conversation
0416f78 to
bfcabe5
Compare
| "US (default)", | ||
| "EU", | ||
| "other", | ||
| "auto (let BigQuery choose dynamically)", |
There was a problem hiding this comment.
With "auto", location is unset, but BigQueryDbAdapter.createSchema still does location || this.bigQueryCredentials.location || "US", and the region-${location}.INFORMATION_SCHEMA.COLUMN_FIELD_PATHS query also falls back to US. So a user with EU data who picks "auto" would get new datasets created in US, and then cross-region queries would fail. Could we either handle the unset location in those paths, or at least say in this label/help text that dataset creation defaults to US?
There was a problem hiding this comment.
This is a bigger change that I'm working on, but you are right that the message is misleading as of now - perhaps best to wait with this PR until the automatic location inference is ready
| ); | ||
| } | ||
| "The default location to use (optional). If not set, BigQuery chooses the location " + | ||
| "dynamically. See https://cloud.google.com/bigquery/docs/locations for supported values.", |
There was a problem hiding this comment.
"BigQuery chooses the location dynamically" holds for query jobs, but not for dataset creation, which falls back to US in the BigQuery adapter (createSchema). The new README text for default-location / init-creds makes the same claim. Could we reword both (or fix the fallback) so users aren't surprised?
There was a problem hiding this comment.
As above, this was written with further changes in mind. I'm going to turn this PR into a draft until that is ready.
| }); | ||
| }); | ||
|
|
||
| test("ADC with auto location leaves location unset", () => { |
There was a problem hiding this comment.
Nice coverage. Could you also add tests for (a) "other" with an empty region name, and (b) what the adapter does (e.g. createSchema) when no location is set?
There was a problem hiding this comment.
For A, explicitly set location to undefined and print out a warning if empty region name provided. For B, tests were introduced by #2394
| DATAFORM_LINEAGE_DEBUG=1 dataform run --emit-lineage | ||
| ``` | ||
|
|
||
| Lineage events are sent to the location set by `defaultLocation` in `workflow_settings.yaml` or, if that is not set, by `location` in `.df-credentials.json`. If neither is set, lineage emission is skipped with a warning and the run proceeds normally. |
There was a problem hiding this comment.
This doesn't seem to match the code: cli/api/lineage/emitter.ts does const location = (this.credentials.location || "US").toLowerCase();, so when neither defaultLocation nor the credentials location is set, lineage is sent to US without a warning rather than skipped. Either this PR needs the skip-and-warn logic, or the README should describe the US fallback.
There was a problem hiding this comment.
This should be changed in the PR that this one is stacked on (#2393)
bfcabe5 to
5e533ec
Compare
|
I rebased this on top of #2408 which actually allows BigQuery to pick the location dynamically, so now the changes to README and CLI prompts are accurate. |
| location = "EU"; | ||
| } else if (locationIndex === 2) { | ||
| location = prompts.question("Enter the location's region name (e.g. 'asia-south1'):"); | ||
| if (!location) { |
There was a problem hiding this comment.
Consider calling .trim() on prompts.question("Enter the location's region name (e.g. 'asia-south1'):") (matching withServiceAccountToImpersonate below) so whitespace-only input is treated as empty and accidental surrounding spaces are stripped.
| | `--json` | `compile`, `test`, `run` | Requests JSON output instead of human-readable text. | | ||
| | `--timeout` | `compile`, `test`, `run` | Bounds project **compilation** time only (default `60s`). | | ||
|
|
||
| ## Credentials file |
There was a problem hiding this comment.
Under ## Credentials file, consider noting that "location" is optional in .df-credentials.json (omitted when "auto" is selected).
39ab153 to
60db22f
Compare
8251533 to
929abfe
Compare
Make location optional in the init and init-creds commands: