-
Notifications
You must be signed in to change notification settings - Fork 121
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: add Dataset ACL support #1763
feat: add Dataset ACL support #1763
Conversation
Blocked by cl/421673680. Will update the generated client version once the CL is released. |
…ng>) based on pending discovery doc changes (cl/421673680)
…526/java-bigquery into authorize-datasets
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.
Mostly my concern is naming confusion, but the precedent was set before this change made.
@@ -119,6 +122,11 @@ public Type getType() { | |||
abstract Access toPb(); | |||
|
|||
static Entity fromPb(Access access) { | |||
if (access.getDataset() != null) { | |||
return new Dataset( |
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.
minor constructor surprise for me, until I realized we're dealing with an ACL.Dataset and not a Dataset. I suspect this is just local ambiguity; any other places where we need to consider the naming?
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.
Yeah I agree it's a little confusing so I renamed it to be more explicit: DatasetAclEntity
.
@@ -146,6 +154,64 @@ static Entity fromPb(Access access) { | |||
} | |||
} | |||
|
|||
/** | |||
* Class for a BigQuery Dataset entity. Objects of this class represent a dataset from a different |
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.
Maybe clarify this is a Dataset ACL entity, not a Dataset entity?
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.
Updated. Thanks!
🤖 I have created a release *beep* *boop* --- ## [2.8.0](v2.7.1...v2.8.0) (2022-02-02) ### Features * add Dataset ACL support ([#1763](#1763)) ([18a11e8](18a11e8)) ### Dependencies * update dependency com.google.apis:google-api-services-bigquery to v2-rev20220123-1.32.1 ([#1819](#1819)) ([82175f1](82175f1)) * update dependency com.google.cloud:google-cloud-bigtable to v2.5.2 ([#1821](#1821)) ([0fe0a78](0fe0a78)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fixes b/211633697