Alt approach for grant location #218
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
12 tasks
dbeatty10
approved these changes
Jul 11, 2022
There was a problem hiding this comment.
This feels like a great balance between implementing our generalized patterns while making the minimal amount of code changes.
👍 I feel good about adopting both components/approaches (1 and 2).
It seems like the only thing to determine is whether to raise an exception or leave as-is (without raising the exception).
|
|
||
| select privilege_type, grantee | ||
| from `{{ relation.project }}`.`region-{{ target.location }}`.INFORMATION_SCHEMA.OBJECT_PRIVILEGES | ||
| from {{ relation.information_schema("OBJECT_PRIVILEGES") }} |
Comment on lines
+82
to
+84
| def get_region_identifier(self) -> str: | ||
| region_id = f"region-{self.location}" | ||
| return self.quoted(region_id) |
dbt/adapters/bigquery/relation.py
Outdated
Comment on lines
86
to
90
| @classmethod | ||
| def from_relation(cls, relation, information_schema_view): | ||
| info_schema = super().from_relation(relation, information_schema_view) | ||
| if information_schema_view == "OBJECT_PRIVILEGES" and relation.location: | ||
| # TODO: raise an exception if relation.location is missing? |
There was a problem hiding this comment.
Are there any concerns about raising an exception?
It seems like either we raise the exception (hopefully with a useful message) or BigQuery will raise it during execution.
dbeatty10
reviewed
Jul 11, 2022
Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com>
…b.com/dbt-labs/dbt-bigquery into jerco/bq-grants-alt-location-approach
…dbt-labs/dbt-bigquery into jerco/bq-grants-alt-location-approach
emmyoop
added a commit
that referenced
this pull request
Jul 11, 2022
* Add get_show_grant_sql * first pass at granting with sql * more macro overrides * update macros, temp update to dev-req for CI * tweak to sql and added some TODOs * move things around * exclude session_user * fix mypy error, fix exclude to be more than users * simplify revoke logic * small cleanup, point to ct-660 branch * grant entire list in one statement * wip * wip, broken tests * Update dbt/include/bigquery/macros/materializations/incremental.sql Co-authored-by: Anders <anders.swanson@dbtlabs.com> * working on getting tests working, very WIP * All tests passing locally * Updated user / group names * Ongoing test cleanup * Account for refactor in dbt-labs/dbt-core@c763601 * Updated third value * Alt approach for grant location (#218) * Alt approach to grabbing + factoring info_schema region * add exception for blank location * Update dbt/adapters/bigquery/relation.py Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com> * point to existing branch * fix pre commit errors Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com> * reset to point dbt-core to main instead of branch * fix examples in test.env.example * add changelog Co-authored-by: Anders <anders.swanson@dbtlabs.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Doug Beatty <doug.beatty@dbtlabs.com> Co-authored-by: Doug Beatty <44704949+dbeatty10@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
builds off #212
Alternative approach, with two main components:
locationfrom the client's dataset reference, rather than requiring it in the user'sprofiles.yml. This value should always be available. It does require an API/client call, so we could look into either (a) caching it, or (b) taking the user-suppliedcredentials.locationif available—but the call is super quick one, and I'd vote better safe than sorry.locationas an attribute ofBigQueryRelation+BigQueryInformationSchema. Add it into the rendered form of{{ relation.information_schema() }}in a pretty hacky way. A better approach would require us to update lots of underlying class methods, to registerRegionas a realComponentName. This feels like a reasonable compromise for now, which still lets us benefit from therelation.information_schema()abstraction within dbt (Add Grants to BigQuery Materializations #212 (comment)).These approaches could be separated out, if (e.g.) we wanted to use 1 but not 2. I wasn't super neat about it in my commits, but it's not a lot of code.