-
Notifications
You must be signed in to change notification settings - Fork 61
feat(terraform): automate firestore setup #163
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
terraform/prod.tf
Outdated
resource "google_app_engine_application" "prod_app" { | ||
project = "emblem-prod-${var.suffix}" | ||
# us-central1 not recognized by App Engine resource. | ||
location_id = "us-central" |
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.
This should be a variable.
location_id = "us-central" | |
location_id = var.google_region |
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.
Once I saw this suggestion in code, I realized this is a problem. Terraform expects app engine regions in a different formation, e.g., us-central
. At the same time, Cloud Run expects standard regions us-central1
.
From https://cloud.google.com/appengine/docs/locations it looks like we can adapt the Cloud Run region to App Engine by removing the numeric suffix. I'll need to investigate string manipulation options in Terraform.
@dinagraves should the comment above location_id be more emphatic? Even seeing it in the GitHub UI neither of us noticed it providing critical context to this discussion.
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
resource "google_app_engine_application" "prod_app" { | ||
project = "emblem-prod-${var.suffix}" | ||
# us-central1 not recognized by App Engine resource. | ||
# https://cloud.google.com/appengine/docs/locations |
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.
Can you add what this is expected to return?
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.
Overhauled the comment.
Co-authored-by: Dina Graves Portman <dinagraves@google.com>
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
Fixes #114
This change is based on proven work in another project. I did not test as part of Emblem because our current Emblem Terraform setup is not compatible with how I understand how to test Terraform. Sending PR to get reviews started, but flagging as do-not-merge for now while I sort out what needs to happen.