drop the grants of authorized view when it's full refresh #1189
Conversation
salimmoulouel
force-pushed
the
bug/updating_of_grant_access_on_full_refresh
branch
11 times, most recently
from
1905626
to
8b46b6c
Compare
salimmoulouel
force-pushed
the
bug/updating_of_grant_access_on_full_refresh
branch
from
8b46b6c
to
9ae626d
Compare
dbt/adapters/bigquery/dataset.py
Outdated
| dataset (Dataset): the dataset to be updated | ||
| access_entry (AccessEntry): the access entry to be removed from the dataset | ||
| """ | ||
| access_entries = list(dataset.access_entries) |
There was a problem hiding this comment.
Why do we need to coerce this to be a list?
There was a problem hiding this comment.
It was a precautionary measure, and you're correct, it turned out to be unnecessary.
dbt/adapters/bigquery/impl.py
Outdated
| dataset = add_access_entry_to_dataset(dataset, access_entry) | ||
| dataset = client.update_dataset(dataset, ["access_entries"]) | ||
| full_dataset_id = f"{dataset.project}.{dataset.dataset_id}" | ||
| print(f"allowed dataset access for '{access_entry.entity_id}' to ' dataset '{full_dataset_id}.'") |
dbt/adapters/bigquery/impl.py
Outdated
| ) # Make an API request. | ||
|
|
||
| full_dataset_id = f"{dataset.project}.{dataset.dataset_id}" | ||
| print(f"Revoked dataset access for '{access_entry.entity_id}' to ' dataset '{full_dataset_id}.'") |
There was a problem hiding this comment.
swap print for logger.info
|
Mind adding another functional test? |
salimmoulouel
force-pushed
the
bug/updating_of_grant_access_on_full_refresh
branch
2 times, most recently
from
18e55ce
to
6f9632a
Compare
salimmoulouel
force-pushed
the
bug/updating_of_grant_access_on_full_refresh
branch
from
6f9632a
to
b3ef011
Compare
|
Thank you for your feedback, Colin. I tried to address all the points you raised in your review. |
salimmoulouel
force-pushed
the
bug/updating_of_grant_access_on_full_refresh
branch
from
1744e65
to
adeefeb
Compare
| # Need to run twice to validate idempotency | ||
| results = run_dbt(["run"]) | ||
| assert len(results) == 2 | ||
| time.sleep(10) |
There was a problem hiding this comment.
Why do we need the sleep call?
There was a problem hiding this comment.
I don't know exactly why they used it, I found the sleep call already existing in the not full-refresh call of the function, I reproduce it by precaution.
resolves #1175
Problem
When you delete a view in BigQuery, all associated dataset grant access are lost because they are tied to the view's ID rather than its name. Consequently, during a full refresh that involves dropping the view, the old grants are not automatically removed and new ones must be created. This oversight can lead to various bugs and inconsistencies.
Solution
Delete the grant to the dataset when we are on full refresh
Checklist