-
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 API Gateway resources #4066
Conversation
Fork Update
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. @danawillow, please review this PR or find an appropriate assignee. |
I have tested them but there are a few snags with
|
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 have tested them but there are a few snags with
google_api_gateway_api_config
:
- The API requires the openapiDocuments object in the POST call but doesn't return it in the GET call. Is there an API that behaves like that?
check out ignore_read
on the property
- I didn't implement the grpc definitions because it is not available yet. cloud.google.com/sdk/docs/release-notes#cloud_api_gateway_2 and I don't see grpc examples in the docs.
That's fine- it's always better to start with a resource that has too few fields and add more later, than to start with one that has too many and not all of them work.
- I need to write a diffsupression for the
googleServiceAccount
field.
Ok, let me know when that's done!
I started a first pass before realizing this didn't seem quite review-ready, so here are the comments I have in the meantime.
# The instance state - short description. | ||
# output: true | ||
# exclude: true | ||
- !ruby/object:Api::Type::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.
I don't think our users typically care about having createTime in state, so this can be removed (but it's not super important)
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'm confused by this behaviour. The diff is the same.
|
This is one of the frustrating things about certain api behaviors. The OpenAPI spec is available if you open the cloud console but they couldn't make the api send it back. |
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.
Mentioned this in the other PR I'm reviewing for you, but please re-read the release notes guide for new resources so that our team doesn't have to adjust the formatting later.
Also, this PR is a bit big for my liking, especially once you make the requested test fixes. Even if it's slower for you, it's far easier on our team to review a single resource in each PR (and paradoxically, it may end up making the overall process faster for you so that you don't have to wait for us to have a large enough chunk of uninterrupted time to do a review)
if serviceAccount.MatchString(new.(string)) { | ||
parts := serviceAccount.FindStringSubmatch(new.(string)) | ||
if old == "" { | ||
// Service account was initially null and the resource used the default service account. |
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.
Just to confirm- if you don't set a service account, does the API return a value for it?
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'm currently testing it without one and it returns the value of the default service account.
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.
In that case, could it just be an optional+computed field instead of needing the customizediff?
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.
Quick question, the ignore_read property doesn't seem to be doing anything. The email diff is fixed now.
# google_api_gateway_api_config.api_cfg will be updated in-place
~ resource "google_api_gateway_api_config" "api_cfg" {
api = "test"
api_config_id = "tf-test-api-cfgtcg2rpcoux"
display_name = "MM Dev API Config"
id = "projects/REDACTED/locations/global/apis/test/configs/tf-test-api-cfgtcg2rpcoux"
labels = {
"environment" = "dev"
}
name = "projects/550924169191/locations/global/apis/test/configs/tf-test-api-cfgtcg2rpcoux"
project = "REDACTED"
service_config_id = "tf-test-api-cfgtcg2rpcoux-1ft53xmbt9bmr"
gateway_config {
backend_config {
google_service_account = "projects/-/serviceAccounts/terraform@REDACTED.iam.gserviceaccount.com"
}
}
+ openapi_documents {
+ document {
+ contents = "c3dhZ2dlcjogJzIuMCcKaW5mbzoKICB0aXRsZTogZGV2IAogIGRlc2NyaXB0aW9uOiBTYW1wbGUgQVBJIG9uIEFQSSBHYXRld2F5IHdpdGggYSBDbG91ZCBSdW4gYmFja2VuZAogIHZlcnNpb246IDEuMC4wCnNjaGVtZXM6CiAgLSBodHRwcwpwcm9kdWNlczoKICAtIGFwcGxpY2F0aW9uL2pzb24KcGF0aHM6CiAgL2hlbGxvOgogICAgZ2V0OgogICAgICBzdW1tYXJ5OiBHcmVldCBhIHVzZXIKICAgICAgb3BlcmF0aW9uSWQ6IGhlbGxvCiAgICAgIHgtZ29vZ2xlLWJhY2tlbmQ6CiAgICAgICAgYWRkcmVzczogaHR0cHM6Ly9nb29nbGUuY29tCiAgICAgIHJlc3BvbnNlczoKICAgICAgICAnMjAwJzoKICAgICAgICAgIGRlc2NyaXB0aW9uOiBBIHN1Y2Nlc3NmdWwgcmVzcG9uc2UKICAgICAgICAgIHNjaGVtYToKICAgICAgICAgICAgdHlwZTogc3RyaW5n"
+ path = "spec.yaml"
}
}
}
Saw this issue https://github.com/GoogleCloudPlatform/magic-modules/issues/1019
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.
Does it make sense / work to ignore_read
on the whole openapi_documents
field instead of just on contents/path?
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, I applied it to the parent field and it works now.
I have it working now but couple of observations/questions:
All the tests are passing except for |
Yeah, that happens sometimes. I usually just play around with tests and API explorer until I find out what works.
There have been a bunch of changes made over time to apis to try to have a consistent style guide for how things should behave (see https://google.aip.dev/). This means that, unfortunately, things that were once canonical behavior are now legacy. Our job as Terraform provider developers is to try to expose a UX that works for our users regardless of whether the API was created yesterday or 10 years ago.
Does that cause issues practically, or is it just annoying because it's different?
Set
If you set
Nice, getting there! I'm going to hold off on a thorough review until you let me know that you're ready for one. |
It doesn't work. Also it generates dodgy getImportIdQualifiers.
No, the workaround is to use the id field instead of the name field. It is ready for review. If we can't get |
Cool, I can try to help get the IAM resources working tomorrow. My comment from my last round of review about adding more tests still stands, but I can at least run the ones that are there now. /gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=152391" |
There isn't much point in writing tests that update fields now. All the fields(except displayName and labels) are immutable right now for all 3 resources. |
How do we know that updating labels with Terraform works without a test to verify it? |
I see. I'll write a set tomorrow |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccDataSourceSpannerInstance_basic|TestAccApiGatewayApiIamBindingGenerated|TestAccApiGatewayApiIamMemberGenerated|TestAccApiGatewayApiIamPolicyGenerated|TestAccApiGatewayGatewayIamMemberGenerated|TestAccApiGatewayGatewayIamBindingGenerated|TestAccApiGatewayGatewayIamPolicyGenerated|TestAccProviderMeta_setModuleName|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExample|TestAccApiGatewayApi_apigatewayApiBasicExample|TestAccApiGatewayGateway_apigatewayGatewayBasicExample|TestAccComputeInstance_attachedDisk|TestAccComputeInstance_attachedDisk_sourceUrl|TestAccComputeInstance_attachedDisk_modeRo|TestAccComputeInstance_bootDisk_source|TestAccComputeInstance_kmsDiskEncryption|TestAccComputeInstance_bootDisk_sourceUrl|TestAccComputeInstance_attachedDiskUpdate|TestAccComputeInstance_bootDisk_type|TestAccComputeInstance_bootDisk_mode|TestAccComputeInstance_update|TestAccComputeInstance_forceNewAndChangeMetadata|TestAccComputeInstance_scheduling|TestAccComputeInstance_serviceAccount|TestAccComputeInstance_subnet_auto|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeInstance_subnet_custom|TestAccComputeInstance_networkIPAuto|TestAccComputeInstance_private_image_family|TestAccComputeInstance_network_ip_custom|TestAccComputeInstance_forceChangeMachineTypeManually|TestAccComputeInstance_multiNic|TestAccComputeInstance_deletionProtectionExplicitFalse|TestAccComputeInstance_primaryAliasIpRange|TestAccComputeInstance_deletionProtectionExplicitTrueAndUpdateFalse|TestAccComputeInstance_secondaryAliasIpRange|TestAccComputeInstance_hostname|TestAccComputeInstance_shieldedVmConfig|TestAccComputeInstance_enableDisplay|TestAccComputeInstance_desiredStatusUpdateBasic|TestAccComputeInstance_updateRunning_desiredStatusNotSet_notAllowStoppingForUpdate|TestAccComputeInstance_updateRunning_desiredStatusRunning_notAllowStoppingForUpdate|TestAccComputeInstance_desiredStatusTerminatedUpdateFields|TestAccComputeInstance_updateRunning_desiredStatusRunning_allowStoppingForUpdate|TestAccComputeInstance_updateRunning_desiredStatusTerminated_allowStoppingForUpdate|TestAccComputeInstance_desiredStatusOnCreation|TestAccComputeInstance_updateRunning_desiredStatusTerminated_notAllowStoppingForUpdate|TestAccComputeInstance_updateTerminated_desiredStatusNotSet_allowStoppingForUpdate|TestAccComputeMachineImage_machineImageBasicExample|TestAccComputeInstance_updateTerminated_desiredStatusTerminated_allowStoppingForUpdate|TestAccComputeInstance_updateTerminated_desiredStatusTerminated_notAllowStoppingForUpdate|TestAccComputeInstance_updateTerminated_desiredStatusNotSet_notAllowStoppingForUpdate|TestAccComputeInstance_updateTerminated_desiredStatusRunning_notAllowStoppingForUpdate|TestAccComputeInstance_updateTerminated_desiredStatusRunning_allowStoppingForUpdate|TestAccComputeInstance_subnetworkUpdate|TestAccComputeNodeGroup_nodeGroupAutoscalingPolicyExample|TestAccComputeNodeGroup_nodeGroupBasicExample|TestAccComputeNodeGroup_updateNodeTemplate|TestAccComputePacketMirroring_computePacketMirroringFullExample|TestAccComputePerInstanceConfig_update|TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample|TestAccComputeRegionAutoscaler_update|TestAccComputeRegionAutoscaler_scaleDownControl|TestAccComputeRegionBackendService_regionBackendServiceBalancingModeExample|TestAccComputeRegionBackendService_withBackendInternal|TestAccComputeRegionBackendService_withBackendMultiNic|TestAccComputeRegionBackendService_withBackendInternalManaged|TestAccComputeRegionBackendService_basic|TestAccComputeRegionDisk_deleteDetach|TestAccComputeRegionBackendService_ilbUpdateFull|TestAccRegionInstanceGroupManager_basic|TestAccRegionInstanceGroupManager_targetSizeZero|TestAccRegionInstanceGroupManager_update|TestAccRegionInstanceGroupManager_versions|TestAccRegionInstanceGroupManager_autoHealingPolicies|TestAccRegionInstanceGroupManager_stateful|TestAccRegionInstanceGroupManager_distributionPolicy|TestAccComputeRegionHealthCheck_tcp_update|TestAccComputeTargetInstance_targetInstanceBasicExample|TestAccComputeTargetInstance_targetInstanceCustomNetworkExample|TestAccContainerCluster_withNodeConfig|TestAccContainerCluster_withSandboxConfig|TestAccContainerNodePool_withSandboxConfig|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withLifecycleConfigIdleDeleteTtl|TestAccDataprocCluster_withAutoscalingPolicy|TestAccDNSPolicy_dnsPolicyBasicExample|TestAccDNSPolicy_update|TestAccIapBrand_iapBrandExample|TestAccIapClient_iapClientExample|TestAccNetworkManagementConnectivityTest_networkManagementConnectivityTestInstancesExample|TestAccNotebooksInstance_notebookInstanceBasicExample|TestAccNotebooksInstance_notebookInstanceBasicContainerExample|TestAccNotebooksInstance_notebookInstanceFullExample|TestAccOSConfigPatchDeployment_osConfigPatchDeploymentBasicExample|TestAccOSConfigGuestPolicies_osConfigGuestPoliciesBasicExample|TestAccOSConfigPatchDeployment_osConfigPatchDeploymentInstanceExample|TestAccStorageBucketIamPolicy You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=152473" |
Here's an example IAM config that works for API Config:
For the tests, reminder that it's good to have, for each resource, one test that sets as many fields as possible (to know they all work) and another that sets as few fields as possible (to know the resource still works if certain fields aren't set) |
This appeared recently. https://cloud.google.com/api-gateway/docs/reference/rest/v1beta/projects.locations.apis.configs/get#configview We are ready to merge this. All the tests are passing.
|
create_url: projects/{{project}}/locations/global/apis/{{api}}/configs?apiConfigId={{api_config_id}} | ||
self_link: projects/{{project}}/locations/global/apis/{{api}}/configs/{{api_config_id}} | ||
base_url: projects/{{project}}/locations/global/apis/{{api}}/configs | ||
read_query_params: '?view=FULL' |
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.
Have you tried just setting this on self_link
? If that works, then you can avoid having to add an extra MM attribute
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 haven't but I generated one now and it doesn't work. It replaced all the urls including Create and Delete methods
2020/10/21 19:23:23 [DEBUG] Google API Request Details:
---[ REQUEST ]---------------------------------------
DELETE /v1beta/projects/REDACTED/locations/global/apis/test/configs/tf-test-api-cfgwn1ovme2i3?alt=json&view=FULL HTTP/1.1
Host: apigateway.googleapis.com
User-Agent: Terraform/0.12.29 (+https://www.terraform.io) Terraform-Plugin-SDK/2.0.3 terraform-provider-google-beta/acc
Content-Type: application/json
Accept-Encoding: gzip
-----------------------------------------------------
2020/10/21 19:23:24 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 400 Bad Request
Connection: close
Transfer-Encoding: chunked
Alt-Svc: h3-Q050=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-27=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-T050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"
Cache-Control: private
Content-Type: application/json; charset=UTF-8
Date: Wed, 21 Oct 2020 18:23:24 GMT
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0
229
{
"error": {
"code": 400,
"message": "Invalid JSON payload received. Unknown name \"view\": Cannot bind query parameter. Field 'view' could not be found in request message.",
"status": "INVALID_ARGUMENT",
"details": [
{
"@type": "type.googleapis.com/google.rpc.BadRequest",
"fieldViolations": [
{
"description": "Invalid JSON payload received. Unknown name \"view\": Cannot bind query parameter. Field 'view' could not be found in request message."
}
]
}
]
}
}
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.
I'm going to go with the mm attribute.
# The instance state - short description. | ||
# output: true | ||
# exclude: true | ||
- !ruby/object:Api::Type::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.
^
Backend settings that are applied to all backends of the Gateway. | ||
properties: | ||
- !ruby/object:Api::Type::String | ||
name: 'googleServiceAccount' |
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.
same here
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.
No, it defaults to the GCE default service account.
Same with the parent 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.
That true if the user doesn't specify a gatewayConfig
though, right? If so, then we can just allow users that don't set one to have the API-given default, and require users that do set it to actually specify the 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 see, make it required on googleServiceAccount but not on backendConfig
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 guess my question is, is there a functional difference between these three scenarios?
1:
gatewayConfig: null
2:
gatewayConfig: {}
3:
gatewayConfig: {
backendConfig: {}
}
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.
The null is tested in the basic example and there are no problems there.
The other two.
REDACTED MCW0CDP3YY ~ $ echo '{"openapiDocuments":[{"document":{"contents":"c3dhZ2dlcjogJzIuMCcKaW5mbzoKICB0aXRsZTogZGV2IAogIGRlc2NyaXB0aW9uOiBTYW1wbGUgQVBJIG9uIEFQSSBHYXRld2F5IHdpdGggYSBDbG91ZCBSdW4gYmFja2VuZAogIHZlcnNpb246IDEuMC4wCnNjaGVtZXM6CiAgLSBodHRwcwpwcm9kdWNlczoKICAtIGFwcGxpY2F0aW9uL2pzb24KcGF0aHM6CiAgL2hlbGxvOgogICAgZ2V0OgogICAgICBzdW1tYXJ5OiBHcmVldCBhIHVzZXIKICAgICAgb3BlcmF0aW9uSWQ6IGhlbGxvCiAgICAgIHgtZ29vZ2xlLWJhY2tlbmQ6CiAgICAgICAgYWRkcmVzczogaHR0cHM6Ly9nb29nbGUuY29tCiAgICAgIHJlc3BvbnNlczoKICAgICAgICAnMjAwJzoKICAgICAgICAgIGRlc2NyaXB0aW9uOiBBIHN1Y2Nlc3NmdWwgcmVzcG9uc2UKICAgICAgICAgIHNjaGVtYToKICAgICAgICAgICAgdHlwZTogc3RyaW5n","path":"spec.yaml"}}],"gatewayConfig":{"backendConfig":{}}}' | http POST "https://apigateway.googleapis.com/v1beta/projects/REDACTED/locations/global/apis/test/configs?alt=json&apiConfigId=potato" "Authorization: Bearer $(gcloud auth print-access-token)"
HTTP/1.1 200 OK
Alt-Svc: h3-Q050=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-27=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-T050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"
Cache-Control: private
Content-Encoding: gzip
Content-Type: application/json; charset=UTF-8
Date: Wed, 21 Oct 2020 18:35:45 GMT
Server: ESF
Transfer-Encoding: chunked
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-XSS-Protection: 0
{
"done": false,
"metadata": {
"@type": "type.googleapis.com/google.cloud.apigateway.v1beta.OperationMetadata",
"apiVersion": "v1beta",
"createTime": "2020-10-21T18:35:45.486637683Z",
"requestedCancellation": false,
"target": "projects/REDACTED/locations/global/apis/test/configs/potato",
"verb": "create"
},
"name": "projects/REDACTED/locations/global/operations/operation-1603305344956-5b2329d8d7825-80f08ca0-cb70a3e5"
}
REDACTED MCW0CDP3YY ~ SIGINT $ echo '{"openapiDocuments":[{"document":{"contents":"c3dhZ2dlcjogJzIuMCcKaW5mbzoKICB0aXRsZTogZGV2IAogIGRlc2NyaXB0aW9uOiBTYW1wbGUgQVBJIG9uIEFQSSBHYXRld2F5IHdpdGggYSBDbG91ZCBSdW4gYmFja2VuZAogIHZlcnNpb246IDEuMC4wCnNjaGVtZXM6CiAgLSBodHRwcwpwcm9kdWNlczoKICAtIGFwcGxpY2F0aW9uL2pzb24KcGF0aHM6CiAgL2hlbGxvOgogICAgZ2V0OgogICAgICBzdW1tYXJ5OiBHcmVldCBhIHVzZXIKICAgICAgb3BlcmF0aW9uSWQ6IGhlbGxvCiAgICAgIHgtZ29vZ2xlLWJhY2tlbmQ6CiAgICAgICAgYWRkcmVzczogaHR0cHM6Ly9nb29nbGUuY29tCiAgICAgIHJlc3BvbnNlczoKICAgICAgICAnMjAwJzoKICAgICAgICAgIGRlc2NyaXB0aW9uOiBBIHN1Y2Nlc3NmdWwgcmVzcG9uc2UKICAgICAgICAgIHNjaGVtYToKICAgICAgICAgICAgdHlwZTogc3RyaW5n","path":"spec.yaml"}}],"gatewayConfig":{}}' | http POST "https://apigateway.googleapis.com/v1beta/projects/REDACTED/locations/global/apis/test/configs?alt=json&apiConfigId=tf-test-api-cfgvjqjfh298j" "Authorization: Bearer $(gcloud auth print-access-token)"
HTTP/1.1 200 OK
Alt-Svc: h3-Q050=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-27=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-T050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"
Cache-Control: private
Content-Encoding: gzip
Content-Type: application/json; charset=UTF-8
Date: Wed, 21 Oct 2020 18:31:53 GMT
Server: ESF
Transfer-Encoding: chunked
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-XSS-Protection: 0
{
"done": false,
"metadata": {
"@type": "type.googleapis.com/google.cloud.apigateway.v1beta.OperationMetadata",
"apiVersion": "v1beta",
"createTime": "2020-10-21T18:31:53.702041935Z",
"requestedCancellation": false,
"target": "projects/REDACTED/locations/global/apis/test/configs/tf-test-api-cfgvjqjfh298j",
"verb": "create"
},
"name": "projects/REDACTED/locations/global/operations/operation-1603305113163-5b2328fbc959d-c44b83b5-1c27fca5"
}
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.
It's not about whether or not there are problems, but about providing a consistent interface to our users and helping them write their configs correctly. If there are two ways of doing the exact same thing, we can simplify and make it so that there's only one way. hashicorp/terraform-provider-google#3928 has more context on why we try not to allow objects with only optional fields. It's a thing we allow only when there is a specific need.
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 don't follow.
Are you saying make all the child fields mandatory and the top level field (gatewayConfig) optional?
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.
Yup!
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.
Cool, did that in my last commit.
/gcbrun |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=153825" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccApiGatewayApiConfigIamBindingGenerated|TestAccApiGatewayApiConfigIamMemberGenerated|TestAccApiGatewayApiConfigIamPolicyGenerated|TestAccApiGatewayApiIamBindingGenerated|TestAccApiGatewayApiIamMemberGenerated|TestAccApiGatewayGatewayIamBindingGenerated|TestAccApiGatewayApiIamPolicyGenerated|TestAccDataSourceSpannerInstance_basic|TestAccApiGatewayGatewayIamPolicyGenerated|TestAccApiGatewayGatewayIamMemberGenerated|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExample|TestAccApiGatewayApiConfig_apigatewayApiConfigFullExample|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExampleUpdated|TestAccApiGatewayApi_apigatewayApiBasicExample|TestAccApiGatewayApi_apigatewayApiFullExample|TestAccApiGatewayGateway_apigatewayGatewayBasicExample|TestAccApiGatewayGateway_apigatewayGatewayFullExample|TestAccApiGatewayApi_apigatewayApiBasicExampleUpdated|TestAccApiGatewayGateway_apigatewayGatewayBasicExampleUpdated|TestAccComputeInstance_attachedDisk|TestAccComputeInstance_attachedDisk_sourceUrl|TestAccComputeInstance_attachedDisk_modeRo|TestAccComputeInstance_attachedDiskUpdate|TestAccComputeInstance_bootDisk_source|TestAccComputeInstance_kmsDiskEncryption|TestAccComputeInstance_bootDisk_sourceUrl|TestAccComputeInstance_private_image_family|TestAccComputeRegionDisk_deleteDetach|TestAccContainerCluster_withNodeConfigShieldedInstanceConfig You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=153831" |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=153837" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceSpannerInstance_basic|TestAccApiGatewayApiConfigIamBindingGenerated|TestAccApiGatewayApiConfigIamMemberGenerated|TestAccApiGatewayApiConfigIamPolicyGenerated|TestAccApiGatewayApiIamBindingGenerated|TestAccApiGatewayApiIamMemberGenerated|TestAccApiGatewayApiIamPolicyGenerated|TestAccApiGatewayGatewayIamBindingGenerated|TestAccApiGatewayGatewayIamMemberGenerated|TestAccApiGatewayGatewayIamPolicyGenerated|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExample|TestAccApiGatewayApiConfig_apigatewayApiConfigFullExample|TestAccApiGatewayApiConfig_apigatewayApiConfigBasicExampleUpdated|TestAccApiGatewayApi_apigatewayApiBasicExample|TestAccApiGatewayApi_apigatewayApiFullExample|TestAccApiGatewayApi_apigatewayApiBasicExampleUpdated|TestAccApiGatewayGateway_apigatewayGatewayBasicExample|TestAccApiGatewayGateway_apigatewayGatewayFullExample|TestAccApiGatewayGateway_apigatewayGatewayBasicExampleUpdated|TestAccComputeInstance_attachedDisk_sourceUrl|TestAccComputeInstance_attachedDisk|TestAccComputeInstance_attachedDisk_modeRo|TestAccComputeInstance_attachedDiskUpdate|TestAccComputeInstance_bootDisk_source|TestAccComputeInstance_bootDisk_sourceUrl|TestAccComputeInstance_kmsDiskEncryption|TestAccComputeInstance_private_image_family|TestAccComputeRegionDisk_deleteDetach|TestAccContainerCluster_withNodeConfigShieldedInstanceConfig You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=153862" |
Thanks |
Fixes: hashicorp/terraform-provider-google#7212
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)