Epic 0: Firebase auth, Cloud Run deploy, CI/CD automation#51
Open
Epic 0: Firebase auth, Cloud Run deploy, CI/CD automation#51
Conversation
- Add Firebase Terraform module with Identity Platform config for Google Sign-In and authorized domains (localhost for dev) - Add Cloud Run Terraform module with scale-to-zero, IAM, and CI image lifecycle management - Add deploy-api CI job: WIF auth → Artifact Registry → Cloud Run (main only) - Add mobile-build CI job: .env from secrets → Expo web export → artifact - Add bootstrap-secrets.sh to populate GitHub secrets from Terraform outputs - Wire firebase and cloud-run modules into dev environment with outputs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR establishes automated GCP infrastructure provisioning with Terraform, introduces Cloud Run deployment via GitHub Actions CI, adds Firebase and Identity Platform configuration, implements Workload Identity Federation for keyless authentication, and creates a new mobile web build pipeline using Expo. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant GitHub as GitHub Actions
participant GCP as Google Cloud
participant Terraform
participant Secrets as GitHub Secrets
User->>GitHub: Trigger bootstrap.yml
GitHub->>GCP: Authenticate with service account
GitHub->>GCP: Enable required APIs
GCP-->>GitHub: APIs enabled
GitHub->>GCP: Create GCS bucket & Artifact Registry
GitHub->>GCP: Create CI service account
GitHub->>GCP: Create Workload Identity pool & OIDC provider
GitHub->>GCP: Bind GitHub repo for OIDC impersonation
GitHub->>Terraform: terraform init
GitHub->>Terraform: terraform apply (Firebase, Cloud Run, etc.)
Terraform->>GCP: Provision Firebase project, web app, Identity Platform
Terraform->>GCP: Provision Cloud Run service
GCP-->>Terraform: Return resource outputs
Terraform-->>GitHub: Output values
GitHub->>Secrets: Set EXPO_PUBLIC_FIREBASE_* secrets
GitHub->>Secrets: Set EXPO_PUBLIC_API_URL secret
GitHub->>Secrets: Overwrite GCP_SERVICE_ACCOUNT to WIF email
Secrets-->>GitHub: Secrets configured
GitHub-->>User: Bootstrap complete
sequenceDiagram
actor Developer
participant GitHub as GitHub (main branch)
participant CI as CI/CD Pipeline
participant Registry as Artifact Registry
participant CloudRun as Cloud Run
Developer->>GitHub: Push code to main
GitHub->>CI: Trigger ci.yml workflow
CI->>CI: Authenticate to GCP (WIF)
CI->>CI: Build Docker image (apps/api)
CI->>Registry: Push image with sha & latest tags
Registry-->>CI: Image pushed
CI->>CloudRun: gcloud run deploy (with image sha)
CloudRun->>CloudRun: Update service with new image
CloudRun-->>CI: Deployment complete
CI-->>GitHub: Mark workflow success
CI->>CI: Build Expo web (apps/mobile)
CI->>CI: Export to dist/
CI->>GitHub: Upload web-build artifact (14-day retention)
GitHub-->>Developer: Deployment & builds complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
infra/terraform/modules/firebase/outputs.tf (1)
1-4:web_app_idandapp_idexport the same value.Both outputs reference
google_firebase_web_app.default.app_id. If intentional for different downstream consumers, consider adding a clarifying comment. Otherwise, consolidate to avoid confusion.Also applies to: 27-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/terraform/modules/firebase/outputs.tf` around lines 1 - 4, The outputs web_app_id and app_id both export the same expression google_firebase_web_app.default.app_id; either consolidate by removing one of the duplicate outputs (keep the preferred name) or change one output to a different value if both are intentionally needed, and if keeping both add a clarifying comment above the outputs explaining why both names are exported; update the output blocks named "web_app_id" and "app_id" (and the duplicate block referenced at lines 27-30) accordingly to avoid confusion..github/workflows/ci.yml (2)
195-201: Hardcoded service name and region could drift from Terraform.The service name
broodly-api-devand regionus-central1are hardcoded here but derived from variables in Terraform ("broodly-api-${var.environment}"andvar.region). If the Terraform configuration changes, this workflow won't automatically update.Consider extracting these as GitHub secrets (populated via
bootstrap-secrets.sh) or adding a comment noting the coupling to Terraform config ininfra/terraform/environments/dev/main.tf:190-198.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 195 - 201, The Deploy to Cloud Run step hardcodes the service name "broodly-api-dev" and region "us-central1" which can drift from Terraform's "broodly-api-${var.environment}" and var.region; change the step to read service name and region from GitHub secrets or workflow env vars (e.g., GCP_SERVICE_NAME, GCP_REGION) populated by bootstrap-secrets.sh, update the action to use those variables instead of literals, and add a short comment noting the coupling to Terraform variables so future changes to infra/terraform/environments/dev main.tf are not missed.
226-236: Consider grouping redirects for cleaner shell style.Static analysis (shellcheck SC2129) suggests using grouped redirects instead of individual
>> .envappends.♻️ Proposed refactor
- name: Generate .env from secrets working-directory: apps/mobile run: | - cat > .env << 'ENVEOF' - EXPO_PUBLIC_FIREBASE_USE_EMULATOR=false - ENVEOF - echo "EXPO_PUBLIC_FIREBASE_API_KEY=${{ secrets.EXPO_PUBLIC_FIREBASE_API_KEY }}" >> .env - echo "EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN=${{ secrets.EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN }}" >> .env - echo "EXPO_PUBLIC_FIREBASE_PROJECT_ID=${{ secrets.EXPO_PUBLIC_FIREBASE_PROJECT_ID }}" >> .env - echo "EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET=${{ secrets.EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET }}" >> .env - echo "EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID=${{ secrets.EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID }}" >> .env - echo "EXPO_PUBLIC_FIREBASE_APP_ID=${{ secrets.EXPO_PUBLIC_FIREBASE_APP_ID }}" >> .env - echo "EXPO_PUBLIC_API_URL=${{ secrets.EXPO_PUBLIC_API_URL }}" >> .env + { + echo "EXPO_PUBLIC_FIREBASE_USE_EMULATOR=false" + echo "EXPO_PUBLIC_FIREBASE_API_KEY=${{ secrets.EXPO_PUBLIC_FIREBASE_API_KEY }}" + echo "EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN=${{ secrets.EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN }}" + echo "EXPO_PUBLIC_FIREBASE_PROJECT_ID=${{ secrets.EXPO_PUBLIC_FIREBASE_PROJECT_ID }}" + echo "EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET=${{ secrets.EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET }}" + echo "EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID=${{ secrets.EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID }}" + echo "EXPO_PUBLIC_FIREBASE_APP_ID=${{ secrets.EXPO_PUBLIC_FIREBASE_APP_ID }}" + echo "EXPO_PUBLIC_API_URL=${{ secrets.EXPO_PUBLIC_API_URL }}" + } > .env🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 226 - 236, The run step currently creates .env with a heredoc then appends each secret with separate echo >> .env (triggering shellcheck SC2129); replace the multiple append calls by writing all variables in one grouped redirect (e.g., a single heredoc or a grouped { echo ...; echo ...; } > .env) so the EXPO_PUBLIC_FIREBASE_API_KEY, EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN, EXPO_PUBLIC_FIREBASE_PROJECT_ID, EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET, EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID, EXPO_PUBLIC_FIREBASE_APP_ID and EXPO_PUBLIC_API_URL are emitted in one write operation inside the same run block.scripts/bootstrap-secrets.sh (2)
35-39: Consider adding prerequisite checks forghCLI authentication.The script checks for Terraform initialization but doesn't verify
ghCLI is authenticated. A failedgh auth statuswould cause cryptic errors during secret setting.💡 Optional: Add gh CLI check
if [ ! -d "${TF_DIR}/.terraform" ]; then echo "ERROR: Terraform not initialized in ${TF_DIR}" echo "Run: cd ${TF_DIR} && terraform init" exit 1 fi +if ! gh auth status &>/dev/null; then + echo "ERROR: GitHub CLI not authenticated" + echo "Run: gh auth login" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bootstrap-secrets.sh` around lines 35 - 39, Add a prerequisite check that verifies the GitHub CLI is authenticated before attempting to set secrets: run `gh auth status` (or equivalent) and if it fails, print a clear error including that `gh` is not authenticated and how to login (e.g., `gh auth login`) and exit non‑zero; place this check near the Terraform initialization check and reference the same TF_DIR/secret-setting block so subsequent `gh` commands do not produce cryptic errors.
61-66: Error suppression may mask legitimate failures.Redirecting stderr to
/dev/nullhides both "output not found" (expected) and other errors like Terraform state issues or authentication problems.Consider capturing and checking the exit code separately to distinguish between missing outputs and actual failures.
♻️ Proposed fix to preserve error diagnostics
- value=$(cd "${TF_DIR}" && terraform output -raw "${tf_output}" 2>/dev/null) || value="" + if ! value=$(cd "${TF_DIR}" && terraform output -raw "${tf_output}" 2>&1); then + # Check if it's a "no output" error vs a real failure + if [[ "${value}" == *"No outputs found"* ]] || [[ "${value}" == *"not found"* ]]; then + value="" + else + echo " ERROR ${secret_name} — terraform output failed: ${value}" + errors=$((errors + 1)) + continue + fi + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bootstrap-secrets.sh` around lines 61 - 66, The current command hides all terraform errors by redirecting stderr to /dev/null when getting value with cd "${TF_DIR}" && terraform output -raw "${tf_output}". Change this to capture both stdout and stderr (e.g., stdout into value and stderr into a separate var), check the terraform exit code after running terraform output, and distinguish cases: if exit code is non-zero and stderr indicates the output is simply missing (e.g., "not found") then treat as empty/skip, but if exit code is non-zero for other reasons (auth/state errors) log the stderr and exit or fail the script; keep references to TF_DIR, tf_output, secret_name and value so you update that block accordingly.infra/terraform/modules/cloud-run/main.tf (1)
62-74: Consider requiring explicit opt-in for public access in production environments.The comment explains the security model well (Firebase Auth at application layer), but defaulting
allow_unauthenticated = truemeans new environments get public endpoints by default. For defense-in-depth, consider:
- Requiring explicit opt-in per environment, or
- Adding a validation rule that requires explicit acknowledgment in production
This is acceptable for dev but worth revisiting before prod deployment.
💡 Optional: Add validation for production environments
resource "google_cloud_run_v2_service_iam_member" "public" { count = var.allow_unauthenticated ? 1 : 0 + # Consider adding this validation in the module or environment config: + # validation { + # condition = var.environment != "prod" || !var.allow_unauthenticated + # error_message = "Production environment requires explicit allow_unauthenticated = false or security review." + # } + project = var.project_id location = var.region name = google_cloud_run_v2_service.api.name role = "roles/run.invoker" member = "allUsers" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/terraform/modules/cloud-run/main.tf` around lines 62 - 74, The module currently grants public access via the google_cloud_run_v2_service_iam_member.public when var.allow_unauthenticated is true; change the default and add guardrails: set allow_unauthenticated default to false and add a Terraform validation in the module (use the existing var.environment or add a var.production_allow_unauthenticated_ack boolean) that throws an error if environment == "prod" and allow_unauthenticated == true unless the explicit ack variable is true; update variable descriptions for allow_unauthenticated and the new ack variable so callers must explicitly opt in for prod.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 224-236: The workflow step "Generate .env from secrets" writes
EXPO_PUBLIC_* secrets into .env without validating presence; update that step to
validate each required secret (EXPO_PUBLIC_FIREBASE_API_KEY,
EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN, EXPO_PUBLIC_FIREBASE_PROJECT_ID,
EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET, EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID,
EXPO_PUBLIC_FIREBASE_APP_ID, EXPO_PUBLIC_API_URL) before appending to .env and
fail the job if any are empty by checking their expanded values and exiting
non‑zero with an explanatory error message so missing GitHub secrets do not
silently produce empty env vars.
In `@infra/terraform/modules/cloud-run/variables.tf`:
- Around line 11-14: The module declares variable "environment" but never uses
it; either remove the unused variable "environment" from the cloud-run module
variables.tf or consume it in the Cloud Run resources (e.g., add environment to
resource naming and/or labels for resources such as google_cloud_run_service
"service", google_cloud_run_service_iam_binding, and any related outputs) so the
module matches other modules' environment tagging conventions; update any
references (inputs/outputs) and the module call site accordingly to keep
interfaces consistent.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 195-201: The Deploy to Cloud Run step hardcodes the service name
"broodly-api-dev" and region "us-central1" which can drift from Terraform's
"broodly-api-${var.environment}" and var.region; change the step to read service
name and region from GitHub secrets or workflow env vars (e.g.,
GCP_SERVICE_NAME, GCP_REGION) populated by bootstrap-secrets.sh, update the
action to use those variables instead of literals, and add a short comment
noting the coupling to Terraform variables so future changes to
infra/terraform/environments/dev main.tf are not missed.
- Around line 226-236: The run step currently creates .env with a heredoc then
appends each secret with separate echo >> .env (triggering shellcheck SC2129);
replace the multiple append calls by writing all variables in one grouped
redirect (e.g., a single heredoc or a grouped { echo ...; echo ...; } > .env) so
the EXPO_PUBLIC_FIREBASE_API_KEY, EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN,
EXPO_PUBLIC_FIREBASE_PROJECT_ID, EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET,
EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID, EXPO_PUBLIC_FIREBASE_APP_ID and
EXPO_PUBLIC_API_URL are emitted in one write operation inside the same run
block.
In `@infra/terraform/modules/cloud-run/main.tf`:
- Around line 62-74: The module currently grants public access via the
google_cloud_run_v2_service_iam_member.public when var.allow_unauthenticated is
true; change the default and add guardrails: set allow_unauthenticated default
to false and add a Terraform validation in the module (use the existing
var.environment or add a var.production_allow_unauthenticated_ack boolean) that
throws an error if environment == "prod" and allow_unauthenticated == true
unless the explicit ack variable is true; update variable descriptions for
allow_unauthenticated and the new ack variable so callers must explicitly opt in
for prod.
In `@infra/terraform/modules/firebase/outputs.tf`:
- Around line 1-4: The outputs web_app_id and app_id both export the same
expression google_firebase_web_app.default.app_id; either consolidate by
removing one of the duplicate outputs (keep the preferred name) or change one
output to a different value if both are intentionally needed, and if keeping
both add a clarifying comment above the outputs explaining why both names are
exported; update the output blocks named "web_app_id" and "app_id" (and the
duplicate block referenced at lines 27-30) accordingly to avoid confusion.
In `@scripts/bootstrap-secrets.sh`:
- Around line 35-39: Add a prerequisite check that verifies the GitHub CLI is
authenticated before attempting to set secrets: run `gh auth status` (or
equivalent) and if it fails, print a clear error including that `gh` is not
authenticated and how to login (e.g., `gh auth login`) and exit non‑zero; place
this check near the Terraform initialization check and reference the same
TF_DIR/secret-setting block so subsequent `gh` commands do not produce cryptic
errors.
- Around line 61-66: The current command hides all terraform errors by
redirecting stderr to /dev/null when getting value with cd "${TF_DIR}" &&
terraform output -raw "${tf_output}". Change this to capture both stdout and
stderr (e.g., stdout into value and stderr into a separate var), check the
terraform exit code after running terraform output, and distinguish cases: if
exit code is non-zero and stderr indicates the output is simply missing (e.g.,
"not found") then treat as empty/skip, but if exit code is non-zero for other
reasons (auth/state errors) log the stderr and exit or fail the script; keep
references to TF_DIR, tf_output, secret_name and value so you update that block
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6104fd00-dcbd-4e2c-af56-ba0c675719b4
📒 Files selected for processing (13)
.github/workflows/ci.ymlinfra/terraform/environments/dev/main.tfinfra/terraform/environments/dev/outputs.tfinfra/terraform/environments/dev/variables.tfinfra/terraform/modules/cloud-run/.terraform.lock.hclinfra/terraform/modules/cloud-run/main.tfinfra/terraform/modules/cloud-run/outputs.tfinfra/terraform/modules/cloud-run/variables.tfinfra/terraform/modules/firebase/.terraform.lock.hclinfra/terraform/modules/firebase/main.tfinfra/terraform/modules/firebase/outputs.tfinfra/terraform/modules/firebase/variables.tfscripts/bootstrap-secrets.sh
There was a problem hiding this comment.
Pull request overview
Introduces infrastructure and CI/CD foundations for Firebase Auth + Cloud Run deployment, plus automation to propagate Terraform outputs into GitHub Actions secrets for Expo/mobile builds and Cloud Run deploys.
Changes:
- Added Terraform modules for Firebase (Identity Platform + web app config) and Cloud Run (service + IAM).
- Extended the
devTerraform environment to instantiate the new modules and expose outputs needed by CI/mobile. - Added CI jobs to deploy the API container to Cloud Run and produce an Expo web build artifact; added a script to bootstrap GitHub secrets from Terraform outputs.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/bootstrap-secrets.sh | New script to map Terraform outputs into GitHub Actions secrets via gh secret set. |
| infra/terraform/modules/firebase/main.tf | Provisions Firebase + Identity Platform config and conditionally enables Google IDP. |
| infra/terraform/modules/firebase/variables.tf | Defines Firebase module inputs (including Google Sign-In client ID/secret). |
| infra/terraform/modules/firebase/outputs.tf | Exposes Firebase web app config values as module outputs. |
| infra/terraform/modules/firebase/.terraform.lock.hcl | Adds a module-local provider lockfile. |
| infra/terraform/modules/cloud-run/main.tf | Provisions a Cloud Run v2 service and (optionally) grants public invoker IAM. |
| infra/terraform/modules/cloud-run/variables.tf | Defines Cloud Run module inputs, including allow_unauthenticated. |
| infra/terraform/modules/cloud-run/outputs.tf | Exposes Cloud Run service URL/name. |
| infra/terraform/modules/cloud-run/.terraform.lock.hcl | Adds a module-local provider lockfile. |
| infra/terraform/environments/dev/main.tf | Instantiates Firebase + Cloud Run modules for the dev environment. |
| infra/terraform/environments/dev/variables.tf | Adds dev vars for Google Sign-In OAuth client ID/secret. |
| infra/terraform/environments/dev/outputs.tf | Adds outputs used by the secrets bootstrap script and mobile CI job. |
| .github/workflows/ci.yml | Adds deploy-api and mobile-build jobs on pushes to main. |
Files not reviewed (2)
- infra/terraform/modules/cloud-run/.terraform.lock.hcl: Language not supported
- infra/terraform/modules/firebase/.terraform.lock.hcl: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- bootstrap.yml: One-time workflow (manual trigger) that creates all foundational GCP resources — state bucket, Artifact Registry, WIF, CI service account, then runs terraform apply and populates all GitHub secrets automatically. Only requires a temporary SA key. - terraform.yml: Ongoing infra management — plan on PRs (with comment), apply on merge to main. Uses WIF (no keys). Only triggers on infra/terraform/** changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the two manual steps (create SA key, add GitHub secret) and the fully automated bootstrap workflow that provisions all GCP resources. Includes troubleshooting, cleanup instructions, and reference tables for all created resources and secrets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/bootstrap.yml:
- Around line 17-24: Add a new workflow_dispatch input named
google_sign_in_client_id and thread it through the Terraform/dispatch steps:
update the top-level inputs block to include google_sign_in_client_id (required
when running phase two) and modify the job steps that currently pass project_id,
region, and environment (the 'with' mapping used to call Terraform or to
dispatch the second run) to also pass google_sign_in_client_id so the second
Firebase auth phase can run inside this workflow.
- Around line 25-28: The workflow exposes an unused inputs.destroy switch which
is misleading; either remove the input or implement its behavior by wiring
inputs.destroy into the job/step conditionals: locate the workflow input named
"destroy" and either delete that input block or update the Terraform job/steps
(the steps that run terraform init/plan/apply) to check
github.event.inputs.destroy (or inputs.destroy) and run terraform destroy when
true (and skip terraform apply), e.g., add an if: condition on the job/steps to
branch between terraform apply vs terraform destroy and ensure the default
remains "false".
In @.github/workflows/terraform.yml:
- Around line 60-68: The Terraform plan is being inlined directly into the
github-script `script` template using `${{ steps.plan.outputs.stdout }}`, which
can inject backticks or `${...}` into the JS source; instead pass the plan via
an environment variable (e.g., set env PLAN: ${{ steps.plan.outputs.stdout }} on
the actions/github-script step) and inside the `script` read it from
process.env.PLAN (or JSON.parse/JSON.stringify it) before constructing the
`body` so the plan is not interpolated into the script source; update the step
that uses `actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea` and
the `script` that currently references `${{ steps.plan.outputs.stdout }}` to use
the env variable approach.
- Around line 53-55: The Terraform workflow step "Terraform Plan" (id: plan) is
not passing the required project_id variable to Terraform; update the run
command for the "Terraform Plan" step to include -var="project_id=${{
secrets.GCP_PROJECT_ID }}" so terraform plan receives the secret, and make the
analogous change to the Terraform apply step (the terraform apply invocation
near lines 96–98) to pass the same -var="project_id=${{ secrets.GCP_PROJECT_ID
}}". Ensure both steps still use -no-color and the plan file (tfplan) as before.
In `@docs/runbooks/gcp-bootstrap.md`:
- Around line 3-15: Update the intro so it doesn't claim only two manual steps
but instead clarifies that there are two preparatory/manual setup steps required
before running the bootstrap workflow: rephrase the sentence referencing "two
manual steps" to "two preparation steps before running the bootstrap workflow
(bootstrap.yml)", and explicitly state that running the workflow and performing
credential cleanup in Steps 3 and 4 are subsequent manual actions; ensure
headings or bullets call out "Preparation Step 1" and "Preparation Step 2" and
reference the bootstrap.yml workflow and Steps 3 and 4 by name so readers
understand the sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f767f51d-ff0a-4e1a-b81e-1532f78be068
📒 Files selected for processing (4)
.github/workflows/bootstrap.yml.github/workflows/terraform.ymlREADME.mddocs/runbooks/gcp-bootstrap.md
✅ Files skipped from review due to trivial changes (1)
- README.md
Update bootstrap workflow and runbook to use the existing GCP_SERVICE_ACCOUNT secret (JSON key) instead of requiring a separate GCP_SA_KEY. After bootstrap, the secret is overwritten with the WIF service account email for all future CI/CD. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
terraform.yml: - Add TF_VAR_project_id from GCP_PROJECT_ID secret (critical — was missing) - Pass Terraform plan via env var to prevent script injection (security) - Use process.env for plan outcome in github-script bootstrap.yml: - Add google_sign_in_client_id workflow input for phase 2 auth setup - Remove unused destroy input - Thread client_id through terraform.tfvars when provided ci.yml: - Add secret validation step — fail early if required secrets are missing - Use grouped redirects for .env generation (SC2129) - Extract service name/region into env vars with coupling comment Terraform modules: - Remove module-level .terraform.lock.hcl files (root-level only) - Consolidate duplicate web_app_id/app_id outputs in firebase module - Add environment labels to Cloud Run service - Default allow_unauthenticated to false (explicit opt-in per environment) - Update IAM security comment to be accurate about auth posture - Make localhost conditional on dev environment in authorized_domains - Use distinct() to prevent duplicate domain entries - Add lifecycle precondition for client_secret when client_id is set - Add infra/terraform/modules/**/.terraform.lock.hcl to .gitignore bootstrap-secrets.sh: - Add gh auth status check before setting secrets - Derive repo from git remote instead of hardcoding - Use --body flag instead of echo pipe for secret values - Distinguish missing outputs from real Terraform errors docs: - Clarify "two preparation steps" wording in bootstrap runbook Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/bootstrap.yml (2)
254-274: Consider grouping redirects toGITHUB_STEP_SUMMARY(shellcheck SC2129).Multiple individual redirects can be consolidated using a command group for cleaner code.
♻️ Grouped redirect approach
- name: Summary run: | - echo "## Bootstrap Complete" >> "$GITHUB_STEP_SUMMARY" - echo "" >> "$GITHUB_STEP_SUMMARY" - echo "### Resources Created" >> "$GITHUB_STEP_SUMMARY" - echo "- GCS state bucket: \`broodly-terraform-state\`" >> "$GITHUB_STEP_SUMMARY" - ... + { + echo "## Bootstrap Complete" + echo "" + echo "### Resources Created" + echo "- GCS state bucket: \`broodly-terraform-state\`" + echo "- Artifact Registry: \`broodly\`" + echo "- Workload Identity Federation pool + provider" + echo "- CI service account: \`github-actions-ci@${{ inputs.project_id }}.iam.gserviceaccount.com\`" + echo "- All Terraform-managed resources (Cloud SQL, Firebase, Cloud Run, etc.)" + echo "" + echo "### GitHub Secrets Set" + echo "- \`GCP_PROJECT_ID\`, \`GCP_WORKLOAD_IDENTITY_PROVIDER\`, \`GCP_SERVICE_ACCOUNT\`" + echo "- \`EXPO_PUBLIC_FIREBASE_*\` (6 secrets)" + echo "- \`EXPO_PUBLIC_API_URL\`" + echo "" + echo "### Next Steps" + echo "1. **Delete the bootstrap service account** in GCP Console (no longer needed)" + echo "2. Enable Google Sign-In in Firebase Console → Authentication → Sign-in method" + echo "3. Retrieve the auto-created OAuth client ID from GCP Console → Credentials" + echo "4. Re-run this workflow with \`google_sign_in_client_id\` to complete auth setup" + } >> "$GITHUB_STEP_SUMMARY"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bootstrap.yml around lines 254 - 274, The Summary step currently appends many echo lines each redirecting to "$GITHUB_STEP_SUMMARY" which triggers shellcheck SC2129; modify the "Summary" job step to consolidate the redirects by using a single heredoc (e.g., cat <<'EOF' >> "$GITHUB_STEP_SUMMARY" ... EOF) or a grouped command block ({ echo ...; echo ...; } >> "$GITHUB_STEP_SUMMARY") to write all lines at once; update the block that contains the echo lines for the Summary step so it emits the same content but with one redirect to "$GITHUB_STEP_SUMMARY".
119-134: IAM role binding loop silently ignores errors.The
|| trueat line 133 suppresses all errors fromgcloud projects add-iam-policy-binding, including legitimate failures (permission denied, invalid role, etc.). Consider logging when a binding fails.♻️ Proposed fix to log binding failures
for role in \ roles/run.admin \ roles/artifactregistry.writer \ roles/iam.serviceAccountUser \ roles/storage.admin \ roles/firebase.admin \ roles/secretmanager.admin \ roles/cloudsql.admin \ roles/pubsub.admin; do - gcloud projects add-iam-policy-binding "${PROJECT_ID}" \ + if ! gcloud projects add-iam-policy-binding "${PROJECT_ID}" \ --member="serviceAccount:${SA_EMAIL}" \ --role="${role}" \ --condition=None \ - --quiet 2>/dev/null || true + --quiet 2>/dev/null; then + echo "Warning: Failed to add ${role} (may already exist)" + fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bootstrap.yml around lines 119 - 134, The binding loop that runs gcloud projects add-iam-policy-binding (iterating roles like roles/run.admin, using variables PROJECT_ID and SA_EMAIL) currently silences failures via "|| true"; remove that silent suppression and instead capture the exit status of each gcloud call and log failures (including the role being applied, PROJECT_ID and SA_EMAIL) to stderr or the workflow log so failures are visible while still continuing the loop; ensure you keep the loop and gcloud invocation (and --quiet if desired) but replace "|| true" with an explicit check that echoes or logs a descriptive error message when the command exits non‑zero..github/workflows/ci.yml (1)
229-243: Secret validation covers critical secrets only.The validation step checks
API_KEY,PROJECT_ID, andAPI_URL, but the.envgeneration also usesAUTH_DOMAIN,STORAGE_BUCKET,MESSAGING_SENDER_ID, andAPP_ID. If these are required for Firebase to function correctly, consider extending validation.If all Firebase secrets are required for the app to work properly, extend the validation:
- name: Validate required secrets env: FIREBASE_API_KEY: ${{ secrets.EXPO_PUBLIC_FIREBASE_API_KEY }} + FIREBASE_AUTH_DOMAIN: ${{ secrets.EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN }} FIREBASE_PROJECT_ID: ${{ secrets.EXPO_PUBLIC_FIREBASE_PROJECT_ID }} + FIREBASE_STORAGE_BUCKET: ${{ secrets.EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET }} + FIREBASE_MESSAGING_SENDER_ID: ${{ secrets.EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID }} + FIREBASE_APP_ID: ${{ secrets.EXPO_PUBLIC_FIREBASE_APP_ID }} API_URL: ${{ secrets.EXPO_PUBLIC_API_URL }} run: | missing=() [ -z "${FIREBASE_API_KEY}" ] && missing+=("EXPO_PUBLIC_FIREBASE_API_KEY") + [ -z "${FIREBASE_AUTH_DOMAIN}" ] && missing+=("EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN") [ -z "${FIREBASE_PROJECT_ID}" ] && missing+=("EXPO_PUBLIC_FIREBASE_PROJECT_ID") + [ -z "${FIREBASE_STORAGE_BUCKET}" ] && missing+=("EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET") + [ -z "${FIREBASE_MESSAGING_SENDER_ID}" ] && missing+=("EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID") + [ -z "${FIREBASE_APP_ID}" ] && missing+=("EXPO_PUBLIC_FIREBASE_APP_ID") [ -z "${API_URL}" ] && missing+=("EXPO_PUBLIC_API_URL")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 229 - 243, The "Validate required secrets" step only checks FIREBASE_API_KEY, FIREBASE_PROJECT_ID and API_URL but the .env generator also needs AUTH_DOMAIN, STORAGE_BUCKET, MESSAGING_SENDER_ID and APP_ID; update the validation in the CI job to also set env vars for AUTH_DOMAIN, STORAGE_BUCKET, MESSAGING_SENDER_ID and APP_ID (using the corresponding secrets.EXPO_PUBLIC_* names), add checks for each (e.g., [ -z "${AUTH_DOMAIN}" ] && missing+=("EXPO_PUBLIC_AUTH_DOMAIN")), and ensure the error message prints any newly missing secret names so the pipeline fails clearly when any Firebase-related secret is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/runbooks/gcp-bootstrap.md`:
- Around line 52-59: Change the heading "Preparation Step 2: Add the key as a
GitHub secret" from h3 to h2 so it's a sibling of "Preparation Step 1";
specifically locate the heading text "Preparation Step 2: Add the key as a
GitHub secret" in docs/runbooks/gcp-bootstrap.md and replace the leading "###"
with "##" to correct the Markdown heading hierarchy and ensure consistent
document structure.
In `@scripts/bootstrap-secrets.sh`:
- Around line 23-30: The flag handling loop currently assigns ENVIRONMENT="$2"
and REPO="$2" and does shift 2 without validating that a value exists, which
breaks with set -u if someone calls --env or --repo with no argument; update the
case entries for --env and --repo in the while/case block to first verify that
"$2" is set and does not begin with '-' (i.e., is not another flag) and if
validation fails print a clear error and exit nonzero, otherwise assign
ENVIRONMENT="$2" (for --env) or REPO="$2" (for --repo") and then shift 2; keep
DRY_RUN handling as-is.
- Around line 83-96: The temp stderr file (/tmp/tf_stderr.$$.txt) created when
calling terraform in the value assignment may be left behind on the error path
because the code does a continue before the rm -f; modify the error branch in
the if that handles terraform failures so it always removes the temp file before
continuing (or add a trap cleanup at the top of the loop using the same $$-based
filename), ensuring tf_stderr is still read into the tf_stderr variable and then
rm -f /tmp/tf_stderr.$$.txt is executed before the continue; key symbols to
update are the value=$(cd "${TF_DIR}" && terraform output -raw "${tf_output}"
...) call, the tf_stderr variable and the continue in that error-handling branch
(or add a loop-local trap for the tmp file).
---
Nitpick comments:
In @.github/workflows/bootstrap.yml:
- Around line 254-274: The Summary step currently appends many echo lines each
redirecting to "$GITHUB_STEP_SUMMARY" which triggers shellcheck SC2129; modify
the "Summary" job step to consolidate the redirects by using a single heredoc
(e.g., cat <<'EOF' >> "$GITHUB_STEP_SUMMARY" ... EOF) or a grouped command block
({ echo ...; echo ...; } >> "$GITHUB_STEP_SUMMARY") to write all lines at once;
update the block that contains the echo lines for the Summary step so it emits
the same content but with one redirect to "$GITHUB_STEP_SUMMARY".
- Around line 119-134: The binding loop that runs gcloud projects
add-iam-policy-binding (iterating roles like roles/run.admin, using variables
PROJECT_ID and SA_EMAIL) currently silences failures via "|| true"; remove that
silent suppression and instead capture the exit status of each gcloud call and
log failures (including the role being applied, PROJECT_ID and SA_EMAIL) to
stderr or the workflow log so failures are visible while still continuing the
loop; ensure you keep the loop and gcloud invocation (and --quiet if desired)
but replace "|| true" with an explicit check that echoes or logs a descriptive
error message when the command exits non‑zero.
In @.github/workflows/ci.yml:
- Around line 229-243: The "Validate required secrets" step only checks
FIREBASE_API_KEY, FIREBASE_PROJECT_ID and API_URL but the .env generator also
needs AUTH_DOMAIN, STORAGE_BUCKET, MESSAGING_SENDER_ID and APP_ID; update the
validation in the CI job to also set env vars for AUTH_DOMAIN, STORAGE_BUCKET,
MESSAGING_SENDER_ID and APP_ID (using the corresponding secrets.EXPO_PUBLIC_*
names), add checks for each (e.g., [ -z "${AUTH_DOMAIN}" ] &&
missing+=("EXPO_PUBLIC_AUTH_DOMAIN")), and ensure the error message prints any
newly missing secret names so the pipeline fails clearly when any
Firebase-related secret is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef93b4d4-0bbe-4b85-a44e-49d512591e5c
📒 Files selected for processing (13)
.github/workflows/bootstrap.yml.github/workflows/ci.yml.github/workflows/terraform.yml.gitignoredocs/runbooks/gcp-bootstrap.mdinfra/terraform/environments/dev/main.tfinfra/terraform/environments/dev/outputs.tfinfra/terraform/modules/cloud-run/main.tfinfra/terraform/modules/cloud-run/variables.tfinfra/terraform/modules/firebase/main.tfinfra/terraform/modules/firebase/outputs.tfinfra/terraform/modules/firebase/variables.tfscripts/bootstrap-secrets.sh
✅ Files skipped from review due to trivial changes (4)
- infra/terraform/modules/firebase/variables.tf
- .gitignore
- infra/terraform/environments/dev/outputs.tf
- infra/terraform/modules/firebase/outputs.tf
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/terraform.yml
- infra/terraform/environments/dev/main.tf
- infra/terraform/modules/cloud-run/main.tf
- infra/terraform/modules/firebase/main.tf
- infra/terraform/modules/cloud-run/variables.tf
Comment on lines
+52
to
+59
| ### Preparation Step 2: Add the key as a GitHub secret | ||
|
|
||
| 1. Go to your GitHub repo → **Settings** → **Secrets and variables** → **Actions** | ||
| 2. Click **New repository secret** | ||
| 3. **Name:** `GCP_SERVICE_ACCOUNT` | ||
| 4. **Value:** Paste the **entire contents** of the downloaded JSON key file | ||
| 5. Click **Add secret** | ||
|
|
There was a problem hiding this comment.
Heading level inconsistency: "Preparation Step 2" should be h2.
"Preparation Step 2" is currently h3 (###) which makes it appear nested under "Preparation Step 1". It should be h2 (##) to be a sibling heading.
-### Preparation Step 2: Add the key as a GitHub secret
+## Preparation Step 2: Add the key as a GitHub secretAs per coding guidelines, "Focus on documentation quality: clear structure, correct Markdown syntax, consistent formatting, accurate cross-references, and proper heading hierarchy."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Preparation Step 2: Add the key as a GitHub secret | |
| 1. Go to your GitHub repo → **Settings** → **Secrets and variables** → **Actions** | |
| 2. Click **New repository secret** | |
| 3. **Name:** `GCP_SERVICE_ACCOUNT` | |
| 4. **Value:** Paste the **entire contents** of the downloaded JSON key file | |
| 5. Click **Add secret** | |
| ## Preparation Step 2: Add the key as a GitHub secret | |
| 1. Go to your GitHub repo → **Settings** → **Secrets and variables** → **Actions** | |
| 2. Click **New repository secret** | |
| 3. **Name:** `GCP_SERVICE_ACCOUNT` | |
| 4. **Value:** Paste the **entire contents** of the downloaded JSON key file | |
| 5. Click **Add secret** | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/runbooks/gcp-bootstrap.md` around lines 52 - 59, Change the heading
"Preparation Step 2: Add the key as a GitHub secret" from h3 to h2 so it's a
sibling of "Preparation Step 1"; specifically locate the heading text
"Preparation Step 2: Add the key as a GitHub secret" in
docs/runbooks/gcp-bootstrap.md and replace the leading "###" with "##" to
correct the Markdown heading hierarchy and ensure consistent document structure.
Comment on lines
+23
to
+30
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| --dry-run) DRY_RUN=true; shift ;; | ||
| --env) ENVIRONMENT="$2"; shift 2 ;; | ||
| --repo) REPO="$2"; shift 2 ;; | ||
| *) echo "Unknown option: $1"; exit 1 ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Missing argument validation for --env and --repo flags.
If --env or --repo are passed without a value (e.g., ./bootstrap-secrets.sh --env), shift 2 will fail or $2 will be unset, causing the script to exit unexpectedly due to set -u.
🛡️ Proposed fix to validate arguments
while [[ $# -gt 0 ]]; do
case "$1" in
--dry-run) DRY_RUN=true; shift ;;
- --env) ENVIRONMENT="$2"; shift 2 ;;
- --repo) REPO="$2"; shift 2 ;;
+ --env)
+ if [[ -z "${2:-}" ]]; then echo "ERROR: --env requires a value"; exit 1; fi
+ ENVIRONMENT="$2"; shift 2 ;;
+ --repo)
+ if [[ -z "${2:-}" ]]; then echo "ERROR: --repo requires a value"; exit 1; fi
+ REPO="$2"; shift 2 ;;
*) echo "Unknown option: $1"; exit 1 ;;
esac
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while [[ $# -gt 0 ]]; do | |
| case "$1" in | |
| --dry-run) DRY_RUN=true; shift ;; | |
| --env) ENVIRONMENT="$2"; shift 2 ;; | |
| --repo) REPO="$2"; shift 2 ;; | |
| *) echo "Unknown option: $1"; exit 1 ;; | |
| esac | |
| done | |
| while [[ $# -gt 0 ]]; do | |
| case "$1" in | |
| --dry-run) DRY_RUN=true; shift ;; | |
| --env) | |
| if [[ -z "${2:-}" ]]; then echo "ERROR: --env requires a value"; exit 1; fi | |
| ENVIRONMENT="$2"; shift 2 ;; | |
| --repo) | |
| if [[ -z "${2:-}" ]]; then echo "ERROR: --repo requires a value"; exit 1; fi | |
| REPO="$2"; shift 2 ;; | |
| *) echo "Unknown option: $1"; exit 1 ;; | |
| esac | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bootstrap-secrets.sh` around lines 23 - 30, The flag handling loop
currently assigns ENVIRONMENT="$2" and REPO="$2" and does shift 2 without
validating that a value exists, which breaks with set -u if someone calls --env
or --repo with no argument; update the case entries for --env and --repo in the
while/case block to first verify that "$2" is set and does not begin with '-'
(i.e., is not another flag) and if validation fails print a clear error and exit
nonzero, otherwise assign ENVIRONMENT="$2" (for --env) or REPO="$2" (for
--repo") and then shift 2; keep DRY_RUN handling as-is.
Comment on lines
+83
to
+96
| if value=$(cd "${TF_DIR}" && terraform output -raw "${tf_output}" 2>/tmp/tf_stderr.$$.txt); then | ||
| tf_stderr=$(cat /tmp/tf_stderr.$$.txt 2>/dev/null) || true | ||
| else | ||
| tf_stderr=$(cat /tmp/tf_stderr.$$.txt 2>/dev/null) || true | ||
| # Distinguish "output not found" from real failures | ||
| if echo "${tf_stderr}" | grep -qiE "not found|no outputs"; then | ||
| value="" | ||
| else | ||
| echo " ERROR ${secret_name} — terraform output failed: ${tf_stderr}" | ||
| errors=$((errors + 1)) | ||
| continue | ||
| fi | ||
| fi | ||
| rm -f /tmp/tf_stderr.$$.txt |
There was a problem hiding this comment.
Temp file not cleaned up on error path.
When a Terraform error occurs (line 91-93), the script continues to the next iteration, skipping the rm -f on line 96. This leaves orphan temp files in /tmp.
🧹 Proposed fix to ensure cleanup
if echo "${tf_stderr}" | grep -qiE "not found|no outputs"; then
value=""
else
echo " ERROR ${secret_name} — terraform output failed: ${tf_stderr}"
errors=$((errors + 1))
+ rm -f /tmp/tf_stderr.$$.txt
continue
fi
fi
rm -f /tmp/tf_stderr.$$.txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/bootstrap-secrets.sh` around lines 83 - 96, The temp stderr file
(/tmp/tf_stderr.$$.txt) created when calling terraform in the value assignment
may be left behind on the error path because the code does a continue before the
rm -f; modify the error branch in the if that handles terraform failures so it
always removes the temp file before continuing (or add a trap cleanup at the top
of the loop using the same $$-based filename), ensuring tf_stderr is still read
into the tf_stderr variable and then rm -f /tmp/tf_stderr.$$.txt is executed
before the continue; key symbols to update are the value=$(cd "${TF_DIR}" &&
terraform output -raw "${tf_output}" ...) call, the tf_stderr variable and the
continue in that error-handling branch (or add a loop-local trap for the tmp
file).
Take main's more complete cloud-run module (db_connection_secret, cors_origin, traffic block, Secret Manager refs) and layer our additions: environment labels, lifecycle ignore for CI image deploys. Update action SHAs to match main's Dependabot bumps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
227-241:⚠️ Potential issue | 🟡 MinorSecret validation is incomplete—only 3 of 7 Firebase secrets are checked.
The validation step checks
FIREBASE_API_KEY,FIREBASE_PROJECT_ID, andAPI_URL, but the.envgeneration (lines 248-254) uses 4 additional Firebase secrets that aren't validated:AUTH_DOMAIN,STORAGE_BUCKET,MESSAGING_SENDER_ID, andAPP_ID. If any of these are missing, they'll be written as empty strings.If all Firebase secrets are required for the app to function correctly, consider extending the validation:
🛡️ Proposed fix to validate all secrets
- name: Validate required secrets env: FIREBASE_API_KEY: ${{ secrets.EXPO_PUBLIC_FIREBASE_API_KEY }} + FIREBASE_AUTH_DOMAIN: ${{ secrets.EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN }} FIREBASE_PROJECT_ID: ${{ secrets.EXPO_PUBLIC_FIREBASE_PROJECT_ID }} + FIREBASE_STORAGE_BUCKET: ${{ secrets.EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET }} + FIREBASE_MESSAGING_SENDER_ID: ${{ secrets.EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID }} + FIREBASE_APP_ID: ${{ secrets.EXPO_PUBLIC_FIREBASE_APP_ID }} API_URL: ${{ secrets.EXPO_PUBLIC_API_URL }} run: | missing=() [ -z "${FIREBASE_API_KEY}" ] && missing+=("EXPO_PUBLIC_FIREBASE_API_KEY") + [ -z "${FIREBASE_AUTH_DOMAIN}" ] && missing+=("EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN") [ -z "${FIREBASE_PROJECT_ID}" ] && missing+=("EXPO_PUBLIC_FIREBASE_PROJECT_ID") + [ -z "${FIREBASE_STORAGE_BUCKET}" ] && missing+=("EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET") + [ -z "${FIREBASE_MESSAGING_SENDER_ID}" ] && missing+=("EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID") + [ -z "${FIREBASE_APP_ID}" ] && missing+=("EXPO_PUBLIC_FIREBASE_APP_ID") [ -z "${API_URL}" ] && missing+=("EXPO_PUBLIC_API_URL")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 227 - 241, The validation step only checks FIREBASE_API_KEY, FIREBASE_PROJECT_ID and API_URL but not the other Firebase secrets used later; update the secret validation block to also export and verify EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN, EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET, EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID and EXPO_PUBLIC_FIREBASE_APP_ID (i.e., check variables AUTH_DOMAIN, STORAGE_BUCKET, MESSAGING_SENDER_ID, APP_ID), adding corresponding [ -z "..."] checks that append those keys to the missing array and preserve the existing error/exit behavior so the workflow fails if any of those secrets are absent.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
193-204: Hard-coded Cloud Run service name and region limits multi-environment deployment.The service name
broodly-api-devand regionus-central1are hard-coded, but Terraform generates these dynamically (broodly-api-${var.environment}). While the comment acknowledges this coupling, consider parameterizing via environment variables or GitHub repository variables to support staging/prod deployments without workflow changes.For now this works for dev-only, but if you plan to add more environments, consider:
♻️ Example parameterization approach
- name: Deploy to Cloud Run env: - CLOUD_RUN_SERVICE: broodly-api-dev - CLOUD_RUN_REGION: us-central1 + CLOUD_RUN_SERVICE: broodly-api-${{ vars.ENVIRONMENT || 'dev' }} + CLOUD_RUN_REGION: ${{ vars.GCP_REGION || 'us-central1' }} run: |Or use reusable workflows with environment inputs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 193 - 204, The Deploy to Cloud Run step currently hard-codes CLOUD_RUN_SERVICE=broodly-api-dev and CLOUD_RUN_REGION=us-central1; change it to read these values from workflow inputs/environment variables (e.g., use ENVIRONMENT, CLOUD_RUN_SERVICE and CLOUD_RUN_REGION), provide sensible defaults for dev (broodly-api-${ENVIRONMENT} / us-central1), and replace the fixed values in the run block so the gcloud command uses "${{ env.CLOUD_RUN_SERVICE }}" and "${{ env.CLOUD_RUN_REGION }}"; update the workflow header to accept an ENVIRONMENT input or set repository-level variables so the step (named "Deploy to Cloud Run") works for dev/staging/prod without editing the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 227-241: The validation step only checks FIREBASE_API_KEY,
FIREBASE_PROJECT_ID and API_URL but not the other Firebase secrets used later;
update the secret validation block to also export and verify
EXPO_PUBLIC_FIREBASE_AUTH_DOMAIN, EXPO_PUBLIC_FIREBASE_STORAGE_BUCKET,
EXPO_PUBLIC_FIREBASE_MESSAGING_SENDER_ID and EXPO_PUBLIC_FIREBASE_APP_ID (i.e.,
check variables AUTH_DOMAIN, STORAGE_BUCKET, MESSAGING_SENDER_ID, APP_ID),
adding corresponding [ -z "..."] checks that append those keys to the missing
array and preserve the existing error/exit behavior so the workflow fails if any
of those secrets are absent.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 193-204: The Deploy to Cloud Run step currently hard-codes
CLOUD_RUN_SERVICE=broodly-api-dev and CLOUD_RUN_REGION=us-central1; change it to
read these values from workflow inputs/environment variables (e.g., use
ENVIRONMENT, CLOUD_RUN_SERVICE and CLOUD_RUN_REGION), provide sensible defaults
for dev (broodly-api-${ENVIRONMENT} / us-central1), and replace the fixed values
in the run block so the gcloud command uses "${{ env.CLOUD_RUN_SERVICE }}" and
"${{ env.CLOUD_RUN_REGION }}"; update the workflow header to accept an
ENVIRONMENT input or set repository-level variables so the step (named "Deploy
to Cloud Run") works for dev/staging/prod without editing the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94d35f8e-f4cc-4b17-bdc3-0569bc9c52a4
📒 Files selected for processing (7)
.github/workflows/ci.ymlapps/api/serverinfra/terraform/environments/dev/main.tfinfra/terraform/environments/dev/outputs.tfinfra/terraform/modules/cloud-run/main.tfinfra/terraform/modules/cloud-run/variables.tftest-results/.last-run.json
✅ Files skipped from review due to trivial changes (1)
- test-results/.last-run.json
🚧 Files skipped from review as they are similar to previous changes (3)
- infra/terraform/modules/cloud-run/main.tf
- infra/terraform/environments/dev/outputs.tf
- infra/terraform/modules/cloud-run/variables.tf
don-petry
commented
Apr 4, 2026
Contributor
Author
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
Review: Epic 0 — Firebase auth, Cloud Run deploy, CI/CD automation
CI Failures
Terraform check: WIF authentication failure — the GCP_WORKLOAD_IDENTITY_PROVIDER secret references a pool/provider that doesn't exist yet. This is a bootstrap sequencing issue:
- The
bootstrap.ymlworkflow (manual dispatch) creates the WIF resources - The Terraform CI job requires those WIF resources to authenticate
- Fix: Either run the bootstrap workflow first, or add a graceful skip condition when WIF secrets are unconfigured
SonarCloud: Security Rating E (11 hotspots) — PR-specific findings in new infra code, not the systemic broodly issue.
Code Review Findings
| Severity | Issue | File |
|---|---|---|
| Critical | .terraform.lock.hcl pins provider v7.26.0, conflicting with root constraint ~> 5.0 |
Module lock files |
| High | allow_unauthenticated = true default makes Cloud Run publicly callable |
infra/modules/cloud-run/ |
| High | google_sign_in_client_secret defaults to empty — will cause API errors |
infra/modules/firebase-auth/ |
| Medium | authorized_domains always includes localhost in staging/prod |
infra/modules/firebase-auth/ |
| Medium | bootstrap-secrets.sh uses echo for secrets (fragile), lacks arg validation |
scripts/ |
| Low | Deploy step hardcodes service name and region | .github/workflows/ |
Recommended Fix Order
- Resolve provider version conflict (lock vs constraint)
- Run bootstrap workflow to create WIF pool/provider
- Address SonarCloud security hotspots (especially
allow_unauthenticated) - Fix bootstrap script robustness
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Contributor
Author
Automated review — NEEDS HUMAN REVIEWRisk: HIGH SummaryPR introduces GCP infra (Firebase/Cloud Run/WIF) with a sound overall design, but contains a critical committed binary (apps/api/server, 100755), two failing CI gates (Terraform + SonarCloud), and real IAM hardening gaps. The WIF provider is created without an --attribute-condition (the principalSet binding restricts by repo, but defense-in-depth is warranted), and the CI service account aggregates multiple admin roles (secretmanager/cloudsql/pubsub/storage/firebase/run). Escalate: cannot approve with a failing Terraform gate, a committed executable, and over-privileged auth surface. FindingsCritical
Major
Minor
Info
CI statusTwo required checks are failing on Reviewed by the don-petry PR-review cascade (triage: haiku 4.5 → deep: sonnet 4.6 + duck: gpt-5.4 → audit: opus 4.6). Reply with |
Contributor
|
Auto-rebase blocked — the base branch contains Please rebase this branch manually: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ignore_changeson image so CI can deploy new revisions without Terraform drift.deploy-apiCI job — builds Docker image, pushes to Artifact Registry, deploys to Cloud Run via Workload Identity Federation (main branch only).mobile-buildCI job — generates.envfrom GitHub secrets, runsexpo export --platform web, uploads build artifact.bootstrap-secrets.sh— reads Terraform outputs and populates GitHub Actions secrets viagh secret set. Supports--dry-runand--envflags.Two-phase Firebase setup
terraform apply: Creates Firebase project + Identity Platform. Firebase auto-creates an OAuth client for Google Sign-In.terraform applywithgoogle_sign_in_client_idvariable set to enable the Google provider.Required GitHub Secrets (manual, not from Terraform)
GCP_PROJECT_IDGCP_WORKLOAD_IDENTITY_PROVIDERGCP_SERVICE_ACCOUNTTest plan
terraform validate— all 3 modules pass (firebase, cloud-run, dev environment)terraform planagainst real GCP project./scripts/bootstrap-secrets.sh --dry-runafter Terraform apply🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation