Skip to content
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

Global forwarding rule and forwarding rule DCL conversion. #5138

Merged
merged 8 commits into from
Sep 8, 2021

Conversation

nat-henderson
Copy link
Contributor

Converts the Global forwarding rule and Forwarding Rule resources to the DCL. (Before reviewing, wait for tests to finish - they passed locally, but this is a brownfield resource and I'm opening this PR as a next-step, not as an indicator that this is certainly ready to go in).

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)

(question - I put in no release note because this doesn't matter to end users. Does that seem right to you?)

@google-cla google-cla bot added the cla: yes label Aug 25, 2021
@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 7 files changed, 911 insertions(+), 1409 deletions(-))
Terraform Beta: Diff ( 4 files changed, 15 insertions(+), 12 deletions(-))

@nat-henderson
Copy link
Contributor Author

Ah right, of course, I forgot to add this to beta - forgot that we had to do that. I'll grab that later this afternoon, I suspect.

@nat-henderson nat-henderson force-pushed the global_forwarding_rule branch from e836daa to d13a4e3 Compare August 28, 2021 01:06
@nat-henderson
Copy link
Contributor Author

go run scripts/diff.go --resource=google_compute_global_forwarding_rule --verbose                                                                                    
------------Diffing resource google_compute_global_forwarding_rule------------
DiffSuppressFunc for path project, was:  is now compareSelfLinkOrResourceName
------------Done------------

aaaand

go run scripts/diff.go --resource=google_compute_forwarding_rule --verbose       
------------Diffing resource google_compute_forwarding_rule------------
DiffSuppressFunc for path project, was:  is now compareSelfLinkOrResourceName
------------Done------------

(I assume this diff suppress is okay - it would take a fourth new override to remove it and it's pretty clearly harmless - but let me know if that's not all right).

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 9 files changed, 720 insertions(+), 1360 deletions(-))
Terraform Beta: Diff ( 10 files changed, 705 insertions(+), 1636 deletions(-))

@nat-henderson
Copy link
Contributor Author

Ah, bummer - /gcbrun

@nat-henderson
Copy link
Contributor Author

(in case anyone's wondering, it was the RAM issue in TeamCity again)

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 9 files changed, 720 insertions(+), 1360 deletions(-))
Terraform Beta: Diff ( 9 files changed, 704 insertions(+), 1635 deletions(-))

@nat-henderson
Copy link
Contributor Author

Oh interesting, the way that the test fails in VCR is funny - it fails with "cannot create, already exists", because the existing VCR cassette contains responses that tickle the DCL.

request response DCL MM
1st GET 404 issued to check if resource exists issued to check if resource exists
2nd GET 200 issued at start of Create apply for canonicalization issued after Create is done

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleForwardingRule|TestAccDataSourceGoogleGlobalForwardingRule|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeForwardingRule_forwardingRuleBasicExample|TestAccComputeForwardingRule_forwardingRuleGlobalInternallbExample|TestAccComputeForwardingRule_forwardingRuleExternallbExample|TestAccComputeForwardingRule_forwardingRuleL3DefaultExample|TestAccComputeForwardingRule_forwardingRuleInternallbExample|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeForwardingRule_update|TestAccComputeForwardingRule_networkTier|TestAccComputeForwardingRule_ip|TestAccComputeGlobalForwardingRule_globalForwardingRuleHttpExample|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeGlobalForwardingRule_updateTarget|TestAccComputeGlobalForwardingRule_ipv6|TestAccComputeGlobalForwardingRule_labels|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeManagedSslCertificate_managedSslCertificateBasicExample|TestAccComputePacketMirroring_computePacketMirroringFullExample|TestAccComputeRoute_routeIlbExample|TestAccComputeRouterPeer_basic|TestAccComputeRouterPeer_advertiseMode|TestAccComputeRouterPeer_enable|TestAccComputeRouterInterface_basic|TestAccComputeRouterInterface_withTunnel|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample|TestAccComputeVpnGateway_targetVpnGatewayBasicExample|TestAccComputeVpnTunnel_vpnTunnelBasicExample|TestAccComputeVpnTunnel_vpnTunnelBetaExample|TestAccComputeVpnTunnel_regionFromGateway|TestAccComputeVpnTunnel_router|TestAccComputeVpnTunnel_defaultTrafficSelectors|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=203485

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode: TestAccDataSourceGoogleGlobalForwardingRule|TestAccComputeGlobalForwardingRule_updateTarget|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline Please fix these to complete your PR

@nat-henderson
Copy link
Contributor Author

Cool - two minor issues, will fix.

@nat-henderson
Copy link
Contributor Author

