-
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
Compute serviceattachment dcl #5444
Compute serviceattachment dcl #5444
Conversation
/gcbrun |
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.
Thanks for making these changes @trodge!
I have some first-pass comments, inline- the documentation (inline) and a few remaining schema changes are the big things:
- google_compute_service_attachment.consumer_accept_lists.connection_limit went from required -> optional
- google_compute_service_attachment.enable_proxy_protocol went from required -> optional
- google_compute_service_attachment.service_attachment_id does not appear to be returned by the API, so I'm unsure why it is supported by the DCL
Once those are resolved there's a couple pieces of tooling I'd like to run- I want to try out the release diff tester (ack that Scott already did, it's mostly for my own learning) and the schema diff tool.
@@ -3,7 +3,7 @@ module github.com/hashicorp/terraform-provider-google<%= "-" + version unless ve | |||
|
|||
require ( | |||
cloud.google.com/go/bigtable v1.10.1 | |||
github.com/GoogleCloudPlatform/declarative-resource-client-library v0.0.0-20211027225138-ef28ca390518 |
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.
@ndmckinley can you verify that the newly-available fields in CloudBuild WorkerPool are intentional? It seems so, but I was surprised by the etag.
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.
Are you referring to the fields that were added to beta in cl/408655015 ?
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.
Yeah - #5356 will add those to GA. They work in beta as of now! Would you like etag exluded? I don't mind either way, I forget what we decided on fingerprints. Could you comment on that PR with that?
@@ -25,7 +25,7 @@ func TestAccComputeServiceAttachment_serviceAttachmentBasicExampleUpdate(t *test | |||
ResourceName: "google_compute_service_attachment.psc_ilb_service_attachment", | |||
ImportState: true, | |||
ImportStateVerify: true, | |||
ImportStateVerifyIgnore: []string{"target_service", "region"}, | |||
ImportStateVerifyIgnore: []string{"target_service", "region", "nat_subnets.0"}, |
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.
Why does this newly need to be set? Generally, having to set an ISVI value indicates that something's changed that will affect end users.
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.
Without this the test still fails with:
provider_test.go:278: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
(map[string]string) (len=1) {
(string) (len=13) "nat_subnets.0": (string) (len=121) "https://www.googleapis.com/compute/beta/projects/tjr-dm-test-1/regions/us-west2/subnetworks/tf-test-psc-ilb-natn86tr1bw0e"
}
(map[string]string) (len=1) {
(string) (len=13) "nat_subnets.0": (string) (len=81) "projects/tjr-dm-test-1/regions/us-west2/subnetworks/tf-test-psc-ilb-natn86tr1bw0e"
}
nat_subnets
Elem.DiffSuppressFunc
is compareSelfLinkOrResourceName
, so I'm fairly confident this is a problem that only occurs in testing.
@@ -0,0 +1,8 @@ | |||
- type: EXCLUDE | |||
field: region |
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.
Ah, weird- the base DCL resource includes a (presumably output-only) region
field but also a location
field which maps to the region
field in the API?
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.
Yes, this is the way KCC requested we name the fields. location
corresponds to the URL parameter.
@@ -3,7 +3,7 @@ module github.com/hashicorp/terraform-provider-google<%= "-" + version unless ve | |||
|
|||
require ( | |||
cloud.google.com/go/bigtable v1.10.1 | |||
github.com/GoogleCloudPlatform/declarative-resource-client-library v0.0.0-20211027225138-ef28ca390518 | |||
github.com/GoogleCloudPlatform/declarative-resource-client-library v0.0.0-20211111235242-eee3ce55868a |
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.
Note, we'll want to handwrite documentation in the DCL and add reference links such as was done in #5325. That'll put the generated docs are at parity with the original ones. See the current diff:
Not sure on the process- you may have to look at @ndmckinley's history, and/or pester them to document it if it isn't intuitive.
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.
cl/409490555 should add the documentation to the DCL schema that the template needs to generate the page as it already exists.
I have verified that the two fields which went from required to optional are in fact optional. I don't want to make them required in the DCL because this resource is already in KCC with the optional fields. I meant to exclude |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccAssuredWorkloadsWorkload_BasicHandWritten|TestAccAssuredWorkloadsWorkload_FullHandWritten|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeRegionHealthCheck_tcp_update|TestAccComputeRegionHealthCheck_typeTransition|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeServiceAttachment_BasicHandWritten|TestAccContainerCluster_withILBSubsetting|TestAccContainerCluster_regionalWithNodePool|TestAccContainerCluster_withNodePoolBasic|TestAccContainerCluster_withNodePoolResize|TestAccContainerCluster_withNodePoolUpdateVersion|TestAccContainerCluster_withNodePoolMultiple|TestAccContainerCluster_withNodePoolAutoscaling|TestAccContainerCluster_withNodePoolNodeConfig|TestAccContainerCluster_withEnableKubernetesAlpha|TestAccContainerNodePool_basic|TestAccContainerNodePool_basicWithClusterId|TestAccContainerNodePool_nodeLocations|TestAccContainerNodePool_maxPodsPerNode|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_autoscaling|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_resize|TestAccContainerNodePool_regionalClusters|TestAccContainerNodePool_version|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccContainerNodePool_ephemeralStorageConfig|TestAccDataprocWorkflowTemplate_basic|TestAccEventarcTrigger_BasicHandWritten|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccMonitoringMonitoredProject_BasicMonitoredProject|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginBasicExample|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccOrgPolicyPolicy_EnforcePolicy|TestAccOrgPolicyPolicy_FolderPolicy|TestAccOrgPolicyPolicy_ProjectPolicy|TestAccPrivatecaCertificateTemplate_BasicCertificateTemplate|TestAccSqlDatabaseInstance_withPrivateNetwork|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=218087 |
71e222e
to
c47d39f
Compare
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccAssuredWorkloadsWorkload_BasicHandWritten|TestAccAssuredWorkloadsWorkload_FullHandWritten|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeRegionHealthCheck_tcp_update|TestAccComputeRegionHealthCheck_typeTransition|TestAccComputeRegionHealthCheck_logConfigDisabled|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeServiceAttachment_BasicHandWritten|TestAccContainerCluster_withILBSubsetting|TestAccContainerCluster_regionalWithNodePool|TestAccContainerCluster_withNodePoolBasic|TestAccContainerCluster_withNodePoolResize|TestAccContainerCluster_withNodePoolUpdateVersion|TestAccContainerCluster_withNodePoolAutoscaling|TestAccContainerCluster_withNodePoolMultiple|TestAccContainerCluster_withNodePoolNodeConfig|TestAccContainerCluster_withEnableKubernetesAlpha|TestAccContainerNodePool_basic|TestAccContainerNodePool_basicWithClusterId|TestAccContainerNodePool_nodeLocations|TestAccContainerNodePool_maxPodsPerNode|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_autoscaling|TestAccContainerNodePool_resize|TestAccContainerNodePool_regionalClusters|TestAccContainerNodePool_version|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccContainerNodePool_ephemeralStorageConfig|TestAccDataprocWorkflowTemplate_basic|TestAccEventarcTrigger_BasicHandWritten|TestAccMonitoringMonitoredProject_BasicMonitoredProject|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginBasicExample|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccOrgPolicyPolicy_EnforcePolicy|TestAccOrgPolicyPolicy_FolderPolicy|TestAccOrgPolicyPolicy_ProjectPolicy|TestAccPrivatecaCertificateTemplate_BasicCertificateTemplate|TestAccSqlDatabaseInstance_withPrivateNetwork|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=218106 |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccEventarcTrigger_BasicHandWritten|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=218308 |
Tests failed during RECORDING mode: TestAccEventarcTrigger_BasicHandWritten|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_basic|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccSqlUser_postgresIAM Please fix these to complete your PR |
Nice - the new docs are just as good as the old ones, thanks for being diligent on that point. |
Note: |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeServiceAttachment_ConsumerListsHandWritten|TestAccComputeServiceAttachment_BasicHandWritten|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccEventarcTrigger_BasicHandWritten|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccServiceDirectoryEndpoint_serviceDirectoryEndpointWithNetworkExample|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=220259 |
/gcbrun |
546d960
to
4f43639
Compare
Ran both manual checks. I believe that we'll need to correct the last diff.go change as it causes a negative change in behaviour for some users, so it'd count as a backwards incompatibility:
diff.go:
RELEASE_DIFF=true:
|
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccRedisInstanceDatasource_basic|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeBackendBucket_withCdnPolicy|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccEventarcTrigger_BasicHandWritten|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccRedisInstance_redisInstanceBasicExample|TestAccRedisInstance_redisInstanceFullExample|TestAccRedisInstance_update|TestAccRedisInstance_regionFromLocation|TestAccRedisInstance_redisInstanceAuthEnabled|TestAccRedisInstance_downgradeRedisVersion|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=221745 |
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.
Do you mind redoing the DCL upgrade to resolve conflicts? (upgrades are fairly annoying as they'll cause them fairly frequently)
@@ -166,7 +166,7 @@ func (l Link) Markdown() string { | |||
} | |||
|
|||
func (r *Resource) fillLinksFromExtensionsMap(m map[string]interface{}) { | |||
ref, ok := m["x-dcl-ref"].(map[string]interface{}) | |||
ref, ok := m["x-dcl-ref"].(map[interface{}]interface{}) |
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.
What prompted this change? Given I can't see any key changes, these still seems to be strings all of the time.
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.
With the previous line, ref is always an empty map and ok is always false and that's not the case with the new line. I suspect this has something to do with how the yaml is being read, but I'm not sure why the value is a map[interface{}]interface{}
rather than a map[string]interface{}
.
e9ecf8c
to
f8be99c
Compare
Fixed bug related to diff suppress function on array elements. Renamed variables in service attachment handwritten samples.
f8be99c
to
4db6dbc
Compare
Moved compute service attachment back to DCL implementation, fixing issues outlined in #5442
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)