feat: update gcp action#1938
Conversation
|
Claude finished @benjamin-747's task in 2m 29s —— View job Code Review Complete ✓Tasks
SummaryThis PR adds GCP Artifact Registry support alongside existing AWS ECR for three deployment workflows. The changes enable pushing Docker images to both AWS and GCP registries simultaneously. 🔍 Detailed Review🔴 Critical Issues (Must Fix Before Merge)
|
| Category | Rating | Critical Issues |
|---|---|---|
| Security | 🔴 Needs Immediate Action | Service account keys (use Workload Identity instead) |
| Functionality | 🔴 Has Bugs | Buck2hub manifest job will fail |
| Code Quality | 🟢 Good | Clean structure, good use of set -euo pipefail |
| Performance | 🟢 Good | Efficient single-build approach, web has caching |
| Documentation | 🔴 Poor | No PR description, minimal inline comments |
✅ What's Good
- Single Build, Multiple Registries - Building once and tagging for both registries is efficient and ensures consistency
- Good Error Handling - Use of
set -euo pipefailthroughout scripts - Clean Code Structure - Clear separation of AWS and GCP authentication steps with comment headers
- GitHub Actions Cache - Added to web-deploy.yml (lines 96-97) for faster builds
- Simplified Matrix Strategy - Removed complex multi-platform matrix, focusing on amd64 is cleaner
🎯 Action Items Before Merge
Must Fix:
- Replace service account key auth with Workload Identity Federation
- Fix buck2hub manifest job (exclude from matrix OR add GCP auth)
- Add PR description explaining changes and architecture
Should Fix:
- Add image pull verification after GCP push
- Extract hard-coded region to environment variable
- Document project ID date range intent
- Add build caching to Rust workflows
Nice to Have:
- Add inline comments for conditional buck2hub logic
- Document rollback strategy for partial registry failures
📚 References
- Google's Workload Identity Federation Guide
- GitHub Actions Docker Build Cache
- GCP Artifact Registry Best Practices
Note: I was unable to add inline comments directly to specific lines because this PR appears to have an unusual branch configuration (main → main). The review above covers all critical findings with file paths and line numbers for reference.
There was a problem hiding this comment.
Pull request overview
This PR updates the GitHub Actions deployment workflows to add GCP Cloud Run deployment capability and remove the "openatom" environment. The changes split the existing deployment job into separate AWS and GCP deployment jobs, allowing the repository to deploy services to both cloud providers.
Changes:
- Removed "openatom" from deployment environment matrices in web-deploy.yml
- Renamed the "deploy" job to "deploy-aws" in all three deployment workflows for clarity
- Added new "deploy-gcp" jobs that authenticate to GCP and deploy to Cloud Run services using a force-redeploy strategy
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/web-deploy.yml | Removed "openatom" from matrix, renamed deploy job to deploy-aws, added deploy-gcp job for buck2hub-ui service |
| .github/workflows/orion-server-deploy.yml | Renamed deploy job to deploy-aws, added deploy-gcp job for buck2hub-orion service (repository check commented out) |
| .github/workflows/mono-engine-deploy.yml | Renamed deploy job to deploy-aws, added deploy-gcp job for buck2hub-mono service (repository check commented out) |
|
|
||
| deploy-gcp: | ||
| needs: manifest | ||
| # if: ${{ github.repository == 'web3infra-foundation/mega' }} |
There was a problem hiding this comment.
The repository check is commented out for the deploy-gcp job, which means this deployment will run on all forks of the repository, not just the canonical web3infra-foundation/mega repository. This could lead to unauthorized deployments or failed deployments on forks that don't have the necessary GCP_SA_KEY secret configured.
The deploy-aws job on line 125 has this check enabled, and the deploy-gcp job in web-deploy.yml (line 178) also has it enabled. For consistency and security, this check should be uncommented.
| # if: ${{ github.repository == 'web3infra-foundation/mega' }} | |
| if: ${{ github.repository == 'web3infra-foundation/mega' }} |
|
|
||
| deploy-gcp: | ||
| needs: manifest | ||
| # if: ${{ github.repository == 'web3infra-foundation/mega' }} |
There was a problem hiding this comment.
The repository check is commented out for the deploy-gcp job, which means this deployment will run on all forks of the repository, not just the canonical web3infra-foundation/mega repository. This could lead to unauthorized deployments or failed deployments on forks that don't have the necessary GCP_SA_KEY secret configured.
The deploy-aws job on line 135 has this check enabled, and the deploy-gcp job in web-deploy.yml (line 178) also has it enabled. For consistency and security, this check should be uncommented.
| # if: ${{ github.repository == 'web3infra-foundation/mega' }} | |
| if: ${{ github.repository == 'web3infra-foundation/mega' }} |
No description provided.