Conversation
WalkthroughEnables the Google Cloud Storage API and adds a new Terraform-managed GCS bucket Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant WI as Workload Identity Pool
participant GCP as Google Cloud (IAM)
participant GCS as Google Cloud Storage
GH->>WI: Exchange OIDC token (repo attribute)
WI->>GCP: Token validated, maps to service account principal
GH->>GCS: Upload object (using mapped principal)
GCS->>GCP: Check IAM binding (roles/storage.objectCreator)
GCS-->>GH: Accept object
GCS-->>Public Users: Objects served via public read (roles/storage.objectViewer)
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 unit tests (beta)
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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
gcp/pull-reviews.tf (1)
34-38:⚠️ Potential issue | 🟠 MajorVerify the WIF repository claim matches the actual caller workflow repo.
Line 37 restricts writes to
attribute.repository/ambient-code/platform. For reusable workflows, GitHub OIDC standard claims describe the calling workflow repo; if uploads are triggered from another caller (for exampleambient-code/pull-reviews), this binding will 403 at runtime.In GitHub Actions reusable workflows, does the OIDC `repository` claim represent the calling workflow repository or the called reusable workflow repository? Please include the official GitHub docs reference.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gcp/pull-reviews.tf` around lines 34 - 38, The IAM binding in google_storage_bucket_iam_member.pull_reviews_gha_write currently restricts writes to attribute.repository/ambient-code/platform which will 403 if the OIDC token's repository claim is the calling workflow repo (e.g., ambient-code/pull-reviews); update the member principal to match the actual caller repo or broaden it (for example attribute.repository/ambient-code/pull-reviews or attribute.repository/ambient-code/*) after verifying which repository initiates the uploads, and re-run tests to confirm the principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github_pool.name}/attribute.repository/... value matches the real GitHub Actions caller.
🤖 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`:
- Line 9: Replace the hardcoded bucket name "ambient-code-pull-reviews" with a
project-scoped name to avoid global collisions: build the name using the project
identifier (e.g., var.project_id or data.google_project.current.project_id) and
optional environment suffix (e.g., format("%s-ambient-code-pull-reviews-%s",
var.project_id, var.env) or using substr(hash,…) for length), update the
resource that contains name = "ambient-code-pull-reviews" to use that
expression, and ensure any other references to the bucket use the same computed
value (extract into a variable or local like local.pull_reviews_bucket_name if
needed).
---
Duplicate comments:
In `@gcp/pull-reviews.tf`:
- Around line 34-38: The IAM binding in
google_storage_bucket_iam_member.pull_reviews_gha_write currently restricts
writes to attribute.repository/ambient-code/platform which will 403 if the OIDC
token's repository claim is the calling workflow repo (e.g.,
ambient-code/pull-reviews); update the member principal to match the actual
caller repo or broaden it (for example
attribute.repository/ambient-code/pull-reviews or
attribute.repository/ambient-code/*) after verifying which repository initiates
the uploads, and re-run tests to confirm the
principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github_pool.name}/attribute.repository/...
value matches the real GitHub Actions caller.
🪄 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: 4adfce13-37dc-4a35-a02e-345623f2930d
📒 Files selected for processing (1)
gcp/pull-reviews.tf
|
|
||
| resource "google_storage_bucket" "pull_reviews" { | ||
| project = var.project_id | ||
| name = "ambient-code-pull-reviews" |
There was a problem hiding this comment.
Use a project-scoped bucket name to avoid global-name collisions.
Line 9 uses a globally fixed GCS bucket name. Because bucket names are global (not project-scoped), this can break applies in other environments/projects and makes reuse brittle.
Proposed fix
resource "google_storage_bucket" "pull_reviews" {
project = var.project_id
- name = "ambient-code-pull-reviews"
+ name = "${var.project_id}-ambient-code-pull-reviews"
location = "US"As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| name = "ambient-code-pull-reviews" | |
| resource "google_storage_bucket" "pull_reviews" { | |
| project = var.project_id | |
| name = "${var.project_id}-ambient-code-pull-reviews" | |
| location = "US" | |
| uniform_bucket_level_access = true | |
| lifecycle_rule { | |
| action { | |
| type = "Delete" | |
| } | |
| condition { | |
| age = 45 | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gcp/pull-reviews.tf` at line 9, Replace the hardcoded bucket name
"ambient-code-pull-reviews" with a project-scoped name to avoid global
collisions: build the name using the project identifier (e.g., var.project_id or
data.google_project.current.project_id) and optional environment suffix (e.g.,
format("%s-ambient-code-pull-reviews-%s", var.project_id, var.env) or using
substr(hash,…) for length), update the resource that contains name =
"ambient-code-pull-reviews" to use that expression, and ensure any other
references to the bucket use the same computed value (extract into a variable or
local like local.pull_reviews_bucket_name if needed).
There was a problem hiding this comment.
@coderabbitai hard-coding works for simplicity.
There was a problem hiding this comment.
@ktdreyer, understood — a fixed name is simpler to reason about and reference externally. I'll defer to your judgement here.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
ktdreyer
left a comment
ktdreyer
left a comment
There was a problem hiding this comment.
principalSet looks right now
The pull-reviews GCS bucket (#5) requires CI to manage its IAM bindings on subsequent applies. The bucket itself was bootstrapped with a local "tofu apply" because "storage.buckets.create" is a project-level permission that CI does not have. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The pull-reviews GCS bucket (#5) requires CI to manage its IAM bindings on subsequent applies. I bootstrapped the bucket itself by running `tofu apply` on my computer. The "storage.buckets.create" permission is project-level, and I did not want to grant that to CI in order to limit the blast radius. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
This adds a simple GCS resource to the IaS configuration.