Skip to content

Conversation

eddavisson
Copy link

@eddavisson eddavisson commented Oct 14, 2025

Add support for the Google Firestore UserCreds resource.

`google_firestore_user_creds`

@github-actions github-actions bot requested a review from BBBmau October 14, 2025 16:27
Copy link

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

Googlers: For automatic test runs see go/terraform-auto-test-runs.

@BBBmau, 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
Copy link
Collaborator

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.

google provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 71 insertions(+))

Missing test report

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

Resource: google_firestore_user_creds (2 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_firestore_user_creds" "primary" {
  resource_identity {
    principal = # value needed
  }
}

Missing service labels

The following new resources do not have corresponding service labels:

  • google_firestore_user_creds

If you believe this detection to be incorrect please raise the concern with your reviewer. Googlers: This error is safe to ignore once you've completed go/fix-missing-service-labels.
An override-missing-service-label label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 30
Passed tests: 28
Skipped tests: 0
Affected tests: 2

Click here to see the affected service packages
  • firestore

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
  • TestAccFirestoreUserCreds_firestoreUserCredsBasicExample
  • TestAccFirestoreUserCreds_firestoreUserCredsWithSecretManagerExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccFirestoreUserCreds_firestoreUserCredsBasicExample [Debug log]
TestAccFirestoreUserCreds_firestoreUserCredsWithSecretManagerExample [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🟢 All tests passed!

View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

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.

google provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 71 insertions(+))

Missing service labels

The following new resources do not have corresponding service labels:

  • google_firestore_user_creds

If you believe this detection to be incorrect please raise the concern with your reviewer. Googlers: This error is safe to ignore once you've completed go/fix-missing-service-labels.
An override-missing-service-label label can be added to allow merging.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 30
Passed tests: 30
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • firestore

🟢 All tests passed!

View the build log

Copy link
Collaborator

@BBBmau BBBmau left a comment

Choose a reason for hiding this comment

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

the service label error can be ignored since the team signed up today, some things to consider in this review

@github-actions github-actions bot requested a review from BBBmau October 14, 2025 22:27
@modular-magician
Copy link
Collaborator

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.

google provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 71 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 30
Passed tests: 30
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • firestore

🟢 All tests passed!

View the build log

@modular-magician
Copy link
Collaborator

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.

google provider: Diff ( 5 files changed, 752 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 752 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 71 insertions(+))

Errors

google provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

google-beta provider:

  • The diff processor failed to build. This is usually due to the downstream provider failing to compile.

@modular-magician
Copy link
Collaborator

Non-exercised tests

🔴 Tests were added that are skipped in VCR:

  • TestAccFirestoreUserCreds_firestoreUserCredsBasicExample
  • TestAccFirestoreUserCreds_firestoreUserCredsWithSecretManagerExample

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • firestore

🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

@modular-magician
Copy link
Collaborator

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.

google provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
google-beta provider: Diff ( 5 files changed, 740 insertions(+), 2 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 71 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 30
Passed tests: 30
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages
  • firestore

🟢 All tests passed!

View the build log

@BBBmau
Copy link
Collaborator

BBBmau commented Oct 17, 2025

Non-exercised tests

🔴 Tests were added that are skipped in VCR:

  • TestAccFirestoreUserCreds_firestoreUserCredsBasicExample
  • TestAccFirestoreUserCreds_firestoreUserCredsWithSecretManagerExample

Tests analytics

Total tests: 0 Passed tests: 0 Skipped tests: 0 Affected tests: 0

Click here to see the affected service packages

  • firestore

🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

apologies for the late reply, needed to run the two acceptance tests in a teamcity build. After running the two they both show passing.

@eddavisson
Copy link
Author

Thanks, @BBBmau -- let me know if any additional action is required on my part. I'm new to the repo (and have limited experience with GitHub in general). :-)

be UUID-like /[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}/.
required: true
custom_flatten: 'templates/terraform/custom_flatten/name_from_self_link.tmpl'
custom_expand: 'templates/terraform/custom_expand/shortname_to_url.go.tmpl'
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the reason behind including these the custom_flatten and custom_expand? It looks like it's asking for just the id of the user credential so the case of someone providing an entire project/*/database/* doesn't seem likely as its only asking for the id to use for the user cred being created.

Copy link
Author

Choose a reason for hiding this comment

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

I based this on what the other Firestore resources that support a short name are doing:

custom_flatten: 'templates/terraform/custom_flatten/name_from_self_link.tmpl'

But I believe it's necessary because the UserCreds message has a name field that contains the full project/*/database/*/userCreds/* name.

If I remove these and manually test, I see POST requests to /v1/projects/eddavisson-2025/databases/terraform/userCreds/projects/eddavisson-2025/databases/terraform/userCreds/ed-tf4 in the logs (with projects/eddavisson-2025/databases/terraform/userCreds/ed-tf4 duplicated in the path), which is incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants