-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[#16607] Fix: support iamMember (i.e. for WIF ids) in big query datasets #9948
[#16607] Fix: support iamMember (i.e. for WIF ids) in big query datasets #9948
Conversation
Hello! I am a robot. It looks like you are a: @ScottSuarez, 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. Terraform GA: Diff ( 2 files changed, 8 insertions(+), 5 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packages
|
Please wait to merge this. Adding tests and fixing a few things. |
3520b86
to
b556cb0
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. Terraform GA: Diff ( 4 files changed, 77 insertions(+), 14 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigqueryDatasetIamMember_iamMember|TestAccBigqueryDatasetIamMember_serviceAccount|TestAccWorkstationsWorkstationConfig_updateHostDetails |
Rerun these tests in REPLAYING mode to catch issues
|
@rileykarson @ScottSuarez this is ready for review on my side but apparently the test here is failing. I cannot reproduce the error on Can you please help me to figure out what's going on? Thank you! |
case "user": | ||
return "userByEmail", pieces[1], nil | ||
case "serviceAccount": | ||
return "userByEmail", pieces[1], nil | ||
default: | ||
return "", "", fmt.Errorf("Failed to parse BigQuery Dataset IAM member type: %s", member) | ||
} | ||
} | ||
if member == "projectOwners" || member == "projectReaders" || member == "projectWriters" || member == "allAuthenticatedUsers" { |
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.
Opportunistic refactoring/fix opportunity:
1- projectWriters, projectReaders, projectOwners are not documented as possible output https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/bigquery_dataset_iam#google_bigquery_dataset_iam_member
2- allUsers is documented but doesn't look to be a case handled by the API (?). https://cloud.google.com/bigquery/docs/reference/rest/v2/datasets#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.
Feel free to ignore this comment if not relevant / I lack of context but I just ping you in case you missed this comment @LucaPrete .
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.
Hey, sorry..I missed this. We can certainly work on this in a separate PR.
This may be due to an existing bug with I'll try running again to see if it is consistent |
/gcbrun |
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. Terraform GA: Diff ( 4 files changed, 77 insertions(+), 14 deletions(-)) |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigqueryDatasetIamMember_iamMember|TestAccWorkstationsWorkstationConfig_updateHostDetails |
|
Hello @c2thorn do we know why this is still failing? We've a p1 open and we'd like to go through the review/merge as soon as we can. Please, let me know if there's anything I can do from my side. |
@LucaPrete I do not know why it is failing, I believe it is an existing bug as seen in hashicorp/terraform-provider-google#17172 Nothing from the test config for your addition points to a bug that I can see, but I'm not going to be able to merge this yet until we solve it. I won't have time to do a deep debugging dive until late tomorrow. I can run the test in a different environment to see if it environment-specific if you aren't able to recreate it. |
Test failed again. @LucaPrete do you have a debug log of a successful run? |
mmv1/third_party/terraform/services/bigquery/resource_bigquery_dataset_iam_member_test.go
Show resolved
Hide resolved
99654b3
to
801b1f0
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. Terraform GA: Diff ( 4 files changed, 78 insertions(+), 16 deletions(-)) |
@c2thorn it should be all fixed now! |
Tests analyticsTotal tests: Click here to see the affected service packagesall service packages are affected Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigqueryDatasetIamMember_iamMember|TestAccWorkstationsWorkstationConfig_updateHostDetails |
|
…ets (GoogleCloudPlatform#9948) Co-authored-by: Luca Prete <[email protected]>
…ets (GoogleCloudPlatform#9948) Co-authored-by: Luca Prete <[email protected]>
…ets (GoogleCloudPlatform#9948) Co-authored-by: Luca Prete <[email protected]>
…ets (GoogleCloudPlatform#9948) Co-authored-by: Luca Prete <[email protected]>
Fix: allow users to set permissions for
principal
/principalSets
(iamMember
) in BigQuery datasets.Fixes github.com/hashicorp/terraform-provider-google/issues/16607