Skip to content

feat(RHOAIENG-47026): add gcs resource #5

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.

Sign up for GitHub

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

Merged
ktdreyer merged 2 commits into from
Apr 9, 2026
Merged

feat(RHOAIENG-47026): add gcs resource #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
58b46f3
feat: add gcs resource
syntaxsdev Apr 7, 2026
3a86c45
feat: swap resource to write from platform repo
syntaxsdev Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions gcp/apis.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ resource "google_project_service" "cloudresourcemanager" {
disable_on_destroy = false
}

# Required for managing GCS buckets
resource "google_project_service" "storage" {
project = var.project_id
service = "storage.googleapis.com"
disable_on_destroy = false
}

Copy link
Copy Markdown
Contributor

@ktdreyer ktdreyer Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part LGTM

# Required for managing Workload Identity Federation settings
resource "google_project_service" "iam" {
project = var.project_id
Expand Down
38 changes: 38 additions & 0 deletions gcp/pull-reviews.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# GCS bucket for ambient-code/pull-reviews public content.
#
# This bucket serves publicly-accessible artifacts generated by
# the pull-reviews GitHub Actions workflow. Objects are automatically
# deleted after 45 days.

resource "google_storage_bucket" "pull_reviews" {
project = var.project_id
name = "ambient-code-pull-reviews"
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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).

Copy link
Copy Markdown
Contributor

@ktdreyer ktdreyer Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai hard-coding works for simplicity.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

location = "US"

uniform_bucket_level_access = true

lifecycle_rule {
action {
type = "Delete"
}
condition {
age = 45
}
}
}
Comment on lines +7 to +22
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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.

Copy link
Copy Markdown
Contributor

@ktdreyer ktdreyer Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai why would it do that? We don't want to add explicit deps on everything in gcp/apis.tf - that's noisy

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. Google's own best-practice guidance favors implicit dependencies (e.g., project_id = module.project_services.project_id) over explicit depends_on, since referencing output attributes ensures Terraform waits for the upstream resource automatically.
  2. 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.
  3. 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


# Allow anyone to read objects (public bucket).
resource "google_storage_bucket_iam_member" "pull_reviews_public_read" {
bucket = google_storage_bucket.pull_reviews.name
role = "roles/storage.objectViewer"
member = "allUsers"
}

# Allow the platform GitHub Actions workflow to upload objects.
# The platform repo runs the workflow that writes to this bucket,
# so the WIF principal must match that repo's OIDC token.
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/platform"
}
Loading