Conversation
WalkthroughThis PR adds Terraform infrastructure code to enable the Google Cloud Storage API and provision a public GCS bucket named Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gcp/pull-reviews.tf`:
- Around line 7-22: Add an explicit dependency so the google_storage_bucket
"pull_reviews" waits for the storage API to be enabled; update the
google_storage_bucket resource (resource "google_storage_bucket" "pull_reviews")
to include a depends_on referencing the google_project_service resource that
enables the storage API (e.g., depends_on = [google_project_service.storage]) to
prevent the bucket creation from running before the API is available.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0980db37-beef-4ece-97b9-1f6deb5dd911
📒 Files selected for processing (2)
gcp/apis.tfgcp/pull-reviews.tf
| resource "google_storage_bucket" "pull_reviews" { | ||
| project = var.project_id | ||
| name = "ambient-code-pull-reviews" | ||
| location = "US" | ||
|
|
||
| uniform_bucket_level_access = true | ||
|
|
||
| lifecycle_rule { | ||
| action { | ||
| type = "Delete" | ||
| } | ||
| condition { | ||
| age = 45 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding explicit dependency on the storage API.
Without depends_on, Terraform may attempt to create this bucket before google_project_service.storage is fully enabled, causing failures on initial apply.
Proposed fix
resource "google_storage_bucket" "pull_reviews" {
project = var.project_id
name = "ambient-code-pull-reviews"
location = "US"
uniform_bucket_level_access = true
lifecycle_rule {
action {
type = "Delete"
}
condition {
age = 45
}
}
+
+ depends_on = [google_project_service.storage]
}📝 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.
| resource "google_storage_bucket" "pull_reviews" { | |
| project = var.project_id | |
| name = "ambient-code-pull-reviews" | |
| location = "US" | |
| uniform_bucket_level_access = true | |
| lifecycle_rule { | |
| action { | |
| type = "Delete" | |
| } | |
| condition { | |
| age = 45 | |
| } | |
| } | |
| } | |
| resource "google_storage_bucket" "pull_reviews" { | |
| project = var.project_id | |
| name = "ambient-code-pull-reviews" | |
| location = "US" | |
| uniform_bucket_level_access = true | |
| lifecycle_rule { | |
| action { | |
| type = "Delete" | |
| } | |
| condition { | |
| age = 45 | |
| } | |
| } | |
| depends_on = [google_project_service.storage] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gcp/pull-reviews.tf` around lines 7 - 22, Add an explicit dependency so the
google_storage_bucket "pull_reviews" waits for the storage API to be enabled;
update the google_storage_bucket resource (resource "google_storage_bucket"
"pull_reviews") to include a depends_on referencing the google_project_service
resource that enables the storage API (e.g., depends_on =
[google_project_service.storage]) to prevent the bucket creation from running
before the API is available.
There was a problem hiding this comment.
@coderabbitai why would it do that? We don't want to add explicit deps on everything in gcp/apis.tf - that's noisy
There was a problem hiding this comment.
@ktdreyer, fair point — you're right to push back. The concern was about a potential race condition on an initial terraform apply when the Storage API hasn't been enabled yet, but in practice:
- Google's own best-practice guidance favors implicit dependencies (e.g.,
project_id = module.project_services.project_id) over explicitdepends_on, since referencing output attributes ensures Terraform waits for the upstream resource automatically. - The Storage API is a foundational GCP service and is very likely already enabled on any non-brand-new project, making this a low-real-world-risk concern.
- Scattering
depends_on = [google_project_service.*]across every resource in the codebase is noisy and adds maintenance overhead, as you noted.
The comment was already marked as a nitpick/trivial and was overly cautious. I'll drop it — no change needed here.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| resource "google_storage_bucket_iam_member" "pull_reviews_gha_write" { | ||
| bucket = google_storage_bucket.pull_reviews.name | ||
| role = "roles/storage.objectCreator" | ||
| member = "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github_pool.name}/attribute.repository/ambient-code/pull-reviews" |
There was a problem hiding this comment.
I wonder if we need principalSet to point at ambient-code/platform instead, if the actions will run under that repo?
I'm not sure how WIF authz works when we're including one action in another repo.
| service = "storage.googleapis.com" | ||
| disable_on_destroy = false | ||
| } | ||
|
|
ktdreyer
changed the title
2 participants
This adds a simple GCS resource to the IaS configuration.