Skip to content

fix(gcp): remove the project from the number call#2231

Open
rh-hemartin wants to merge 1 commit into
mainfrom
feat/remove-quota-project
Open

fix(gcp): remove the project from the number call#2231
rh-hemartin wants to merge 1 commit into
mainfrom
feat/remove-quota-project

Conversation

@rh-hemartin

Copy link
Copy Markdown
Member
  • I found that doing this removed the need to enable cloudresourcemanager permissions on the GCP project.

Signed-off-by: Hector Martinez <hemartin@redhat.com>
@github-actions

Copy link
Copy Markdown

Site preview

Preview: https://2e6c018d-site.fullsend-ai.workers.dev

Commit: 0aa29ef08cd679f716e9d2d627d553d9496a797a

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:00 PM UTC · Completed 3:10 PM UTC
Commit: 0aa29ef · View workflow run →

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Low

  • [test adequacy] internal/dispatch/gcf/gcp_test.go:2076 — The existing TestLiveGCFClient_GetProjectNumber tests construct clients via newTestClient, which uses gcp.NewClientWithHTTP and leaves QuotaProject as the zero value. The new shallow-copy-and-clear-QuotaProject logic is never exercised by a test that starts with a non-empty QuotaProject.
    Remediation: Add a test case that sets c.Client.QuotaProject = "billing-project" before calling GetProjectNumber, and asserts the outgoing request does NOT contain the x-goog-user-project header.

  • [struct-copying-pattern] internal/dispatch/gcf/gcp.go:1866 — The shallow copy noQuotaClient := *c.Client shares pointer fields (httpClient, tokenFunc) with the original. This is safe because those fields are read-only after initialization, but the pattern is novel in this codebase. See also: [design-fit] finding at this location.
    Remediation: Add a brief inline comment noting the shallow copy is intentional and safe because the shared pointer fields are read-only after initialization.

  • [design-fit] internal/dispatch/gcf/gcp.go:1864 — The comment explains what is done (omit x-goog-user-project) but does not explain the billing model implication — that CRM API quota is billed to the caller's ADC credentials, not the target project. See also: [struct-copying-pattern] finding at this location.
    Remediation: Expand the comment to briefly note that CRM quota is billed to the caller's credentials, not the target project.

resp, err := c.Client.DoRequest(ctx, http.MethodGet, reqURL, "")
// CRM is a global API — omit x-goog-user-project to avoid requiring
// the API to be enabled on the target project.
noQuotaClient := *c.Client

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] struct-copying-pattern

The shallow copy noQuotaClient := *c.Client shares pointer fields (httpClient, tokenFunc) with the original. This is safe because those fields are read-only after initialization, but the pattern is novel in this codebase.

Suggested fix: Add a brief inline comment noting the shallow copy is intentional and safe because the shared pointer fields are read-only after initialization.

url.PathEscape(projectID))

resp, err := c.Client.DoRequest(ctx, http.MethodGet, reqURL, "")
// CRM is a global API — omit x-goog-user-project to avoid requiring

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] design-fit

The comment explains what is done (omit x-goog-user-project) but does not explain the billing model implication -- that CRM API quota is billed to the callers ADC credentials, not the target project.

Suggested fix: Expand the comment to briefly note that CRM quota is billed to the callers credentials, not the target project.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 12, 2026

@ralphbean ralphbean left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this works, then cloudresourcemanager still needs to be removed from 2-3 places in the cli and 4-5 places in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants