Added support for approval when build is run #5837

abhinavrau · 2022-03-19T02:34:31Z

Fixes hashicorp/terraform-provider-google#9880

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)

cloudbuild: Add approval config

modular-magician · 2022-03-19T02:35:31Z

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

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@shuyama1, please review this PR or find an appropriate assignee.

wyardley · 2022-03-19T02:43:15Z

Woo! Much appreciated @abhinavrau

modular-magician · 2022-03-19T02:49:04Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 99 insertions(+))
Terraform Beta: Diff ( 3 files changed, 99 insertions(+))
TF Validator: Diff ( 3 files changed, 32 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 6 insertions(+))

modular-magician · 2022-03-19T04:54:25Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceSqlDatabaseInstance_basic|TestAccApikeysKey_AndroidKey|TestAccApikeysKey_BasicKey|TestAccApikeysKey_IosKey|TestAccApikeysKey_MinimalKey|TestAccApikeysKey_ServerKey|TestAccArtifactRegistryRepository_create_mvn_snapshot|TestAccArtifactRegistryRepository_create_mvn_release|TestAccBigqueryReservationAssignment_BasicHandWritten|TestAccCGCSnippet_sqlMysqlInstanceBackupExample|TestAccCGCSnippet_sqlPostgresInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupExample|TestAccCGCSnippet_sqlMysqlInstanceAuthorizedNetworkExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupRetentionExample|TestAccCloudBuildTrigger_cloudbuildTriggerPubsubConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerWebhookConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerManualExample|TestAccCloudBuildTrigger_cloudbuildTriggerFilenameExample|TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample|TestAccCloudBuildTrigger_available_secrets_config|TestAccCloudBuildTrigger_basic|TestAccCloudBuildTrigger_cloudbuildTriggerServiceAccountExample|TestAccCloudBuildTrigger_disable|TestAccCloudBuildTrigger_fullStep|TestAccCloudFunctionsFunction_secretEnvVar|TestAccComputeForwardingRule_update|TestAccContainerAwsCluster_BasicHandWritten|TestAccContainerAwsNodePool_BasicHandWritten|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccContainerNodePool_gvnic|TestAccDataprocCluster_nonPreemptibleSecondary|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withConfigOverrides|TestAccFirebaserulesRelease_BasicRelease|TestAccFirebaserulesRelease_MinimalRelease|TestAccFirebaserulesRuleset_BasicRuleset|TestAccFirebaserulesRuleset_MinimalRuleset|TestAccLoggingLogView_BasicHandWritten|TestAccMonitoringUptimeCheckConfig_uptimeCheckConfigHttpExample|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheOrigin_updateAndImport|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccNetworkServicesEdgeCacheService_updateAndImport|TestAccOSConfigPatchDeployment_osConfigPatchDeploymentDailyMidnightExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=270099

modular-magician · 2022-03-19T05:33:30Z

Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR

abhinavrau · 2022-03-20T15:45:14Z

Not sure why this test failed since my PR does not have anything to do with what this test is testing

wyardley · 2022-03-21T22:03:33Z

@abhinavrau best I can see, it's failing for other PRs, so maybe expected? Also, I think it looks like a maintainer may need to run /gcbrun or something to trigger additional cloudbuild tests?

shuyama1 · 2022-03-21T22:09:34Z

Thanks @abhinavrau for making the changes and thanks @wyardley for looking into the issue. Test failed seem not related to this PR. I am working on reviewing the PR and testing it locally.

shuyama1

Tested the field locally and it seemed like setting approval_required to false will fail the test

shuyama1 · 2022-03-21T23:17:30Z

The problems are

The value is not got sent if approval_config.approval_required is set to false
This can be solve by adding send_empty_value to the approval_required field to force sending to value in the request body.
The API does return the value when it is set to false, which would then cause a perma-diff after we force sending the value.
This could be solved by adding a custom flattener for approval_config

…proval_config with allow_emptyy_object and approval_required field to have a default value of false

abhinavrau · 2022-03-24T23:08:04Z

Thanks @shuyama1 for finding the issue. When approval_config.approval_required is set to false and then an terraform apply followed by a terraform plan is run, it shows changes which is failing the test.

The problems are

