-
Notifications
You must be signed in to change notification settings - Fork 71
feat: support multiple firebase sites #58
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
Conversation
…terraform-dynamic-python-webapp into feat/multi-firebase
I was getting this in testing this code yesterday, and if I reapplied the terraform, I'd get a 409 error, but the site was created. |
…terraform-dynamic-python-webapp into feat/multi-firebase
grayside
left a comment
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.
LGTM with comments. Thank you for continuing to push through on making multi-deploy per project work!
|
|
||
| - id: init-all | ||
| name: 'gcr.io/cloud-foundation-cicd/$_DOCKER_IMAGE_DEVELOPER_TOOLS:$_DOCKER_TAG_VERSION_DEVELOPER_TOOLS' | ||
| args: ['/bin/bash', '-c', 'cft test run all --stage init --verbose'] |
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.
question: Since both sections below have an init step, what is this init-all doing?
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.
I'm not sure, this was copied from https://github.com/GoogleCloudPlatform/terraform-google-secure-cicd/blob/main/build/int.cloudbuild.yaml#L31 as a presumed 'best practice' (note: only example I could find)
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.
I'm chatting with tf blueprint maintainers around best practices here, tbd
| service_account = google_service_account.client.email | ||
| containers { | ||
| image = local.client_image | ||
| env { |
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.
discussion: At first I found this variable's presence on the Cloud Run Jobs mysterious, and wondered if we should have inline comments linking to usage. But I'm not sure we want to try maintaining a list like that to be comprehensive. This is one of those pieces that hand-off to future maintainers might lose track how to handle changes.
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.
I've added links to the script usage, which might help, but happy to do more
(code duplication fix pending)
With #56, will help resolve #45
This change uses Firebase Hosting Sites, which allows for a multiple deployments per project.
Suffixes are appended to the project ID to entire global uniqueness.