Fix compute instance iam asset name by iyabchen · Pull Request #6390 · GoogleCloudPlatform/magic-modules

iyabchen · 2022-08-08T16:31:30Z

Fix compute instance iam asset name

Asset name was expecting instance, but compute instance iam uses instance_name as attribute name.

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

modular-magician · 2022-08-08T16:32:31Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @rileykarson, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

modular-magician · 2022-08-08T16:43:38Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 13 files changed, 1816 insertions(+), 18 deletions(-))

modular-magician · 2022-08-08T17:17:14Z

Tests analytics

Total tests: 2125
Passed tests 1890
Skipped tests: 227
Failed tests: 8

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccComputeInstance_networkPerformanceConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCloudFunctions2Function_fullUpdate|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample

modular-magician · 2022-08-08T17:24:59Z

Tests passed during RECORDING mode:
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view]
TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample[view]
TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample[view]
TestAccCloudFunctions2Function_fullUpdate[view]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[view]
TestAccComputeInstance_networkPerformanceConfig[view]
TestAccComputeGlobalForwardingRule_internalLoadBalancing[view]
TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view]

Please fix these to complete your PR
View the build log or the debug log for each test

melinath

This case should be handled by the resource_converter_iam template; ideally we'd be able to fix the bug instead of making this handwritten. The resource converter template has access to instance_name being the parent_resource_attribute - this seems like maybe a bug in how the asset_name_template is built when there isn't an explicit import_format.

magic-modules/mmv1/templates/validator/resource_converter_iam.go.erb

Line 28 in 0f8b95f

if import_format.nil? || import_format.empty?

iyabchen · 2022-08-08T20:29:29Z

This case should be handled by the resource_converter_iam template; ideally we'd be able to fix the bug instead of making this handwritten. The resource converter template has access to instance_name being the parent_resource_attribute - this seems like maybe a bug in how the asset_name_template is built when there isn't an explicit import_format.

magic-modules/mmv1/templates/validator/resource_converter_iam.go.erb

Line 28 in 0f8b95f

if import_format.nil? || import_format.empty?

Should we add an import_format for that resource, or add a logic to replace the {{instance}} as well in the else?

  import_format = object.iam_policy.import_format || object.import_format
  if import_format.nil? || import_format.empty?
    ...
  else
    asset_name_template = '//' + product_backend_name.downcase + '.googleapis.com/' + import_format[0].gsub('{{name}}', "{{#{parent_resource_name}}}").gsub('{{instance}}', "{{#{parent_resource_name}}}").gsub('%', '')
  end

melinath · 2022-08-08T20:37:23Z

@iyabchen we shouldn't need to add an import format and we shouldn't need to make a special case - we should just need to change the first case (import format being nil) to correctly reference parent_resource_name when building the asset name template, which will probably look similar to what currently happens in the second case.

modular-magician · 2022-08-09T07:40:24Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 15 files changed, 1752 insertions(+), 8 deletions(-))

modular-magician · 2022-08-09T08:24:17Z

Tests analytics

Total tests: 2128
Passed tests 1891
Skipped tests: 227
Failed tests: 10

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccComputeInstance_networkPerformanceConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudFunctions2Function_fullUpdate|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccDataSourceGoogleServiceAccountIdToken_impersonation

modular-magician · 2022-08-09T08:31:41Z

Tests passed during RECORDING mode:
TestAccCloudFunctions2Function_fullUpdate[view]
TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample[view]
TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample[view]
TestAccCGCSnippet_eventarcWorkflowsExample[view]
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view]
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[view]

Tests failed during RECORDING mode:
TestAccComputeInstance_networkPerformanceConfig[view]
TestAccComputeInstance_soleTenantNodeAffinities[view]
TestAccComputeGlobalForwardingRule_internalLoadBalancing[view]
TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view]

Please fix these to complete your PR
View the build log or the debug log for each test

melinath

💯 this is looking good - it fixes a few other iam resources as well! I think it might be possible to simplify the logic a bit? LGTM otherwise.

melinath · 2022-08-09T17:04:49Z

  import_format = object.iam_policy.import_format || object.import_format
  if import_format.nil? || import_format.empty?
-    asset_name_template = '//' + product_backend_name.downcase + '.googleapis.com/' + (object.cai_base_url || object.self_link || object.base_url) + '/{{' + object.name.downcase + '}}'
+    asset_name_template = '//' + product_backend_name.downcase + '.googleapis.com/' + (object.cai_base_url || object.self_link || object.base_url) + '/{{' + asset_resource_name + '}}'