The value is not got sent if approval_config.approval_required is set to false
This can be solve by adding send_empty_value to the approval_required field to force sending to value in the request body.

The above is not the case since the API does not expect it to be sent when false

The API does return the value when it is set to false, which would then cause a perma-diff after we force sending the value.
This could be solved by adding a custom flattener for approval_config

The above is the problem when this flag is set to false. I added a flatten_object: true in the terraform.yaml file with a custom flattener as you suggested. I based my flattener on this one which should work. But I am facing the following issue when I run the TestAccCloudBuildTrigger_cloudbuildTriggerManualExample test.


Error: Plugin did not respond

  with google_cloudbuild_trigger.manual-trigger,
  on terraform_plugin_test.tf line 3, in resource "google_cloudbuild_trigger" "manual-trigger":
   3: resource "google_cloudbuild_trigger" "manual-trigger" {

The plugin encountered an error, and failed to respond to the
plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain
more details.
    provider_test.go:301: Step 1/3 error: Error running apply: exit status 1
        
        Error: Plugin did not respond
        
          with google_cloudbuild_trigger.manual-trigger,
          on terraform_plugin_test.tf line 3, in resource "google_cloudbuild_trigger" "manual-trigger":
           3: resource "google_cloudbuild_trigger" "manual-trigger" {
        
        The plugin encountered an error, and failed to respond to the
        plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain
        more details.

The output does not tell me much of what went wrong. I tried adding log.Printf statements to the flatterner but it does not print them out. I know it gets to the flattener code since removing the code in the flattener with just the log statements provide some output. Any ideas on what could be wrong?

shuyama1 · 2022-03-24T23:48:36Z

Thanks @shuyama1 for finding the issue. When approval_config.approval_required is set to false and then an terraform apply followed by a terraform plan is run, it shows changes which is failing the test.

The problems are

The value is not got sent if approval_config.approval_required is set to false
This can be solve by adding send_empty_value to the approval_required field to force sending to value in the request body.

The above is not the case since the API does not expect it to be sent when false

The API does return the value when it is set to false, which would then cause a perma-diff after we force sending the value.
This could be solved by adding a custom flattener for approval_config

The above is the problem when this flag is set to false. I added a flatten_object: true in the terraform.yaml file with a custom flattener as you suggested. I based my flattener on this one which should work. But I am facing the following issue when I run the TestAccCloudBuildTrigger_cloudbuildTriggerManualExample test.
Error: Plugin did not respond

  with google_cloudbuild_trigger.manual-trigger,
  on terraform_plugin_test.tf line 3, in resource "google_cloudbuild_trigger" "manual-trigger":
   3: resource "google_cloudbuild_trigger" "manual-trigger" {

The plugin encountered an error, and failed to respond to the
plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain
more details.
    provider_test.go:301: Step 1/3 error: Error running apply: exit status 1
        
        Error: Plugin did not respond
        
          with google_cloudbuild_trigger.manual-trigger,
          on terraform_plugin_test.tf line 3, in resource "google_cloudbuild_trigger" "manual-trigger":
           3: resource "google_cloudbuild_trigger" "manual-trigger" {
        
        The plugin encountered an error, and failed to respond to the
        plugin.(*GRPCProvider).ApplyResourceChange call. The plugin logs may contain
        more details.
The output does not tell me much of what went wrong. I tried adding log.Printf statements to the flatterner but it does not print them out. I know it gets to the flattener code since removing the code in the flattener with just the log statements provide some output. Any ideas on what could be wrong?

Would you mind committing your code?

…ide custom flattener for field

abhinavrau · 2022-03-25T02:23:01Z

Never mind. I debugged the test and it turns out I had to use the fieldname as specified in the terraform.yaml to the flattened field name. It is working now. I have updated the PR

modular-magician · 2022-03-25T02:35:39Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 89 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 89 insertions(+), 3 deletions(-))
TF Validator: Diff ( 3 files changed, 25 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 6 insertions(+))

mmv1/templates/terraform/custom_flatten/cloudbuild_approval_required.go.erb

modular-magician · 2022-03-25T04:48:27Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=272764

modular-magician · 2022-03-25T16:16:19Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 89 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 89 insertions(+), 3 deletions(-))
TF Validator: Diff ( 3 files changed, 25 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 6 insertions(+))

modular-magician · 2022-03-25T18:25:17Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccFirebaserulesRelease_BasicRelease|TestAccServiceNetworkingPeeredDNSDomain_basic You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=272823

modular-magician · 2022-03-25T19:27:11Z

Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR

abhinavrau · 2022-03-25T21:09:42Z

These failures are not related to this PR in any way. Could you please review the results to see if this PR looks good? Thanks

…

On Fri, Mar 25, 2022 at 3:27 PM The Magician ***@***.***> wrote: Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR — Reply to this email directly, view it on GitHub <#5837 (comment)>, or unsubscribe </~https://github.com/notifications/unsubscribe-auth/ABAPBA7V5GOASAHEIW7JISTVBYHRTANCNFSM5RDLKGXQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

shuyama1 · 2022-03-29T14:19:45Z

@abhinavrau Thanks for making the update and sorry for the delay on reviewing it! Got a question tho: any reason that we want to flatten the nested object here? I think we'll want to preserve the structure of the resource as it was, as we try to limit the use of changing the schema of resources.
Do you think the following plan would make sense in this case?
Instead of flatten approval_config. approval_required to the top, we can keep the original nested structure (remove the flatten_object: true but keep the custom flatten), add default_from_api: true to approval_config in terraform.yaml - it will mark approval_config as optional + computed, which means that not setting it in the config won't show a perma-diff in the terraform.

abhinavrau · 2022-03-29T20:37:01Z

Thanks @shuyama1 for reviewing and the suggestion. Let me try it out. I made the change. The PR is updated

… API

mmv1/products/cloudbuild/terraform.yaml

modular-magician · 2022-03-29T21:06:22Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 110 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 110 insertions(+), 3 deletions(-))
TF Validator: Diff ( 3 files changed, 32 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 9 insertions(+))

abhinavrau · 2022-03-30T13:26:14Z

Thanks again @shuyama1 for pointing this out. I have removed the override. Hopefully I didn't miss anything else.

modular-magician · 2022-03-30T13:38:53Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 110 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 110 insertions(+), 3 deletions(-))
TF Validator: Diff ( 3 files changed, 32 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 9 insertions(+))