Okay - those tests both pass locally now and this PR is (I believe) ready for review.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 11 files changed, 724 insertions(+), 1362 deletions(-))
Terraform Beta: Diff ( 12 files changed, 709 insertions(+), 1638 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleGlobalForwardingRule|TestAccComputeGlobalForwardingRule_updateTarget|TestAccComputeForwardingRule_update|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=203502

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode: TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample Please fix these to complete your PR

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the VCR test failures although they are on resources that look unrelated (service attachement) are caused by this swap:

Error: Error creating ForwardingRule: googleapi: Error 400: Invalid value for field 'resource.allPorts': 'false'. Invalid field set in Private Service Connect Forwarding Rule. This field should not be set., invalid

@nat-henderson
Copy link
Contributor Author

It looks like the VCR test failures although they are on resources that look unrelated (service attachement) are caused by this swap:

Ah! Of course! Thanks, I will make those tests pass ASAP.

@nat-henderson
Copy link
Contributor Author

Those tests pass on this new change!

--- PASS: TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample (144.2s)

@nat-henderson
Copy link
Contributor Author

Currently running all the regular forwarding rule tests.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 11 files changed, 726 insertions(+), 1362 deletions(-))
Terraform Beta: Diff ( 13 files changed, 713 insertions(+), 1639 deletions(-))

@nat-henderson
Copy link
Contributor Author

Yep, all TestAccComputeGlobalForwardingRule and TestAccComputeForwardingRule passing.

PASS
ok      github.com/hashicorp/terraform-provider-google-beta/google-beta 312.961s

@slevenick slevenick self-requested a review September 1, 2021 23:43
Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good assuming everything passes in CI!

I agree that this shouldn't impact users, so no release note

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleForwardingRule|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExample|TestAccApiGatewayApiConfig_apigatewayApiConfigFullExample|TestAccApiGatewayApi_apigatewayApiBasicExample|TestAccApiGatewayApi_apigatewayApiFullExample|TestAccApiGatewayGateway_apigatewayGatewayBasicExample|TestAccApiGatewayGateway_apigatewayGatewayFullExample|TestAccArtifactRegistryRepository_artifactRegistryRepositoryCmekExample|TestAccArtifactRegistryRepository_artifactRegistryRepositoryBasicExample|TestAccArtifactRegistryRepository_artifactRegistryRepositoryIamExample|TestAccCloudRunService_cloudRunServiceSecretVolumesExample|TestAccCloudRunService_cloudRunServiceSecretEnvironmentVariablesExample|TestAccComputeAutoscaler_autoscalerSingleInstanceExample|TestAccComputeBackendService_backendServiceTrafficDirectorRoundRobinExample|TestAccComputeBackendService_backendServiceTrafficDirectorRingHashExample|TestAccComputeBackendService_backendServiceNetworkEndpointExample|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeForwardingRule_forwardingRuleGlobalInternallbExample|TestAccComputeForwardingRule_forwardingRuleExternallbExample|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeForwardingRule_forwardingRuleBasicExample|TestAccComputeForwardingRule_forwardingRuleL3DefaultExample|TestAccComputeForwardingRule_forwardingRuleInternallbExample|TestAccComputeForwardingRule_update|TestAccComputeForwardingRule_ip|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeForwardingRule_networkTier|TestAccComputeGlobalAddress_globalAddressPrivateServicesConnectExample|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeHealthCheck_healthCheckWithLoggingExample|TestAccComputeMachineImage_machineImageBasicExample|TestAccComputeMachineImage_computeMachineImageKmsExample|TestAccComputeOrganizationSecurityPolicyAssociation_organizationSecurityPolicyAssociationBasicExample|TestAccComputeOrganizationSecurityPolicy_organizationSecurityPolicyBasicExample|TestAccComputePacketMirroring_computePacketMirroringFullExample|TestAccComputeOrganizationSecurityPolicyRule_organizationSecurityPolicyRuleBasicExample|TestAccComputeRegionBackendService_regionBackendServiceCacheExample|TestAccComputeRegionBackendService_regionBackendServiceExternalExample|TestAccComputeRegionHealthCheck_regionHealthCheckHttpLogsExample|TestAccComputeRoute_routeIlbExample|TestAccComputeRouterPeer_basic|TestAccComputeRouterPeer_advertiseMode|TestAccComputeRouterPeer_enable|TestAccComputeRouterInterface_basic|TestAccComputeRouterInterface_withTunnel|TestAccComputeServiceAttachment_serviceAttachmentExplicitProjectsExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExample|TestAccComputeSubnetwork_subnetworkInternalL7lbExample|TestAccComputeTargetInstance_targetInstanceCustomNetworkExample|TestAccComputeVpnGateway_targetVpnGatewayBasicExample|TestAccComputeVpnTunnel_vpnTunnelBasicExample|TestAccComputeVpnTunnel_vpnTunnelBetaExample|TestAccComputeVpnTunnel_regionFromGateway|TestAccComputeVpnTunnel_router|TestAccComputeVpnTunnel_defaultTrafficSelectors|TestAccDataCatalogPolicyTag_dataCatalogTaxonomiesPolicyTagBasicExample|TestAccDataCatalogPolicyTag_dataCatalogTaxonomiesPolicyTagChildPoliciesExample|TestAccDataCatalogTaxonomy_dataCatalogTaxonomyBasicExample|TestAccDataFusionInstance_dataFusionInstanceBasicExample|TestAccDataFusionInstance_dataFusionInstanceFullExample|TestAccDataprocMetastoreService_dataprocMetastoreServiceBasicExample|TestAccDNSManagedZone_dnsManagedZoneServiceDirectoryExample|TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFirebaseProject_firebaseProjectBasicExample|TestAccFirebaseProjectLocation_firebaseProjectLocationBasicExample|TestAccFirebaseWebApp_firebaseWebAppBasicExample|TestAccHealthcareDicomStore_healthcareDicomStoreBqStreamExample|TestAccHealthcareHl7V2Store_healthcareHl7V2StoreParserConfigExample|TestAccHealthcareHl7V2Store_healthcareHl7V2StoreUnschematizedExample|TestAccIAMBetaWorkloadIdentityPool_iamWorkloadIdentityPoolBasicExample|TestAccIAMBetaWorkloadIdentityPool_iamWorkloadIdentityPoolFullExample|TestAccIAMBetaWorkloadIdentityPoolProvider_iamWorkloadIdentityPoolProviderAwsBasicExample|TestAccIAMBetaWorkloadIdentityPoolProvider_iamWorkloadIdentityPoolProviderAwsFullExample|TestAccIAMBetaWorkloadIdentityPoolProvider_iamWorkloadIdentityPoolProviderOidcBasicExample|TestAccIAMBetaWorkloadIdentityPoolProvider_iamWorkloadIdentityPoolProviderOidcFullExample|TestAccOSConfigGuestPolicies_osConfigGuestPoliciesPackagesExample|TestAccOSConfigGuestPolicies_osConfigGuestPoliciesBasicExample|TestAccOSConfigGuestPolicies_osConfigGuestPoliciesRecipesExample|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline|TestAccSecurityScannerScanConfig_scanConfigBasicExample|TestAccServiceDirectoryNamespace_serviceDirectoryNamespaceBasicExample|TestAccServiceDirectoryEndpoint_serviceDirectoryEndpointBasicExample|TestAccServiceDirectoryService_serviceDirectoryServiceBasicExample|TestAccServiceUsageConsumerQuotaOverride_consumerQuotaOverrideExample|TestAccServiceUsageConsumerQuotaOverride_consumerQuotaOverrideZeroValueExample|TestAccServiceUsageConsumerQuotaOverride_regionConsumerQuotaOverrideExample|TestAccVertexAIFeaturestoreEntitytype_vertexAiFeaturestoreEntitytypeExample|TestAccVertexAIFeaturestore_vertexAiFeaturestoreExample|TestAccVPCAccessConnector_vpcAccessConnectorSharedVPCExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=203948

