-
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
add support for Cloud Armor Edge Policies #5794
add support for Cloud Armor Edge Policies #5794
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 241 insertions(+), 5 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccDataSourceSqlDatabaseInstance_basic|TestAccBigqueryReservationAssignment_BasicHandWritten|TestAccCGCSnippet_sqlSqlserverInstanceAuthorizedNetworkExample|TestAccCloudBuildTrigger_cloudbuildTriggerPubsubConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerWebhookConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerManualExample|TestAccCloudBuildTrigger_cloudbuildTriggerFilenameExample|TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample|TestAccCloudBuildTrigger_basic|TestAccCloudBuildTrigger_available_secrets_config|TestAccCloudBuildTrigger_cloudbuildTriggerServiceAccountExample|TestAccCloudBuildTrigger_disable|TestAccCloudBuildTrigger_fullStep|TestAccComputeBackendBucket_backendBucketSecurityPolicyExample|TestAccComputeBackendBucket_withSecurityPolicy|TestAccComputeSecurityPolicy_basic|TestAccComputeSecurityPolicy_update|TestAccContainerCluster_withAuthenticatorGroupsConfig You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=264834 |
Tests failed during RECORDING mode: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccComputeBackendBucket_withSecurityPolicy|TestAccComputeBackendBucket_backendBucketSecurityPolicyExample|TestAccCGCSnippet_sqlSqlserverInstanceAuthorizedNetworkExample|TestAccComputeSecurityPolicy_update|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccDataSourceSqlDatabaseInstance_basic Please fix these to complete your PR |
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.
Looks like tests are failing?
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 239 insertions(+), 5 deletions(-)) |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 239 insertions(+), 5 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccDataSourceSqlDatabaseInstance_basic|TestAccApikeysKey_AndroidKey|TestAccApikeysKey_BasicKey|TestAccApikeysKey_IosKey|TestAccApikeysKey_MinimalKey|TestAccApikeysKey_ServerKey|TestAccArtifactRegistryRepository_create_mvn_snapshot|TestAccArtifactRegistryRepository_create_mvn_release|TestAccCGCSnippet_sqlMysqlInstanceBackupExample|TestAccCGCSnippet_sqlPostgresInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccCloudFunctionsFunction_secretEnvVar|TestAccComputeBackendBucket_backendBucketSecurityPolicyExample|TestAccComputeBackendBucket_withSecurityPolicy|TestAccContainerAwsCluster_BasicHandWritten|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccContainerNodePool_gvnic|TestAccDataprocCluster_nonPreemptibleSecondary|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withConfigOverrides You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=267025 |
Everything should be kosher here @slevenick ^-^ |
@@ -0,0 +1,19 @@ | |||
// security_policy isn't set by Create / Update |
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.
This exists in the post_create, but what if the user goes from this field being unset on a bucket to setting it? Should the field be ForceNew or do we also need this in the Update call?
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.
Oh this is used as post_update as well, makes sense
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.
Can the update part of this be done by setting update_url
on this specific field in the terraform.yaml? It allows calling a specific endpoint for a specific field
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.
I think this would complicate things further by having two separate pathways to do the same thing. This way unifies the codepaths. I would be against such a change.
mmv1/third_party/terraform/tests/resource_compute_security_policy_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_compute_backend_bucket_test.go
Outdated
Show resolved
Hide resolved
|
||
spr := emptySecurityPolicyReference() | ||
spr.SecurityPolicy = pol.RelativeLink() | ||
op, err := config.NewComputeClient(userAgent).BackendBuckets.SetEdgeSecurityPolicy(project, obj["name"].(string), spr).Do() |
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.
Can we do this through the HTTP client directly? It feels a little awkward to add a dependency on the client libraries to a mmv1-based resource
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.
This is already a pattern in another resource https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/templates/terraform/post_create/compute_backend_service_security_policy.go.erb#L8-L9
There is where I adapted the code from
fixed spacing, awaiting response for other suggestions |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 7 files changed, 239 insertions(+), 5 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceSqlDatabaseInstance_basic|TestAccCGCSnippet_sqlMysqlInstanceBackupExample|TestAccCGCSnippet_sqlMysqlInstanceAuthorizedNetworkExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupRetentionExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccLoggingLogView_BasicHandWritten|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheOrigin_updateAndImport|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccNetworkServicesEdgeCacheService_updateAndImport|TestAccOSConfigPatchDeployment_osConfigPatchDeploymentDailyMidnightExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=269575 |
closes hashicorp/terraform-provider-google#10761
Release Note Template for Downstream PRs (will be copied)