Skip to content
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

[ENG-956] A Firestore module #1

Merged
merged 6 commits into from
Sep 30, 2024
Merged

Conversation

tweakster
Copy link
Contributor

@tweakster tweakster commented Sep 23, 2024

Description

  • A new module for creating Firestore resources

Issue(s)

ENG-956

How to test

  1. Check the variable docs looks sensible

Checklist

  • Set the repo to require the Terraform checks CI to pass before merge.

Copy link

@tweakster tweakster marked this pull request as draft September 23, 2024 11:28
@tweakster tweakster requested review from a team and removed request for a team September 24, 2024 12:48
@tweakster tweakster force-pushed the feat/ENG-956-firestore-module branch 4 times, most recently from 2d7bb35 to 283bac6 Compare September 25, 2024 13:56
@tweakster tweakster force-pushed the feat/ENG-956-firestore-module branch from 283bac6 to b0fe463 Compare September 25, 2024 13:59
@tweakster tweakster marked this pull request as ready for review September 25, 2024 14:08
@tweakster tweakster requested a review from a team September 25, 2024 14:08
@@ -0,0 +1,42 @@
# Google Cloud Firestore
Copy link

Choose a reason for hiding this comment

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

Do you think inputs and outputs would be useful here to see all parameters?

Terraform Docs which would show ins/outs specifically and is generated automagically: https://github.com/terraform-docs/terraform-docs

Copy link
Contributor Author

@tweakster tweakster Sep 30, 2024

Choose a reason for hiding this comment

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

Personally I prefer things in one place. I would expect people to read the code, it's not that hard for a developer.

We don't have a pipeline to build docs. If we find there is a need for them later we can add that but that just feels like bloat/complexity atm.


locals {
index_records = {
for i in var.indexes : join("_", concat([i.collection], i.fields[*].path)) => i
Copy link

Choose a reason for hiding this comment

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

Would + be better here rather than concat? I think this should work:

locals {
  index_records = {
    for i in var.indexes : join("_", [i.collection] + i.fields[*].path) => i
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷‍♀️ it feels totally subjective to me.

@tweakster tweakster merged commit 209a342 into main Sep 30, 2024
1 check passed
@tweakster tweakster deleted the feat/ENG-956-firestore-module branch September 30, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants