Introduce google_firestore_field resource. #7853

pcostell · 2023-05-01T13:25:51Z

Introduces a single resource for managing Firestore fields, matching the REST API.

Introduces 2 primary divergences from the proto/REST API:

Firestore fields do not support deletion (this is because all fields
implicitly can exist in a schemaless system). Deletion of a terraform
resource will clear any "overrides" for this field (TTL or indexing).
Single field indexes in the proto API were overly nested to allow
reusing the same resource for composite indexes. The terraform
resource unnests the 1-length field in the indexes list.

fixes hashicorp/terraform-provider-google#12151
fixes hashicorp/terraform-provider-google#11419
fixes hashicorp/terraform-provider-google#7593

Tested with make testacc TEST=./google TESTARGS='-run=TestAccFirestoreField'

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).
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).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (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)

`google_firestore_field`

modular-magician · 2023-05-01T13:26:20Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @SarahFrench, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

modular-magician · 2023-05-01T13:41:46Z

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 ( 5 files changed, 1130 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1130 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 160 insertions(+), 3 deletions(-))

modular-magician · 2023-05-01T14:35:51Z

Tests analytics

Total tests: 2698
Passed tests 2401
Skipped tests: 279
Affected tests: 18

Action taken

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

TestAccFirestoreField_firestoreFieldUpdateAddTTLExample|TestAccFirestoreField_firestoreFieldUpdateAddIndexExample|TestAccFirestoreField_firestoreFieldTimestampExample|TestAccFirestoreField_firestoreFieldBasicExample|TestAccFirestoreField_firestoreFieldMatchOverrideExample|TestAccCloudRunV2JobIamPolicyGenerated|TestAccCloudRunV2JobIamMemberGenerated|TestAccCloudRunV2JobIamBindingGenerated|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccCloudRunV2Job_cloudrunv2JobBasicExample|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample|TestAccCloudRunV2Job_cloudrunv2JobSqlExample|TestAccAlloydbCluster_missingLocation|TestAccAlloydbBackup_missingLocation|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccDataSourceAlloydbLocations_basic

Get to know how VCR tests work

modular-magician · 2023-05-01T14:53:23Z

Tests passed during RECORDING mode:
TestAccFirestoreField_firestoreFieldUpdateAddTTLExample[Debug log]
TestAccFirestoreField_firestoreFieldUpdateAddIndexExample[Debug log]
TestAccFirestoreField_firestoreFieldTimestampExample[Debug log]
TestAccFirestoreField_firestoreFieldBasicExample[Debug log]
TestAccFirestoreField_firestoreFieldMatchOverrideExample[Debug log]
TestAccCloudRunV2JobIamPolicyGenerated[Debug log]
TestAccCloudRunV2JobIamMemberGenerated[Debug log]
TestAccCloudRunV2JobIamBindingGenerated[Debug log]
TestAccCloudRunV2Job_cloudrunv2JobBasicExample[Debug log]
TestAccCloudRunV2Job_cloudrunv2JobFullUpdate[Debug log]
TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample[Debug log]
TestAccCloudRunV2Job_cloudrunv2JobSqlExample[Debug log]
TestAccAlloydbCluster_missingLocation[Debug log]
TestAccAlloydbBackup_missingLocation[Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccDataSourceAlloydbLocations_basic[Debug log]

Tests failed during RECORDING mode:
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

modular-magician · 2023-05-02T22:01:13Z

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 ( 5 files changed, 1133 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1133 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 160 insertions(+), 3 deletions(-))

modular-magician · 2023-05-02T22:56:35Z

Tests analytics

Total tests: 2711
Passed tests 2425
Skipped tests: 279
Affected tests: 7

Action taken

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

TestAccFirebaserulesRelease_BasicRelease|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbBackup_missingLocation|TestAccDataSourceAlloydbLocations_basic

Get to know how VCR tests work

modular-magician · 2023-05-02T23:09:34Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]
TestAccAlloydbCluster_missingLocation[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccAlloydbBackup_missingLocation[Debug log]
TestAccDataSourceAlloydbLocations_basic[Debug log]

All tests passed
View the build log or the debug log for each test

pcostell · 2023-05-02T23:43:33Z

@rwhogg or @clement would appreciate a Firestore-specific review as well.

SarahFrench

This PR looks good to me in terms of how the downstream code generation works, how the tests are comprehensive, and that the tests pass.

I'm not super familiar with Firestore (I reviewed while referring to the docs) so I'm glad you've tagged others for a Firestore-specific review. Once you've responded to any feedback from them and are satisfied give me an @-mention and I can merge the PR

mmv1/third_party/terraform/tests/resource_firestore_field_test.go

SarahFrench · 2023-05-03T10:50:36Z

mmv1/products/firestore/Field.yaml

+properties:
+  - !ruby/object:Api::Type::String
+    name: 'database'
+    default_value: '(default)'


I was going to ask about this default value, and whether there should be an acceptance tests that sets database to another value, but then I saw this in the docs:

A Cloud Firestore Database. Currently only one database is allowed per cloud project; this database must have a databaseId of '(default)'.

Noting this on the PR partly for myself as I'm unfamiliar with Firestore, partly for the benefit of anyone looking at the PR in future!

👍 Once this restriction changes, I expect we should use generative ones for testing.

mmv1/products/firestore/Field.yaml

mmv1/templates/terraform/custom_check_destroy/firestore_field.go.erb

mmv1/templates/terraform/custom_flatten/firestore_field_index_config.go.erb

rwhogg · 2023-05-03T15:18:39Z

mmv1/templates/terraform/custom_import/firestore_field.go.erb

+// Re-populate split fields from the name.
+re := regexp.MustCompile("^projects/([^/]+)/databases/([^/]+)/collectionGroups/([^/]+)/fields/(.+)$")
+match := re.FindStringSubmatch(d.Get("name").(string))
+if len(match) > 0{


Nit: please add a space after the 0

mmv1/products/firestore/Field.yaml

rwhogg · 2023-05-03T15:34:54Z

mmv1/templates/terraform/examples/firestore_field_basic.tf.erb

+  collection = "chatrooms_%{random_suffix}"
+  field = "basic"
+
+  index_config {


👍🏻 on all of the examples included. If possible, could we also add one that explicitly shows how to create an SFI exclusion?

Note these are all technically exclusions (they do not inherit from the ancestor configuration). The timestamp example covers the case where a user wants to completely disable their single field indexes. I added a comment there (and updated the description in the yaml) to make this more obvious.

rwhogg

Nice work!!! Left some largely-nitpicky comments.

pcostell · 2023-05-03T22:02:36Z

Thank you both for the review! Comments should be addressed, PTAL

modular-magician · 2023-05-03T22:11:41Z

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 ( 5 files changed, 1141 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1141 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 160 insertions(+), 3 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_firestore_field (3 total tests)
Untested fields: database

Please add acceptance tests which include these fields.

modular-magician · 2023-05-03T23:04:06Z

Tests analytics

Total tests: 2716
Passed tests 2429
Skipped tests: 279
Affected tests: 8

Action taken

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

TestAccFirebaserulesRelease_BasicRelease|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccComputeFirewallPolicyRule_multipleRules|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbBackup_missingLocation|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataSourceAlloydbLocations_basic

Get to know how VCR tests work

modular-magician · 2023-05-03T23:16:50Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccAlloydbCluster_missingLocation[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccAlloydbBackup_missingLocation[Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]
TestAccDataSourceAlloydbLocations_basic[Debug log]

Tests failed during RECORDING mode:
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

pcostell · 2023-05-05T15:53:17Z

@SarahFrench I think this is ready for merge if you're happy with it.

Introduces a single resource for managing Firestore fields, matching the REST API. Introduces 2 primary divergences from the proto/REST API: 1) Firestore fields do not support deletion (this is because all fields implicitly can exist in a schemaless system). Deletion of a terraform resource will clear any "overrides" for this field (TTL or indexing). 2) Single field indexes in the proto API were overly nested to allow reusing the same resource for composite indexes. The terraform resource unnests the 1-length field in the indexes list. fixes hashicorp/terraform-provider-google#12151 fixes hashicorp/terraform-provider-google#11419

modular-magician · 2023-05-05T19:31:22Z

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 ( 5 files changed, 1144 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 1144 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 3 files changed, 160 insertions(+), 3 deletions(-))

SarahFrench

LGTM - I'll let the tests run and will merge after assuming all is ok

modular-magician · 2023-05-05T20:24:30Z

Tests analytics

Total tests: 2716
Passed tests 2429
Skipped tests: 279
Affected tests: 8

Action taken

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

TestAccFirebaserulesRelease_BasicRelease|TestAccComputeFirewallPolicyRule_multipleRules|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbBackup_missingLocation|TestAccDataSourceAlloydbLocations_basic|TestAccDataSourceGoogleFirebaseAndroidAppConfig

Get to know how VCR tests work

modular-magician · 2023-05-05T20:37:51Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccAlloydbCluster_missingLocation[Debug log]
TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample[Debug log]
TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example[Debug log]
TestAccAlloydbBackup_missingLocation[Debug log]
TestAccDataSourceAlloydbLocations_basic[Debug log]
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]

Tests failed during RECORDING mode:
TestAccComputeFirewallPolicyRule_multipleRules[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

SarahFrench · 2023-05-05T20:50:41Z

Failed test is unrelated, merging

* Introduce google_firestore_field resource. Introduces a single resource for managing Firestore fields, matching the REST API. Introduces 2 primary divergences from the proto/REST API: 1) Firestore fields do not support deletion (this is because all fields implicitly can exist in a schemaless system). Deletion of a terraform resource will clear any "overrides" for this field (TTL or indexing). 2) Single field indexes in the proto API were overly nested to allow reusing the same resource for composite indexes. The terraform resource unnests the 1-length field in the indexes list. fixes hashicorp/terraform-provider-google#12151 fixes hashicorp/terraform-provider-google#11419 * Address feedback in review (mostly minor cleanups). * Fix whitespace and add database to basic example.

modular-magician requested a review from SarahFrench May 1, 2023 13:26

pcostell force-pushed the firestore-sfi branch from f631427 to 20bdf6c Compare May 2, 2023 21:43

SarahFrench approved these changes May 3, 2023

View reviewed changes