Suggested change

asset_name_template = '//' + product_backend_name.downcase + '.googleapis.com/' + (object.cai_base_url || object.self_link || object.base_url) + '/{{' + asset_resource_name + '}}'

asset_name_template = '//' + product_backend_name.downcase + '.googleapis.com/' + (object.cai_base_url || object.self_link || object.base_url) + '/{{' + parent_resource_name + '}}'

Done. parent_resource_name alone works, so no need to || object.name.

modular-magician · 2022-08-09T19:09:10Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

TF Validator: Diff ( 15 files changed, 1752 insertions(+), 8 deletions(-))

modular-magician · 2022-08-09T19:45:42Z

Tests analytics

Total tests: 2128
Passed tests 1891
Skipped tests: 227
Failed tests: 10

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeInstance_networkPerformanceConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample|TestAccCloudFunctions2Function_fullUpdate|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample|TestAccCGCSnippet_eventarcWorkflowsExample

modular-magician · 2022-08-09T19:53:48Z

Tests passed during RECORDING mode:
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view]
TestAccFirebaserulesRelease_BasicRelease[view]
TestAccCloudfunctions2function_cloudfunctions2BasicGcsExample[view]
TestAccCloudFunctions2Function_fullUpdate[view]
TestAccCloudfunctions2function_cloudfunctions2BasicAuditlogsExample[view]
TestAccCGCSnippet_eventarcWorkflowsExample[view]

Tests failed during RECORDING mode:
TestAccComputeGlobalForwardingRule_internalLoadBalancing[view]
TestAccComputeInstance_networkPerformanceConfig[view]
TestAccComputeInstance_soleTenantNodeAffinities[view]
TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view]

Please fix these to complete your PR
View the build log or the debug log for each test

* Fix compute instance iam asset name * Use parent resource name for asset name if not empty * fix iam policy test case * revert a new file * use parent_resource_name

Fix compute instance iam asset name

dc1f4b0

modular-magician requested a review from rileykarson August 8, 2022 16:32

rileykarson requested review from a team and melinath and removed request for a team and rileykarson August 8, 2022 16:34

melinath reviewed Aug 8, 2022

View reviewed changes

iyabchen added 2 commits August 8, 2022 17:37

Use parent resource name for asset name if not empty

962968c

fix iam policy test case

4c057b1

modular-magician requested a review from melinath August 9, 2022 07:26

revert a new file

7d8ced0

iyabchen marked this pull request as ready for review August 9, 2022 14:04

melinath requested changes Aug 9, 2022

View reviewed changes

use parent_resource_name

6aeabce

modular-magician requested a review from melinath August 9, 2022 18:57

melinath approved these changes Aug 9, 2022

View reviewed changes

melinath merged commit bf766eb into GoogleCloudPlatform:main Aug 9, 2022

modular-magician mentioned this pull request Aug 9, 2022

Fix compute instance iam asset name GoogleCloudPlatform/terraform-validator#905

Merged

5 tasks

	asset_name_template = '//' + product_backend_name.downcase + '.googleapis.com/' + (object.cai_base_url \|\| object.self_link \|\| object.base_url) + '/{{' + asset_resource_name + '}}'
	asset_name_template = '//' + product_backend_name.downcase + '.googleapis.com/' + (object.cai_base_url \|\| object.self_link \|\| object.base_url) + '/{{' + parent_resource_name + '}}'

Conversation

iyabchen commented Aug 8, 2022

Uh oh!

modular-magician commented Aug 8, 2022

Uh oh!

modular-magician commented Aug 8, 2022

Diff report:

Uh oh!

modular-magician commented Aug 8, 2022

Tests analytics

Action taken

Uh oh!

modular-magician commented Aug 8, 2022

Uh oh!

melinath left a comment

Choose a reason for hiding this comment

Uh oh!

iyabchen commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melinath commented Aug 8, 2022

Uh oh!

modular-magician commented Aug 9, 2022

Diff report:

Uh oh!

modular-magician commented Aug 9, 2022

Tests analytics

Action taken

Uh oh!

modular-magician commented Aug 9, 2022

Uh oh!

melinath left a comment

Choose a reason for hiding this comment

Uh oh!

melinath Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

iyabchen Aug 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

modular-magician commented Aug 9, 2022

Diff report:

Uh oh!

modular-magician commented Aug 9, 2022

Tests analytics

Action taken

Uh oh!

modular-magician commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iyabchen commented Aug 8, 2022 •

edited

Loading