docs: add sample for revoking dataset access #778

loferris · 2021-07-16T22:44:17Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

[x ] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
[x ] Ensure the tests and linter pass
[x ] Code coverage does not decrease (if any source code was changed)
[x ] Appropriate docs were updated (if necessary)

snippet-bot · 2021-07-16T22:44:21Z

Here is the summary of changes.

You are about to add 4 region tags.

samples/snippets/revoke_dataset_access.py:20, tag bigquery_revoke_dataset_access
samples/snippets/revoke_dataset_access.py:30, tag bigquery_revoke_dataset_access
samples/snippets/update_dataset_access.py:20, tag bigquery_update_dataset_access
samples/snippets/update_dataset_access.py:49, tag bigquery_update_dataset_access

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

plamut

Found a few things that could probably be improved, although nothing really major.

samples/revoke_dataset_access.py

tswast · 2021-07-19T14:49:21Z

samples/revoke_dataset_access.py

+    client = bigquery.Client()
+
+    # TODO(developer): Set dataset_id to the ID of the dataset to fetch.
+    # dataset_id = 'your-project.your_dataset'


I've gotten docs feedback from users skip over this comments and get confused. Could we instead follow this pattern where we set a real value and then immediately unset it?

https://github.com/googleapis/python-bigquery-storage/blob/fb3d0bb7b195c33f8ae602d9e22f612048581891/samples/to_dataframe/read_table_bqstorage.py#L17-L21

I didn't add the TODO comment in that BQ Storage sample, but I do like that you have it.

tswast · 2021-07-19T14:51:09Z

samples/revoke_dataset_access.py

+    entries = list(dataset.access_entries)
+
+    for entry in entries:
+        if entry.entity_id == entity_id:


You use entity_id without showing an example value. Each code sample should be as copy-pasteable as possible. Add the setter for this by the setter for dataset_id.

samples/revoke_dataset_access.py

… formatting

…ython-bigquery into bigquery_remove_view_access merging in changes from remote branch

…hange parameter pattern for readibility

loferris · 2021-07-23T22:48:51Z

@tswast @plamut
I've updated the PR with commits addressing the above comments!

plamut

Looks good, thanks for making the changes.

I'll let Tim comment on START/END section naming (in case there are some implicit conventions), although the changes look consistent with the linked pattern to me.

plamut

Whoops, it appears we need to add __init__.py to samples/snippets, it's currently not present:

/usr/local/lib/python3.6/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
revoke_dataset_access_test.py:15: in <module>
    from .. import update_dataset_access
E   ImportError: attempted relative import with no known parent package

We'll also need to adjust the imports in the test, because both files are now in the same directory. The following will fail now:

from .. import revoke_dataset_access

tswast · 2021-07-26T14:11:30Z

Whoops, it appears we need to add init.py to samples/snippets, it's currently not present:

If we do add it, then we'll need to change all the other tests to in sample/snippets to use relative imports as well.

plamut · 2021-07-26T14:17:22Z

Whoops, it appears we need to add init.py to samples/snippets, it's currently not present:

If we do add it, then we'll need to change all the other tests to in sample/snippets to use relative imports as well.

Indeed. It's a bit more (one-time) work, but it will make the snippets/ subpackage consistent with geography/.

tswast · 2021-07-26T14:20:29Z

samples/snippets/revoke_dataset_access_test.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from .. import update_dataset_access


As mentioned in the comment, adding samples/snippets/__init__.py should fix this, but it'll break the other sample tests. You'll need to also update the tests in samples/snippets/ to use relative imports.

