-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add support for AuthorizedView resource in Bigtable #10203
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @c2thorn, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Errors
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
d60e9b9
to
c970bb2
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Errors
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
|
||
authorizedViewId := d.Get("name").(string) | ||
tableId := d.Get("table_name").(string) | ||
authorizedViewConf := bigtable.AuthorizedViewConf{ |
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.
Apologies, I thought I left a comment but it must not have sent.
The build is failing with bigtable.AuthorizedViewConf
being undefined.
I believe we will have to update the cloud.google.com/go/bigtable
dependency
yaqs/8444818848342867968 should have a note that explains how to do that in our go.mod
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.
Sorry it took a while for us to merge the cloud.google.com/go/bigtable
dependency for this PR. Now the dependency is finally merged, and the go.mod
is updated accordingly.
@c2thorn Can you please start review this PR? Thanks!
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.
We should update the go.sum as gets generated as well. You should just be able to copy and paste the result after running go get cloud.google.com/go/bigtable
in the provider
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.
Thanks for the info! Done.
c970bb2
to
913946e
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Errors
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
55c63e4
to
6377c76
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigtable_authorized_view" "primary" {
project = # value needed
}
|
mmv1/third_party/terraform/services/bigtable/resource_bigtable_authorized_view.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/bigtable/resource_bigtable_authorized_view.go
Show resolved
Hide resolved
|
||
subsetView, ok := d.GetOk("subset_view") | ||
if !ok || len(subsetView.([]interface{})) != 1 { | ||
return fmt.Errorf("subset_view must be specified for authorized view %s", authorizedViewId) |
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.
Why do we have validation for subset_view
here, but have it set as optional in the field schema?
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.
Since the subset_view
is actually one variant in a one-of field in the AuthorizedView API proto as below:
// The type of this AuthorizedView.
oneof authorized_view {
// An AuthorizedView permitting access to an explicit subset of a Table.
SubsetView subset_view = 2;
}
Although it is currently the only possible variant in that one-of field, in the future we might add other types of authorized views. Therefore each one-of variant will be optional in the field schema.
However we also want to make sure that at least one of the possible variants are set, hence the validation here. In the future when we have other variants, we will adjust the validation accordingly.
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.
That makes sense. Refreshing to see considerations for future changes, thank you!
mmv1/third_party/terraform/services/bigtable/resource_bigtable_authorized_view_test.go
Show resolved
Hide resolved
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigtable_authorized_view" "primary" {
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccHealthcareDatasetIamPolicy |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigtable_authorized_view" "primary" {
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected
|
mmv1/third_party/terraform/website/docs/r/bigtable_authorized_view.html.markdown
Show resolved
Hide resolved
Don't see any other changes necessary, so I'm going to go ahead and commit my small suggestion to move things along. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigtable_authorized_view" "primary" {
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected
|
Looks like another PR was merged that modified the go.mod.erb file. May have to rerun the dependency upgrade with |
Done. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigtable_authorized_view" "primary" {
project = # value needed
}
|
1 similar comment
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_bigtable_authorized_view" "primary" {
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected
|
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected
|
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
…orm#10203) Co-authored-by: trollyxia <[email protected]>
…orm#10203) Co-authored-by: trollyxia <[email protected]>
…orm#10203) Co-authored-by: trollyxia <[email protected]>
…orm#10203) Co-authored-by: trollyxia <[email protected]>
…orm#10203) Co-authored-by: trollyxia <[email protected]>
Release Note Template for Downstream PRs (will be copied)