mmv1/templates/terraform/custom_flatten/cloudbuild_approval_required.go.erb

modular-magician · 2022-03-30T15:44:46Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataprocMetastoreServiceDatasource_basic|TestAccCloudBuildTrigger_basic|TestAccCloudBuildTrigger_available_secrets_config|TestAccCloudBuildTrigger_pubsub_config|TestAccCloudBuildTrigger_webhook_config|TestAccCloudBuildTrigger_disable|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccFirebaserulesRelease_BasicRelease|TestAccSpannerDatabase_postgres You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=274376

modular-magician · 2022-03-30T16:11:10Z

Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR

modular-magician · 2022-03-30T16:24:42Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 110 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 3 files changed, 110 insertions(+), 3 deletions(-))
TF Validator: Diff ( 3 files changed, 32 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 1 file changed, 9 insertions(+))

modular-magician · 2022-03-30T18:34:20Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccFirebaserulesRelease_BasicRelease You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=274383

modular-magician · 2022-03-30T19:35:39Z

Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR

shuyama1

LGTM! Thank you so much!

wyardley · 2022-03-30T21:22:25Z

Thanks @abhinavrau @shuyama1! This has been on my want list for a while! Looking forward to seeing this in an upcoming provider release.

Added support for approval when build is run

7d83464

modular-magician requested a review from shuyama1 March 19, 2022 02:35

abhinavrau mentioned this pull request Mar 19, 2022

Add support for manual builds, git source for webhook/pubsub triggered builds and filter field #5738

Merged

5 tasks

shuyama1 requested changes Mar 21, 2022

View reviewed changes

Fix test failure when approval_required is set to false by marking ap…

59e6a64

…proval_config with allow_emptyy_object and approval_required field to have a default value of false

Fix for when approval_required=false. Flatten approvalConfig and prov…

eb72ca2

…ide custom flattener for field

wyardley reviewed Mar 25, 2022

View reviewed changes

mmv1/templates/terraform/custom_flatten/cloudbuild_approval_required.go.erb Outdated Show resolved Hide resolved