Alternatively, we can start making progress on migrating the snippets to samples/snippets (#790). You could make a copy of update_dataset_access.py and it's corresponding tests and put them in the samples/snippets directory in this PR. Then once you've pointed cloud.google.com at the new sample location, send a PR to remove the duplicate sample.

I like the latter approach!

done in latest commit!

tswast · 2021-07-26T14:24:06Z

samples/snippets/revoke_dataset_access_test.py

+# limitations under the License.
+
+from .. import update_dataset_access
+from .. import revoke_dataset_access


This should be from . import revoke_dataset_access since it's the same directory as the test. (Also will need __init__.py for that to work.) If you go with the "copy" alternative (without __init__.py), then this should be import revoke_dataset_access.

tswast · 2021-07-26T14:24:55Z

samples/snippets/revoke_dataset_access_test.py

+    revoke_dataset_access.revoke_dataset_access(dataset_id, entity_id)
+
+    out, err = capsys.readouterr()
+    assert f"Updated dataset '{dataset_id}' with modified user permissions." in out


Could you look for a message specific to "revoke"? I worry that this could catch the original update and the revoke could be failing.

Also, let's add some additional steps to call get_dataset and check that the dataset access was actually updated.

would you recommend the pytest fixture pattern to create a bigquery client that I can call this on? I'm seeing it in some other snippets but not the update_access sample. I also need to move get_dataset to /snippets as well!

samples/snippets/revoke_dataset_access.py

tswast · 2021-07-26T14:42:45Z

samples/snippets/revoke_dataset_access.py

+    entries = list(dataset.access_entries)
+
+    for entry in entries:
+        if entry.entity_id == entity_id:


Do you think we should we look at entity_type, too? Maybe not? I suppose there shouldn't be much conflict between whether an email address is associated with a group, service account, or user.

once I get the tests updated I'll play around with it.

tswast · 2021-07-26T14:44:02Z

samples/snippets/revoke_dataset_access.py

+    dataset = client.update_dataset(dataset, ["access_entries"])  # Make an API request.
+
+    full_dataset_id = f"{dataset.project}.{dataset.dataset_id}"
+    print(f"Updated dataset '{full_dataset_id}' with modified user permissions.")


Including the word "revoked" and possibly the entity ID here could give us a better idea that this actually worked.

samples/snippets/revoke_dataset_access.py

tswast · 2021-07-26T14:55:25Z

samples/snippets/revoke_dataset_access.py

+
+    for entry in entries:
+        if entry.entity_id == entity_id:
+            entry.role = None


I'm curious to see if this works as expected. The alternative is to remove the whole entry from the list.

I'm going to remove it...

…ke_access and update_access

removed nested START/END tags Co-authored-by: Tim Swast <swast@google.com>

update readability in API request Co-authored-by: Tim Swast <swast@google.com>

loferris · 2021-11-22T19:38:23Z

Hi @tswast This is ready for a final/next review!

tswast · 2021-11-23T16:36:35Z

samples/snippets/revoke_dataset_access.py

+    )
+
+    entries = list(dataset.access_entries)
+    entries.append(entry)


I'm a bit confused as to why we'd add the entry and then remove it on line 42. Perhaps a copy-paste error?

I think it's a redundancy - I was adding a specific entry to alter but I think that'll be covered in the test case and sort of muddies the water in the sample itself. I'll refactor.

tswast · 2021-11-23T16:37:32Z

samples/snippets/revoke_dataset_access.py