@nat-henderson
Copy link
Contributor Author

Well, that's a lot, but I spot tested them and it looks okay... guess we'll see when the VCR test finishes.

@slevenick
Copy link
Contributor

Well, that's a lot, but I spot tested them and it looks okay... guess we'll see when the VCR test finishes.

Lots of failures on long names & port comparisons.. Did anything in the DCL change around this with the new version?

@slevenick slevenick self-requested a review September 2, 2021 16:33
@nat-henderson
Copy link
Contributor Author

Bummer - yes, the DCL can canonicalize those strings just fine, but import tests don't provide us with the customer's value, so we return whatever the API returns.

Okay, I'll have to come back to this when I'm back from vacation.

@nat-henderson
Copy link
Contributor Author

Okay - seems to have been a simple git history mistake on my part, covered up by the fact that I was only running make tpgtools on my machine, so my local testing was using the pre-mistake version of mmv1.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 11 files changed, 725 insertions(+), 1363 deletions(-))
Terraform Beta: Diff ( 12 files changed, 714 insertions(+), 1643 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeForwardingRule_forwardingRuleExternallbExample|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeOrganizationSecurityPolicyAssociation_organizationSecurityPolicyAssociationBasicExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExampleUpdate|TestAccFilestoreInstance_filestoreInstanceFullExample|TestAccFirebaseWebApp_firebaseWebAppBasicExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=204730

@modular-magician
Copy link
Collaborator

Tests failed during RECORDING mode: TestAccFirebaseWebApp_firebaseWebAppBasicExample|TestAccComputeOrganizationSecurityPolicyAssociation_organizationSecurityPolicyAssociationBasicExample|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample Please fix these to complete your PR

@nat-henderson
Copy link
Contributor Author

/gcbrun - failure in cloud build to cancel the ongoing run since it was already done.

@modular-magician
Copy link
Collaborator

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

Diff report:

Terraform GA: Diff ( 11 files changed, 725 insertions(+), 1363 deletions(-))
Terraform Beta: Diff ( 12 files changed, 715 insertions(+), 1644 deletions(-))

@modular-magician
Copy link
Collaborator

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

@nat-henderson
Copy link
Contributor Author

Okay - that is all the tests passing. The two that failed are unrelated and are failing at head. I'm going to merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants