Handle bigquery_reservation_capacity_commitment creation when capacity_commitment_id is unspecified

[wj-chen]

Fixes hashicorp/terraform-provider-google#16204.

Release Note Template for Downstream PRs (will be copied)

bigqueryreservation: fixed bug of incorrect resource recreation when `capacity_commitment_id` is unspecified in resource `bigquery_reservation_capacity_commitment`

[modular-magician]

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@zli82016, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 65 insertions(+), 28 deletions(-))
Terraform Beta: Diff ( 4 files changed, 65 insertions(+), 28 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))
TF OiCS: Diff ( 4 files changed, 114 insertions(+))

[modular-magician]

Tests analytics

Total tests: 3151
Passed tests 2831
Skipped tests: 317
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigQueryDataTable_bigtable|TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample|TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentBasicExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_bigtable[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample[Error message] [Debug log]
TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentBasicExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[wj-chen]

Test error:

2023-10-17T20:16:14.562Z [WARN]  sdk.helper_resource: Error running Terraform CLI command:
  error=
  | exit status 1
  | 
  | Error: Import id "projects/ci-test-project-188019/locations/us-west2/capacityCommitments/capacity-tf-testn09wnxri4f" doesn't match any of the accepted formats: [^(?P<name>[^/]+)$]
  | 
  | 

I'm not familiar with import_format. Why did changing it to '{{name}}' affect the tests?

[zli82016]

Test error:

2023-10-17T20:16:14.562Z [WARN]  sdk.helper_resource: Error running Terraform CLI command:
  error=
  | exit status 1
  | 
  | Error: Import id "projects/ci-test-project-188019/locations/us-west2/capacityCommitments/capacity-tf-testn09wnxri4f" doesn't match any of the accepted formats: [^(?P<name>[^/]+)$]
  | 
  | 

I'm not familiar with import_format. Why did changing it to '{{name}}' affect the tests?

In tests, it also verifies if import works or not.

https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-9293-old..auto-pr-9293#diff-8d65f97183e94ff91babafd63b65068bc73eac0ad0677c4280a3501a85e287dfR101-R106

You can check what has been changed in the provider through this link https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-9293-old..auto-pr-9293

[zli82016]

You may try to use custom_import to make import work.

Example
https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/accesscontextmanager/AccessLevel.yaml#L55

[wj-chen]

Test error:

2023-10-17T20:16:14.562Z [WARN]  sdk.helper_resource: Error running Terraform CLI command:
  error=
  | exit status 1
  | 
  | Error: Import id "projects/ci-test-project-188019/locations/us-west2/capacityCommitments/capacity-tf-testn09wnxri4f" doesn't match any of the accepted formats: [^(?P<name>[^/]+)$]
  | 
  | 

I'm not familiar with import_format. Why did changing it to '{{name}}' affect the tests?

In tests, it also verifies if import works or not.

https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-9293-old..auto-pr-9293#diff-8d65f97183e94ff91babafd63b65068bc73eac0ad0677c4280a3501a85e287dfR101-R106

You can check what has been changed in the provider through this link https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-9293-old..auto-pr-9293

Thank you, that's a very handy link. Is it in the contributor guide somewhere?

Meanwhile, I don't quite understand how 1) my change to import_format would cause that diff (for example, the old supported format seemed to include also {{project}}/{{location}}/{{capacity_commitment_id}} and {{location}}/{{capacity_commitment_id}} but I don't see them defined in the YAML), or 2) why the acceptance test failed saying the import IDs didn't match. Though I can see in the case of import, capacity_commitment_id is required as part of the URI so maybe I don't need to update import_format. I'll test it out.

[zli82016]

You can check the diff report comment in this PR to find the Diff links to the generated resource file in the provider.

I don't think it is in the contributor guide https://googlecloudplatform.github.io/magic-modules/.

[zli82016]

Meanwhile, I don't quite understand how 1) my change to import_format would cause that diff (for example, the old supported format seemed to include also {{project}}/{{location}}/{{capacity_commitment_id}} and {{location}}/{{capacity_commitment_id}} but I don't see them defined in the YAML), or 2) why the acceptance test failed saying the import IDs didn't match. Though I can see in the case of import, capacity_commitment_id is required as part of the URI so maybe I don't need to update import_format. I'll test it out.

1. There is a process to generate the resource file based on the YAML file. YAML file will be converted to the object, which will be the input of the resource template. The function import_id_formats_from_resource generates the import id formats.

[zli82016]

Meanwhile, I don't quite understand how 1) my change to import_format would cause that diff (for example, the old supported format seemed to include also {{project}}/{{location}}/{{capacity_commitment_id}} and {{location}}/{{capacity_commitment_id}} but I don't see them defined in the YAML), or 2) why the acceptance test failed saying the import IDs didn't match. Though I can see in the case of import, capacity_commitment_id is required as part of the URI so maybe I don't need to update import_format. I'll test it out.

Yeah, import_format could stay the same.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 68 insertions(+), 25 deletions(-))
Terraform Beta: Diff ( 4 files changed, 68 insertions(+), 25 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))
TF OiCS: Diff ( 5 files changed, 121 insertions(+), 7 deletions(-))