add newline

a53d46e

Removed flatten_object: true and add default_from_api: to reflect the…

e26a77b

… API

shuyama1 requested changes Mar 29, 2022

View reviewed changes

mmv1/products/cloudbuild/terraform.yaml Outdated Show resolved Hide resolved

Remove uneeded override

7ffdc1b

wyardley reviewed Mar 30, 2022

View reviewed changes

mmv1/templates/terraform/custom_flatten/cloudbuild_approval_required.go.erb Outdated Show resolved Hide resolved

Updated copyright year

76217f8

shuyama1 approved these changes Mar 30, 2022

View reviewed changes

shuyama1 merged commit 82e38a6 into GoogleCloudPlatform:main Mar 30, 2022

modular-magician mentioned this pull request Mar 30, 2022

Added support for approval when build is run terraform-google-modules/docs-examples#204

Merged

5 tasks

hedlund mentioned this pull request Mar 31, 2022

Adding support for "approval config" in Cloud Build triggers hashicorp/terraform-provider-google#11377

Closed

betsy-lichtenberg pushed a commit to betsy-lichtenberg/magic-modules that referenced this pull request Apr 25, 2022

Added support for approval when build is run (GoogleCloudPlatform#5837)

7e4e3ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for approval when build is run #5837

Added support for approval when build is run #5837

abhinavrau commented Mar 19, 2022

modular-magician commented Mar 19, 2022

wyardley commented Mar 19, 2022

modular-magician commented Mar 19, 2022

modular-magician commented Mar 19, 2022

modular-magician commented Mar 19, 2022

abhinavrau commented Mar 20, 2022

wyardley commented Mar 21, 2022

shuyama1 commented Mar 21, 2022

shuyama1 left a comment

shuyama1 commented Mar 21, 2022

abhinavrau commented Mar 24, 2022 •

edited

Loading

shuyama1 commented Mar 24, 2022

abhinavrau commented Mar 25, 2022 •

edited

Loading

modular-magician commented Mar 25, 2022

modular-magician commented Mar 25, 2022

modular-magician commented Mar 25, 2022

modular-magician commented Mar 25, 2022

modular-magician commented Mar 25, 2022

abhinavrau commented Mar 25, 2022 via email

shuyama1 commented Mar 29, 2022

abhinavrau commented Mar 29, 2022 •

edited

Loading

modular-magician commented Mar 29, 2022

abhinavrau commented Mar 30, 2022

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

shuyama1 left a comment

wyardley commented Mar 30, 2022

Added support for approval when build is run #5837

Added support for approval when build is run #5837

Conversation

abhinavrau commented Mar 19, 2022

modular-magician commented Mar 19, 2022

wyardley commented Mar 19, 2022

modular-magician commented Mar 19, 2022

Diff report:

modular-magician commented Mar 19, 2022

modular-magician commented Mar 19, 2022

abhinavrau commented Mar 20, 2022

wyardley commented Mar 21, 2022

shuyama1 commented Mar 21, 2022

shuyama1 left a comment

Choose a reason for hiding this comment

shuyama1 commented Mar 21, 2022

abhinavrau commented Mar 24, 2022 • edited Loading

shuyama1 commented Mar 24, 2022

abhinavrau commented Mar 25, 2022 • edited Loading

modular-magician commented Mar 25, 2022

Diff report:

modular-magician commented Mar 25, 2022

modular-magician commented Mar 25, 2022

Diff report:

modular-magician commented Mar 25, 2022

modular-magician commented Mar 25, 2022

abhinavrau commented Mar 25, 2022 via email

shuyama1 commented Mar 29, 2022

abhinavrau commented Mar 29, 2022 • edited Loading

modular-magician commented Mar 29, 2022

Diff report:

abhinavrau commented Mar 30, 2022

modular-magician commented Mar 30, 2022

Diff report:

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

Diff report:

modular-magician commented Mar 30, 2022

modular-magician commented Mar 30, 2022

shuyama1 left a comment

Choose a reason for hiding this comment

wyardley commented Mar 30, 2022

abhinavrau commented Mar 24, 2022 •

edited

Loading

abhinavrau commented Mar 25, 2022 •

edited

Loading

abhinavrau commented Mar 29, 2022 •

edited

Loading