-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(bigquery): Add cloud-client samples for access policies #3975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(bigquery): Add cloud-client samples for access policies #3975
Conversation
- Configure main app.js entry point - Configure viewDatasetAccessPolicy.js file
- Add viewTableOrViewAccessPolicy to the app.js entry point - Configure viewTableOrViewAccessPolicy.js file
|
Here is the summary of changes. You are about to add 6 region tags.
This comment is generated by snippet-bot.
|
…ataset-access-policy
…ix linting errors
…eOrViewAccess.test.js
…ataset-access-policy
eapl-gemugami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments. Please let me know if you require further details.
eapl-gemugami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving a few comments, let me know if you need further details!
…ataset-access-policy
eapl-gemugami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments for the latest commit.
eapl-gemugami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sharing a few comments on Sample formatting and scope of used variables inside the region/block.
bigquery/cloud-client/package.json
Outdated
| "version": "0.0.1", | ||
| "private": true, | ||
| "license": "Apache Version 2.0", | ||
| "author": "Google Inc.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I've seen, the author should be 'Google LLC', as in:
nodejs-docs-samples/.mocharc.js
Line 1 in e542724
| // Copyright 2020 Google LLC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was checking the package.json files of other samples and all of them have Google Inc. in the author part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These reference package.json files using Google Inc. might be outdated, see renderer/package.json#L6 from 2020 or retail/package.json#L4) from 2021
Also check b/169619920
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, you could ask your NodeJS lead for advice.
eapl-gemugami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for minor details found in last commit.
bigquery/cloud-client/package.json
Outdated
| "version": "0.0.1", | ||
| "private": true, | ||
| "license": "Apache Version 2.0", | ||
| "author": "Google Inc.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These reference package.json files using Google Inc. might be outdated, see renderer/package.json#L6 from 2020 or retail/package.json#L4) from 2021
Also check b/169619920
bigquery/cloud-client/package.json
Outdated
| "version": "0.0.1", | ||
| "private": true, | ||
| "license": "Apache Version 2.0", | ||
| "author": "Google Inc.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, you could ask your NodeJS lead for advice.
| } | ||
|
|
||
| if (principalToRemove) { | ||
| // The `bindings` list is immutable. Create a copy for modifications. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
I added that comment on the Python Sample to warn the developer that they can't modify the list but have to create a new one, as working on the current list was the 'obvious' thing to do.
That was required on Python, after some research on the source code and struggling to make the sample work, is it the same case for Node?
My only concern would be to create a new list if it's not necessary on Node cloud-client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike Python, creating a new list in Node is not strictly necessary for this operation to work. JS doesn't have the same restrictions as Python when modifying list during iteration.
However, creating a copy with const bindings = [...policy.bindings]; is still recommended because it prevents potential issues if the BQ Node client has any internal expectations about immutability of the policy object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the record: Besides having problems modifying a list during an iteration, the list on Python is immutable due to the way the bindings were designed:
iam.py#L141-L148
From what I can see here types.d.ts#L3683 and there types.d.ts#L1752-L1756, Node client is more relaxed on how to work the list, but I can't confirm if it's immutable.
Anyway, if you are not sure about the binding list being immutable, to avoid misleading the developer you could just leave:
// Create a copy of the binding list for modifications.
eapl-gemugami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Add sample for viewing access policy to dataset
This PR has been divided into the following 3:
#4023
#4024
#4025
Step of Internal: b/394478489
Checklist
npm test(see Testing)npm run lint(see Style)