Adding IAM support in Terraform for Dataform Repository resource #9457

maverickjoy · 2023-11-13T11:27:21Z

Adding IAM support in Terraform for Dataform Repository resource
Internal tracking bug: b/309433506

Product Resource API Documentation: https://cloud.google.com/dataform/reference/rest/v1beta1/projects.locations.repositories

Did Manual Testing and it adds IAM policy to the repository added screenshots to the internal bug.

Release Note Template for Downstream PRs (will be copied)

dataform: added `google_dataform_repository_iam_*` IAM resources (beta)

modular-magician · 2023-11-13T11:28:57Z

Hello! I am a robot. It looks like you are a: ~~Community Contributor~~ Googler ~~Core Contributor~~. Tests will run automatically.

@SarahFrench, 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.

modular-magician · 2023-11-13T11:38:21Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 220 insertions(+))
Terraform Beta: Diff ( 5 files changed, 870 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 371 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_dataform_repository_iam_binding (2 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_binding" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

Resource: google_dataform_repository_iam_member (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_member" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

SarahFrench

Hi @maverickjoy , thanks for your contribution!

I can see that adding the YAML to trigger generation of the IAM resources has resulted in acceptance tests for those IAM resources being generated only in the Beta version of the provider. This is because the first example in this file is used to generate the IAM tests, and that example is marked as beta-only. Could you please check if that test is able to be promoted to the GA provider too?

There's currently a problem in the generated acceptance tests e.g. :

iam_dataform_repository_generated_test.go:47:24 : fmt.Sprintf format %s reads arg #3, but call has 2 args

I believe the way to solve this is to add a primary_resource_name property to the first example, similar to this example. The value of primary_resource_name will be templated into the generate tests and provide the missing argument to fmt.Sprintf

SarahFrench · 2023-11-13T12:02:40Z

Also, I'm not a Googler so I won't be able to view the internal ticket I'm afraid! If there's any information relevant to this PR that can't be shared publicly let me know and we can figure out next steps

maverickjoy · 2023-11-13T12:33:50Z

Hi @maverickjoy , thanks for your contribution!

I can see that adding the YAML to trigger generation of the IAM resources has resulted in acceptance tests for those IAM resources being generated only in the Beta version of the provider. This is because the first example in this file is used to generate the IAM tests, and that example is marked as beta-only. Could you please check if that test is able to be promoted to the GA provider too?

There's currently a problem in the generated acceptance tests e.g. :
iam_dataform_repository_generated_test.go:47:24 : fmt.Sprintf format %s reads arg #3, but call has 2 args
I believe the way to solve this is to add a primary_resource_name property to the first example, similar to this example. The value of primary_resource_name will be templated into the generate tests and provide the missing argument to fmt.Sprintf

Thanks Sarah, for the review, honestly I am not sure if example should be added to GA since the resource is not published to GA itself ? as it min version is beta :

magic-modules/mmv1/products/dataform/Repository.yaml

Line 20 in 6ba1551

min_version: beta

Thanks for the example, I have added it.

maverickjoy · 2023-11-13T12:38:47Z

Also, I'm not a Googler so I won't be able to view the internal ticket I'm afraid! If there's any information relevant to this PR that can't be shared publicly let me know and we can figure out next steps

Nothing major as such, Its just some screenshots links which I had added in there, I can't attach them here, as the screenshot web page is internal to Google, so wont get opened from outside, but to summarise.
I generated the provider, ran make build on provider and then, created a terraform file and ran commands :

TF_CLI_CONFIG_FILE="./tf-dev-override.tfrc" terraform init -upgrade
TF_CLI_CONFIG_FILE="./tf-dev-override.tfrc" terraform plan
TF_CLI_CONFIG_FILE="./tf-dev-override.tfrc" terraform apply

And through curl command was able to see the IAM policy getting added for a user.

modular-magician · 2023-11-13T12:41:50Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 220 insertions(+))
Terraform Beta: Diff ( 5 files changed, 870 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 371 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_dataform_repository_iam_binding (2 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_binding" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

Resource: google_dataform_repository_iam_member (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_member" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

SarahFrench · 2023-11-13T12:57:22Z

I am not sure if example should be added to GA since the resource is not published to GA itself

Ah I didn't spot that the resource is only managed by a beta API, in that case yes lets leave the example as beta only and we can ignore the Missing test report automation.

SarahFrench

Following my last comment, please add min_version: beta to the iam_policy block - this will mean the IAM resources are only generated in the Beta provider, similar to the Repository resource and the generated IAM acceptance tests being beta-only.

maverickjoy · 2023-11-13T13:12:21Z

Following my last comment, please add min_version: beta to the iam_policy block - this will mean the IAM resources are only generated in the Beta provider, similar to the Repository resource and the generated IAM acceptance tests being beta-only.

For sure, Thank You

modular-magician · 2023-11-13T13:20:37Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 220 insertions(+))
Terraform Beta: Diff ( 5 files changed, 870 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 371 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_dataform_repository_iam_binding (2 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_binding" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

Resource: google_dataform_repository_iam_member (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_member" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

modular-magician · 2023-11-13T14:32:00Z

Tests analytics

Total tests: 3227
Passed tests 2895
Skipped tests: 329
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataformRepositoryIamBindingGenerated|TestAccDataformRepositoryIamPolicyGenerated|TestAccDataformRepositoryIamMemberGenerated

Get to know how VCR tests work

modular-magician · 2023-11-13T14:34:59Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccDataformRepositoryIamBindingGenerated[Error message] [Debug log]
TestAccDataformRepositoryIamPolicyGenerated[Error message] [Debug log]
TestAccDataformRepositoryIamMemberGenerated[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

SarahFrench · 2023-11-13T17:28:53Z

Ok, there's a few problems happening with the acceptance tests.

Firstly, TestAccDataformRepositoryIamMemberGenerated & TestAccDataformRepositoryIamPolicyGenerated are failing because they both provision a secret with a static, hardcoded name, so tests will clash and cause errors like the one below:

Error: Error creating Secret: googleapi: Error 409: Secret [projects/[PROJECT_NUM]/secrets/secret] already exists.

This is due to an already existing problem with the first example in the Repository.yaml file. I've made a PR to fix this here #9462 that you could either pull into this PR or you could rebase your PR once it's merged to main.

Next, something is affecting the TestAccDataformRepositoryIamBindingGenerated test's second step where it tests performing terraform import on the google_dataform_repository_iam_binding resource. It's less clear what the problem is with this and I can do some manual testing to see if something is missing.

vcr_utils.go:152: Failed state verification, resource with ID projects/[PROJECT_ID]/locations/us-central1/repositories/tf_test_repository4kcgewhaul/roles/viewer not found

Edit:

I did some manual tests about importing and found that I was able to import IAM resources fine, so I'm not sure why the TestAccDataformRepositoryIamBindingGenerated test is failing this way. I'll have another look tomorrow!

maverickjoy · 2023-11-14T13:27:25Z

Ok, there's a few problems happening with the acceptance tests.

Firstly, TestAccDataformRepositoryIamMemberGenerated & TestAccDataformRepositoryIamPolicyGenerated are failing because they both provision a secret with a static, hardcoded name, so tests will clash and cause errors like the one below:

Error: Error creating Secret: googleapi: Error 409: Secret [projects/[PROJECT_NUM]/secrets/secret] already exists.

This is due to an already existing problem with the first example in the Repository.yaml file. I've made a PR to fix this here #9462 that you could either pull into this PR or you could rebase your PR once it's merged to main.

Next, something is affecting the TestAccDataformRepositoryIamBindingGenerated test's second step where it tests performing terraform import on the google_dataform_repository_iam_binding resource. It's less clear what the problem is with this and I can do some manual testing to see if something is missing.

vcr_utils.go:152: Failed state verification, resource with ID projects/[PROJECT_ID]/locations/us-central1/repositories/tf_test_repository4kcgewhaul/roles/viewer not found

Edit:

I did some manual tests about importing and found that I was able to import IAM resources fine, so I'm not sure why the TestAccDataformRepositoryIamBindingGenerated test is failing this way. I'll have another look tomorrow!

Sure, Thanks for looking into this and creating that PR, Ill also ping the guy for the PR you had raised, they may have some insights as well for this.

SarahFrench · 2023-11-14T15:29:01Z

That PR's just been merged into main, so if you rebase or pull main into your branch we can re-trigger the tests and see if anything related to the IAM resources requires fixing 🤞

modular-magician · 2023-11-14T16:04:15Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 220 insertions(+))
Terraform Beta: Diff ( 5 files changed, 870 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 371 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_dataform_repository_iam_binding (2 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_binding" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

Resource: google_dataform_repository_iam_member (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_member" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

modular-magician · 2023-11-14T17:16:14Z

Tests analytics

Total tests: 3229
Passed tests 2896
Skipped tests: 329
Affected tests: 4

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataformRepositoryIamBindingGenerated|TestAccDataformRepositoryIamMemberGenerated|TestAccDataformRepositoryIamPolicyGenerated|TestAccSpannerDatabaseIamPolicy

Get to know how VCR tests work

modular-magician · 2023-11-14T17:21:43Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccSpannerDatabaseIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccDataformRepositoryIamBindingGenerated[Error message] [Debug log]
TestAccDataformRepositoryIamMemberGenerated[Error message] [Debug log]
TestAccDataformRepositoryIamPolicyGenerated[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

maverickjoy · 2023-11-15T12:27:26Z

Hey @SarahFrench , Not sure if this is a transient issue or what possibly could be the reason for this, any idea what could be done now for this ?

mmv1/products/dataform/Repository.yaml

modular-magician · 2023-11-15T13:20:24Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 220 insertions(+))
Terraform Beta: Diff ( 5 files changed, 870 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 2 files changed, 371 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_dataform_repository_iam_binding (2 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_binding" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

Resource: google_dataform_repository_iam_member (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_dataform_repository_iam_member" "primary" {
  condition {
    description = # value needed
    expression  = # value needed
    title       = # value needed
  }
}

modular-magician · 2023-11-15T14:35:21Z

Tests analytics

Total tests: 3229
Passed tests 2892
Skipped tests: 329
Affected tests: 8

Action taken

Found 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAttachedCluster_containerAttachedClusterFullExample|TestAccContainerAttachedCluster_update|TestAccDataformRepositoryIamBindingGenerated|TestAccDataformRepositoryIamPolicyGenerated|TestAccDataformRepositoryIamMemberGenerated|TestAccDataprocClusterIamPolicy|TestAccDataprocJobIamPolicy|TestAccSpannerDatabaseIamPolicy

Get to know how VCR tests work

modular-magician · 2023-11-15T14:52:25Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerAttachedCluster_containerAttachedClusterFullExample[Debug log]
TestAccContainerAttachedCluster_update[Debug log]
TestAccDataformRepositoryIamBindingGenerated[Debug log]
TestAccDataformRepositoryIamPolicyGenerated[Debug log]
TestAccDataformRepositoryIamMemberGenerated[Debug log]
TestAccDataprocClusterIamPolicy[Debug log]
TestAccDataprocJobIamPolicy[Debug log]
TestAccSpannerDatabaseIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

SarahFrench

Tests pass! Merging now 🎉

…gleCloudPlatform#9457) * Adding IAM support in Terraform for Dataform Repository resource * Adding primary_resource_name property to Example * Adding min-version: beta to IAM policy of repository resource * Update import id used in acceptance tests --------- Co-authored-by: Sarah French <[email protected]>

modular-magician requested a review from SarahFrench November 13, 2023 11:28

modular-magician added the service/dataform label Nov 13, 2023

SarahFrench reviewed Nov 13, 2023

View reviewed changes

modular-magician requested a review from SarahFrench November 13, 2023 12:32

SarahFrench reviewed Nov 13, 2023

View reviewed changes

modular-magician requested a review from SarahFrench November 13, 2023 13:11

SarahFrench mentioned this pull request Nov 13, 2023

Fix google_dataform_repository resource's acceptance tests to have dynamic generated names for secrets #9462

Merged

maverickjoy added 3 commits November 14, 2023 15:49

Adding IAM support in Terraform for Dataform Repository resource

7bcaa6f

Adding primary_resource_name property to Example

32ac43a

Adding min-version: beta to IAM policy of repository resource

d15554e

maverickjoy force-pushed the dev-vikram branch from a67c256 to d15554e Compare November 14, 2023 15:53

SarahFrench reviewed Nov 15, 2023

View reviewed changes

mmv1/products/dataform/Repository.yaml Outdated Show resolved Hide resolved

Update import id used in acceptance tests

1e1bfca

modular-magician requested a review from SarahFrench November 15, 2023 13:10

SarahFrench approved these changes Nov 15, 2023

View reviewed changes

SarahFrench merged commit d7adf2e into GoogleCloudPlatform:main Nov 15, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding IAM support in Terraform for Dataform Repository resource #9457

Adding IAM support in Terraform for Dataform Repository resource #9457

maverickjoy commented Nov 13, 2023 •

edited by SarahFrench

Loading

modular-magician commented Nov 13, 2023

modular-magician commented Nov 13, 2023

SarahFrench left a comment

SarahFrench commented Nov 13, 2023

maverickjoy commented Nov 13, 2023

maverickjoy commented Nov 13, 2023

modular-magician commented Nov 13, 2023

SarahFrench commented Nov 13, 2023

SarahFrench left a comment

maverickjoy commented Nov 13, 2023

modular-magician commented Nov 13, 2023

modular-magician commented Nov 13, 2023

modular-magician commented Nov 13, 2023

SarahFrench commented Nov 13, 2023 •

edited

Loading

maverickjoy commented Nov 14, 2023 •

edited

Loading

SarahFrench commented Nov 14, 2023

modular-magician commented Nov 14, 2023

modular-magician commented Nov 14, 2023

modular-magician commented Nov 14, 2023

maverickjoy commented Nov 15, 2023

modular-magician commented Nov 15, 2023

modular-magician commented Nov 15, 2023

modular-magician commented Nov 15, 2023

SarahFrench left a comment

Adding IAM support in Terraform for Dataform Repository resource #9457

Adding IAM support in Terraform for Dataform Repository resource #9457

Conversation

maverickjoy commented Nov 13, 2023 • edited by SarahFrench Loading

modular-magician commented Nov 13, 2023

modular-magician commented Nov 13, 2023

Diff report

Missing test report

SarahFrench left a comment

Choose a reason for hiding this comment

SarahFrench commented Nov 13, 2023

maverickjoy commented Nov 13, 2023

maverickjoy commented Nov 13, 2023

modular-magician commented Nov 13, 2023

Diff report

Missing test report

SarahFrench commented Nov 13, 2023

SarahFrench left a comment

Choose a reason for hiding this comment

maverickjoy commented Nov 13, 2023

modular-magician commented Nov 13, 2023

Diff report

Missing test report

modular-magician commented Nov 13, 2023

Tests analytics

Action taken

modular-magician commented Nov 13, 2023

SarahFrench commented Nov 13, 2023 • edited Loading

maverickjoy commented Nov 14, 2023 • edited Loading

SarahFrench commented Nov 14, 2023

modular-magician commented Nov 14, 2023

Diff report

Missing test report

modular-magician commented Nov 14, 2023

Tests analytics

Action taken

modular-magician commented Nov 14, 2023

Rerun these tests in REPLAYING mode to catch issues

maverickjoy commented Nov 15, 2023

modular-magician commented Nov 15, 2023

Diff report

Missing test report

modular-magician commented Nov 15, 2023

Tests analytics

Action taken

modular-magician commented Nov 15, 2023

Rerun these tests in REPLAYING mode to catch issues

SarahFrench left a comment

Choose a reason for hiding this comment

maverickjoy commented Nov 13, 2023 •

edited by SarahFrench

Loading

SarahFrench commented Nov 13, 2023 •

edited

Loading

maverickjoy commented Nov 14, 2023 •

edited

Loading