[modular-magician]

Tests analytics

Total tests: 3162
Passed tests 2842
Skipped tests: 318
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample|TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentBasicExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample[Error message] [Debug log]
TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentBasicExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

[wj-chen]

New test error:

2023/10/19 02:27:05 [DEBUG] matching ID projects/ci-test-project-188019/locations/us-west2/capacityCommitments/3155304492080884857 to regex ^projects/(?P<project>[^/]+)/locations/(?P<location>[^/]+)/capacityCommitments/(?P<capacity_commitment_id>[^/]+)$.
2023/10/19 02:27:05 [DEBUG] importing project = ci-test-project-188019
2023/10/19 02:27:05 [DEBUG] importing location = us-west2
2023/10/19 02:27:05 [DEBUG] importing capacity_commitment_id = 3155304492080884857
2023-10-19T02:27:05.498Z [ERROR] sdk.proto: Response contains error diagnostic: diagnostic_detail="" diagnostic_severity=ERROR diagnostic_summary="The provider returned a resource missing an identifier during ImportResourceState. This is generally a bug in the resource implementation for import. Resource import code should not call d.SetId(\"\") or create an empty ResourceData. If the resource is missing, instead return an error. Please report this to the provider developers." tf_proto_version=5.3 tf_rpc=ImportResourceState tf_resource_type=google_bigquery_capacity_commitment tf_req_id=997cb41f-e56b-dd32-1c52-3f0f64c17117 tf_provider_addr=registry.terraform.io/hashicorp/google

From the log in the test, ID was set correctly: "[DEBUG] Finished creating CapacityCommitment", so what does the error message mean by "The provider returned a resource missing an identifier during ImportResourceState"?

[zli82016]

It looks like it becomes much more complicated to modify the id.

Can you please try the recommendation in the comment?

Also, I think encoder is needed to set the field capacity_commitment_id in state, similar to

magic-modules/mmv1/products/apigateway/ApiConfig.yaml

Line 92 in b57324b

encoder: 'templates/terraform/encoders/api_config.go.erb'

[wj-chen]

It looks like it becomes much more complicated to modify the id.

Can you please try the recommendation in the comment?

Also, I think encoder is needed to set the field capacity_commitment_id in state, similar to

magic-modules/mmv1/products/apigateway/ApiConfig.yaml

Line 92 in b57324b

encoder: 'templates/terraform/encoders/api_config.go.erb'

I don't feel like default_from_api is semantically correct in our case - the server doesn't set or return capacity_commitment_id. It remains user-specified only. The generated ID only gets returned as part of name.

[zli82016]

I don't feel like default_from_api is semantically correct in our case - the server doesn't set or return capacity_commitment_id. It remains user-specified only. The generated ID only gets returned as part of name.

name can be parsed to extract capacity_commitment_id in the encoder function.

With the current option to modify the id format, you have to use the custom_import code to fix the error The provider returned a resource missing an identifier during ImportResourceState to make import work. Because only project, location and capacity_commitment_id are parsed from the import id projects/{{project}}/locations/{{location}}/capacityCommitments/{{capacity_commitment_id}}, and '{{name}}' is unavailable during import.

Another problem is the url for get and patch as mentioned in the comment. The url for get and patch are not working well for empty capacity_commitment_id. But after changing self_link to '{{name}}', the get url doesn't work during importing, as name is unavailable.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 73 insertions(+), 25 deletions(-))
Terraform Beta: Diff ( 4 files changed, 73 insertions(+), 25 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-))
TF OiCS: Diff ( 5 files changed, 121 insertions(+), 7 deletions(-))

[modular-magician]

Tests analytics

Total tests: 3164
Passed tests 2842
Skipped tests: 317
Affected tests: 5

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccDataSourceGoogleServiceAccountJwt

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigqueryReservationCapacityCommitment_bigqueryReservationCapacityCommitmentNoIdExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample[Debug log]
TestAccDataSourceGoogleServiceAccountJwt[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[wj-chen]

I don't feel like default_from_api is semantically correct in our case - the server doesn't set or return capacity_commitment_id. It remains user-specified only. The generated ID only gets returned as part of name.

name can be parsed to extract capacity_commitment_id in the encoder function.

With the current option to modify the id format, you have to use the custom_import code to fix the error The provider returned a resource missing an identifier during ImportResourceState to make import work. Because only project, location and capacity_commitment_id are parsed from the import id projects/{{project}}/locations/{{location}}/capacityCommitments/{{capacity_commitment_id}}, and '{{name}}' is unavailable during import.

Another problem is the url for get and patch as mentioned in the comment. The url for get and patch are not working well for empty capacity_commitment_id. But after changing self_link to '{{name}}', the get url doesn't work during importing, as name is unavailable.

Looks like either way we'll need to introduce something custom - encoder or Import. I prefer using {{name}} because it's clear and representative of self_link and id. It's also already used in other BigQuery resources like bigquery_transfer_config so it's consistent. Since it was Import that had problems, I added a custom_import. The tests are passing now.

[zli82016]

LGTM. Thanks for working on it.