+    entry = bigquery.AccessEntry(
+        role="READER",
+        entity_type="groupByEmail",
+        entity_id="cloud-developer-relations@google.com",


Should this be entity_id=entity_id?

Makes sense to me!

tswast · 2021-11-23T16:38:48Z

samples/snippets/revoke_dataset_access.py

+
+    entry = bigquery.AccessEntry(
+        role="READER",
+        entity_type="groupByEmail",


We should add a comment saying that the developer will need to set this to the correct type.

Although, perhaps we can delete this whole line 32-36? A code sample should only contain the logic necessary for the developer to understand the concept we're trying to teach them.

I took it out!

samples/snippets/revoke_dataset_access.py

tswast · 2021-11-23T16:56:34Z

samples/snippets/update_dataset_access.py

+    entry = bigquery.AccessEntry(
+        role="READER",
+        entity_type="groupByEmail",
+        entity_id="cloud-developer-relations@google.com",


Do we want to use the entity_id fixture here? If so, we should add a variable above.

Added, changed test as well!

tswast · 2021-11-23T16:57:12Z

samples/snippets/update_dataset_access.py

+
+    entry = bigquery.AccessEntry(
+        role="READER",
+        entity_type="groupByEmail",


This needs a comment explaining the various options available, or at least a link to the docs where these are described.

samples/snippets/update_dataset_access.py

tswast · 2021-11-23T16:58:11Z

samples/snippets/dataset_access_permissions_test.py

@@ -0,0 +1,49 @@
+# Copyright 2021 Google LLC


Nit: In the filename, "access" and "permissions" are redundant. Let's call it dataset_access_test.py.

Done in latest commit!

tswast · 2021-11-23T16:58:30Z

samples/snippets/README.rst

@@ -48,7 +47,7 @@ Install Dependencies
   .. _Python Development Environment Setup Guide:
       https://cloud.google.com/python/setup

-#. Create a virtualenv. Samples are compatible with Python 3.6+.
+#. Create a virtualenv. Samples are compatible with Python 2.7 and 3.4+.


@loferris Please revert the changes to this file.

Co-authored-by: Tim Swast <swast@google.com>

loferris · 2021-11-24T01:26:56Z

All comments addressed! @tswast

samples/snippets/README.rst

samples/snippets/dataset_access_test.py

tswast · 2021-11-29T17:10:49Z

samples/snippets/update_dataset_access.py

+    client = bigquery.Client()
+
+    # TODO(developer): Set dataset_id to the ID of the dataset to fetch.
+    # dataset_id = 'your-project.your_dataset'


Looks like we missed this one.

Co-authored-by: Tim Swast <swast@google.com>

tswast

LGTM. I've pushed some minor updates to the comments.

tswast · 2021-12-06T16:29:14Z

samples/snippets/revoke_dataset_access.py

+    dataset = client.get_dataset(dataset_id)  # Make an API request.
+
+    entries = list(dataset.access_entries)
+    dataset.access_entries = entries


This line confuses me. It gets overwritten in the next line of code, so looks unnecessary. Possibly leftover from a debugging?

stale

* revoke dataset access setup * basic template for sample * sample + test * revoke dataset access sample * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * docs: add sample for revoking dataset access - update year and string formatting * docs: add sample for revoking dataset access - move to snippets and change parameter pattern for readibility * moving update_dataset to /snippets and adjusting imports on both revoke_access and update_access * Update samples/snippets/revoke_dataset_access.py removed nested START/END tags Co-authored-by: Tim Swast <swast@google.com> * Update samples/snippets/revoke_dataset_access.py update readability in API request Co-authored-by: Tim Swast <swast@google.com> * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * updated test * 🦉 Updates from OwlBot See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md * change after running test * resolving linting failure, rewriting test * removed relative import errors * remove relative mport from update_dataset_access * adding fixture to conftest.py * updated sample * updating sample to match new update_access sample * fixing region tags * consolidated tests into one file for both methods * updating test to full_dataset format * updated revoke sample * updating test * refactored sample * Update samples/snippets/conftest.py * Update samples/snippets/revoke_dataset_access.py Co-authored-by: Tim Swast <swast@google.com> * Update samples/snippets/update_dataset_access.py Co-authored-by: Tim Swast <swast@google.com> * Update samples/snippets/revoke_dataset_access.py Co-authored-by: Tim Swast <swast@google.com> * Update samples/snippets/revoke_dataset_access.py Co-authored-by: Tim Swast <swast@google.com> * refactoring entry * added comment for entry access * Update samples/snippets/README.rst Co-authored-by: Tim Swast <swast@google.com> * Update samples/snippets/dataset_access_test.py Co-authored-by: Tim Swast <swast@google.com> * Update samples/snippets/dataset_access_test.py Co-authored-by: Tim Swast <swast@google.com> * added develper TODO in sample * add comments to samples Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Tim Swast <swast@google.com> Co-authored-by: Peter Lamut <plamut@users.noreply.github.com> Co-authored-by: Anthonios Partheniou <partheniou@google.com> Co-authored-by: meredithslota <meredithslota@google.com>

Lo Ferris added 4 commits July 1, 2021 16:50

revoke dataset access setup

0abac4c

basic template for sample

d699bec

sample + test

98b245d

revoke dataset access sample

f1adee7

loferris requested a review from a team as a code owner July 16, 2021 22:44

loferris requested review from crwilcox and removed request for a team July 16, 2021 22:44

product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Jul 16, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 16, 2021

🦉 Updates from OwlBot

e729bbd

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

plamut reviewed Jul 19, 2021

View reviewed changes

samples/revoke_dataset_access.py Outdated Show resolved Hide resolved

samples/revoke_dataset_access.py Outdated Show resolved Hide resolved

samples/revoke_dataset_access.py Outdated Show resolved Hide resolved

plamut added the samples Issues that are directly related to samples. label Jul 19, 2021

plamut changed the title ~~feat bigquery remove view access~~ docs: add sample for revoking dataset access Jul 19, 2021

tswast requested changes Jul 19, 2021

View reviewed changes

Lo Ferris added 3 commits July 23, 2021 14:39

docs: add sample for revoking dataset access - update year and string…

b80c4bf

… formatting

Merge branch 'bigquery_remove_view_access' of github.com:googleapis/p…

2960b48

…ython-bigquery into bigquery_remove_view_access merging in changes from remote branch

docs: add sample for revoking dataset access - move to snippets and c…

02bd2b4

…hange parameter pattern for readibility

plamut approved these changes Jul 26, 2021

View reviewed changes

plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 26, 2021

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 26, 2021

plamut previously requested changes Jul 26, 2021

View reviewed changes

tswast requested changes Jul 26, 2021

View reviewed changes

tswast reviewed Jul 26, 2021

View reviewed changes

Lo Ferris and others added 3 commits July 27, 2021 13:45

moving update_dataset to /snippets and adjusting imports on both revo…

fa0e48c

…ke_access and update_access

Update samples/snippets/revoke_dataset_access.py

98bb4a2

removed nested START/END tags Co-authored-by: Tim Swast <swast@google.com>

Update samples/snippets/revoke_dataset_access.py

8c4bc7a

update readability in API request Co-authored-by: Tim Swast <swast@google.com>

Lo Ferris added 2 commits November 19, 2021 16:14

updating test

adc1c76

refactored sample

62ec25e

tswast self-requested a review November 23, 2021 16:32

Update samples/snippets/conftest.py

072b785

tswast requested changes Nov 23, 2021

View reviewed changes

loferris and others added 6 commits November 23, 2021 09:49

Update samples/snippets/revoke_dataset_access.py

dade3b8

Co-authored-by: Tim Swast <swast@google.com>

Update samples/snippets/update_dataset_access.py

c74022e

Co-authored-by: Tim Swast <swast@google.com>

Update samples/snippets/revoke_dataset_access.py

9da6de6

Co-authored-by: Tim Swast <swast@google.com>

Update samples/snippets/revoke_dataset_access.py

ede9158

Co-authored-by: Tim Swast <swast@google.com>

refactoring entry

05a4f19

added comment for entry access

6c04f28

loferris self-assigned this Nov 24, 2021

tswast self-requested a review November 29, 2021 17:02

Merge branch 'main' into bigquery_remove_view_access

d030e21

tswast requested changes Nov 29, 2021

View reviewed changes

loferris and others added 4 commits November 29, 2021 11:34

Update samples/snippets/README.rst

023eb96

Co-authored-by: Tim Swast <swast@google.com>

Update samples/snippets/dataset_access_test.py

6d9f274

Co-authored-by: Tim Swast <swast@google.com>

Update samples/snippets/dataset_access_test.py

3fd6856

Co-authored-by: Tim Swast <swast@google.com>

added develper TODO in sample

6ce391d

loferris requested a review from tswast November 29, 2021 19:49

add comments to samples

04497a1

tswast approved these changes Dec 6, 2021

View reviewed changes

Merge branch 'main' into bigquery_remove_view_access

2e98eca

tswast requested a review from plamut December 6, 2021 16:52

tswast merged commit effd673 into main Dec 6, 2021

tswast deleted the bigquery_remove_view_access branch December 6, 2021 17:50

docs: add sample for revoking dataset access #778

docs: add sample for revoking dataset access #778

Conversation

loferris commented Jul 16, 2021

snippet-bot bot commented Jul 16, 2021 • edited Loading

plamut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loferris commented Jul 23, 2021

plamut left a comment

Choose a reason for hiding this comment

plamut left a comment

Choose a reason for hiding this comment

tswast commented Jul 26, 2021

plamut commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loferris commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loferris commented Nov 24, 2021

Choose a reason for hiding this comment

tswast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Jul 16, 2021 •

edited

Loading