HYPERFLEET-1263 - fix: replace gcloud exec with Go pubsub SDK for subscription cleanup#137
Open
kuudori wants to merge 1 commit into
Open
HYPERFLEET-1263 - fix: replace gcloud exec with Go pubsub SDK for subscription cleanup#137kuudori wants to merge 1 commit into
kuudori wants to merge 1 commit into
Conversation
…scription cleanup
DeletePubSubSubscription shelled out to gcloud which isn't in the test
container image, making every cleanup call a silent no-op. Replace with
cloud.google.com/go/pubsub/v2 using Application Default Credentials.
- Replace exec.CommandContext("gcloud"...) with SubscriptionAdminClient.DeleteSubscription
- Use gRPC codes.NotFound for not-found detection instead of string matching
- Surface PermissionDenied and all other errors as hard failures
- Add package-level var newPubSubDeleteFunc for unit test injection
- Add 6 table-driven tests covering success, NotFound, PermissionDenied,
generic error, factory error, and default project fallback
- Verify cleanup function invocation via defer in all non-factory-error paths
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Sequence Diagram(s)sequenceDiagram
participant Helper as DeletePubSubSubscription
participant Factory as newPubSubDeleteFunc
participant Client as Pub/Sub admin client
Helper->>Factory: create deleteFn and cleanup
Factory->>Client: create client
Helper->>Client: delete subscription
Client-->>Helper: nil or gRPC error
alt NotFound
Helper-->>Helper: return nil
else other error
Helper-->>Helper: wrap error with subscription/project context
end
Helper->>Helper: defer cleanup()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/helper/adapter_test.go`:
- Around line 260-290: The DeletePubSubSubscription test only verifies the
default project path and ignores the subscription argument, so regressions in
forwarding the configured project or subscription can slip through. Update the
test around Helper.DeletePubSubSubscription and newPubSubDeleteFunc to capture
both projectID and subscription, then assert the configured GCPProjectID is
passed when set, the defaultGCPProjectID is used only when expected, and the
subscription ID matches the value supplied to DeletePubSubSubscription.
In `@pkg/helper/adapter.go`:
- Around line 561-564: The cleanup callback in the Pub/Sub admin client close
path is logging a real close failure at Info level, which downplays an error;
update the defer/cleanup function around client.Close() to use an error-level
log (via logger.Error) or return the close error to the caller if it should be
propagated. Keep the fix localized to the deleteFn cleanup logic so the log
severity matches the failure severity.
- Around line 580-595: The Pub/Sub delete path in the adapter currently returns
errors without project context, which breaks the error model for cleanup
failures. Update the newPubSubDeleteFunc setup in adapter.go so the factory
error is wrapped with subscriptionID and projectID context instead of returned
bare, and also include projectID in the wrapped error returned from the
deleteFn(ctx) failure path. Keep the existing logger calls in sync with the
returned error context so callers of the delete helper can diagnose ADC/client
and permission issues from the error alone.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af704f4e-5394-494e-a528-2b26c14a5c3a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/go.sum
📒 Files selected for processing (3)
go.modpkg/helper/adapter.gopkg/helper/adapter_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Comment on lines
+260
to
+290
| var capturedProjectID string | ||
| var cleanupCalled bool | ||
|
|
||
| original := newPubSubDeleteFunc | ||
| t.Cleanup(func() { newPubSubDeleteFunc = original }) | ||
|
|
||
| newPubSubDeleteFunc = func(_ context.Context, projectID, _ string) (func(context.Context) error, func(), error) { | ||
| capturedProjectID = projectID | ||
| if tt.factoryErr != nil { | ||
| return nil, nil, tt.factoryErr | ||
| } | ||
| return func(context.Context) error { return tt.deleteErr }, func() { cleanupCalled = true }, nil | ||
| } | ||
|
|
||
| h := &Helper{Cfg: &config.Config{GCPProjectID: tt.projectID}} | ||
| err := h.DeletePubSubSubscription(context.Background(), "test-sub") | ||
|
|
||
| if tt.wantErr { | ||
| if err == nil { | ||
| t.Fatal("expected error, got nil") | ||
| } | ||
| if tt.wantErrMsg != "" && !strings.Contains(err.Error(), tt.wantErrMsg) { | ||
| t.Errorf("error %q does not contain %q", err.Error(), tt.wantErrMsg) | ||
| } | ||
| } else if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if tt.wantDefaultProject && capturedProjectID != defaultGCPProjectID { | ||
| t.Errorf("expected default project %q, got %q", defaultGCPProjectID, capturedProjectID) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the configured project and subscription are forwarded.
The stub ignores the subscription ID, and capturedProjectID is only checked for the default case. A regression that always uses the default project or passes the wrong subscription can still pass.
Proposed change
var capturedProjectID string
+ var capturedSubscriptionID string
var cleanupCalled bool
@@
- newPubSubDeleteFunc = func(_ context.Context, projectID, _ string) (func(context.Context) error, func(), error) {
+ newPubSubDeleteFunc = func(_ context.Context, projectID, subscriptionID string) (func(context.Context) error, func(), error) {
capturedProjectID = projectID
+ capturedSubscriptionID = subscriptionID
if tt.factoryErr != nil {
return nil, nil, tt.factoryErr
}
@@
- if tt.wantDefaultProject && capturedProjectID != defaultGCPProjectID {
- t.Errorf("expected default project %q, got %q", defaultGCPProjectID, capturedProjectID)
+ wantProjectID := tt.projectID
+ if tt.wantDefaultProject {
+ wantProjectID = defaultGCPProjectID
+ }
+ if capturedProjectID != wantProjectID {
+ t.Errorf("expected project %q, got %q", wantProjectID, capturedProjectID)
+ }
+ if capturedSubscriptionID != "test-sub" {
+ t.Errorf("expected subscription %q, got %q", "test-sub", capturedSubscriptionID)
}As per path instructions, “New exported functions and critical logic paths SHOULD have tests.”
📝 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
| var capturedProjectID string | |
| var cleanupCalled bool | |
| original := newPubSubDeleteFunc | |
| t.Cleanup(func() { newPubSubDeleteFunc = original }) | |
| newPubSubDeleteFunc = func(_ context.Context, projectID, _ string) (func(context.Context) error, func(), error) { | |
| capturedProjectID = projectID | |
| if tt.factoryErr != nil { | |
| return nil, nil, tt.factoryErr | |
| } | |
| return func(context.Context) error { return tt.deleteErr }, func() { cleanupCalled = true }, nil | |
| } | |
| h := &Helper{Cfg: &config.Config{GCPProjectID: tt.projectID}} | |
| err := h.DeletePubSubSubscription(context.Background(), "test-sub") | |
| if tt.wantErr { | |
| if err == nil { | |
| t.Fatal("expected error, got nil") | |
| } | |
| if tt.wantErrMsg != "" && !strings.Contains(err.Error(), tt.wantErrMsg) { | |
| t.Errorf("error %q does not contain %q", err.Error(), tt.wantErrMsg) | |
| } | |
| } else if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if tt.wantDefaultProject && capturedProjectID != defaultGCPProjectID { | |
| t.Errorf("expected default project %q, got %q", defaultGCPProjectID, capturedProjectID) | |
| } | |
| var capturedProjectID string | |
| var capturedSubscriptionID string | |
| var cleanupCalled bool | |
| original := newPubSubDeleteFunc | |
| t.Cleanup(func() { newPubSubDeleteFunc = original }) | |
| newPubSubDeleteFunc = func(_ context.Context, projectID, subscriptionID string) (func(context.Context) error, func(), error) { | |
| capturedProjectID = projectID | |
| capturedSubscriptionID = subscriptionID | |
| if tt.factoryErr != nil { | |
| return nil, nil, tt.factoryErr | |
| } | |
| return func(context.Context) error { return tt.deleteErr }, func() { cleanupCalled = true }, nil | |
| } | |
| h := &Helper{Cfg: &config.Config{GCPProjectID: tt.projectID}} | |
| err := h.DeletePubSubSubscription(context.Background(), "test-sub") | |
| if tt.wantErr { | |
| if err == nil { | |
| t.Fatal("expected error, got nil") | |
| } | |
| if tt.wantErrMsg != "" && !strings.Contains(err.Error(), tt.wantErrMsg) { | |
| t.Errorf("error %q does not contain %q", err.Error(), tt.wantErrMsg) | |
| } | |
| } else if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| wantProjectID := tt.projectID | |
| if tt.wantDefaultProject { | |
| wantProjectID = defaultGCPProjectID | |
| } | |
| if capturedProjectID != wantProjectID { | |
| t.Errorf("expected project %q, got %q", wantProjectID, capturedProjectID) | |
| } | |
| if capturedSubscriptionID != "test-sub" { | |
| t.Errorf("expected subscription %q, got %q", "test-sub", capturedSubscriptionID) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/helper/adapter_test.go` around lines 260 - 290, The
DeletePubSubSubscription test only verifies the default project path and ignores
the subscription argument, so regressions in forwarding the configured project
or subscription can slip through. Update the test around
Helper.DeletePubSubSubscription and newPubSubDeleteFunc to capture both
projectID and subscription, then assert the configured GCPProjectID is passed
when set, the defaultGCPProjectID is used only when expected, and the
subscription ID matches the value supplied to DeletePubSubSubscription.
Source: Path instructions
Comment on lines
+561
to
+564
| return deleteFn, func() { | ||
| if err := client.Close(); err != nil { | ||
| logger.Info("failed to close Pub/Sub admin client", "error", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not log close failures at Info.
Line 563 records an actual cleanup error at Info, which hides resource-lifecycle failures. Use an error-level log, or make the cleanup callback return an error if callers should fail on close.
Proposed change
return deleteFn, func() {
if err := client.Close(); err != nil {
- logger.Info("failed to close Pub/Sub admin client", "error", err)
+ logger.Error("failed to close Pub/Sub admin client", "error", err)
}
}, nilAs per path instructions, “Log levels must match severity (no slog.Error for non-errors, no slog.Info for errors).”
📝 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
| return deleteFn, func() { | |
| if err := client.Close(); err != nil { | |
| logger.Info("failed to close Pub/Sub admin client", "error", err) | |
| } | |
| return deleteFn, func() { | |
| if err := client.Close(); err != nil { | |
| logger.Error("failed to close Pub/Sub admin client", "error", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/helper/adapter.go` around lines 561 - 564, The cleanup callback in the
Pub/Sub admin client close path is logging a real close failure at Info level,
which downplays an error; update the defer/cleanup function around
client.Close() to use an error-level log (via logger.Error) or return the close
error to the caller if it should be propagated. Keep the fix localized to the
deleteFn cleanup logic so the log severity matches the failure severity.
Source: Path instructions
Comment on lines
+580
to
+595
| deleteFn, cleanup, err := newPubSubDeleteFunc(ctx, projectID, subscriptionID) | ||
| if err != nil { | ||
| outputStr := string(output) | ||
| if strings.Contains(outputStr, "NOT_FOUND") || strings.Contains(outputStr, "not found") { | ||
| return err | ||
| } | ||
| defer cleanup() | ||
|
|
||
| if err := deleteFn(ctx); err != nil { | ||
| if status.Code(err) == codes.NotFound { | ||
| logger.Info("Pub/Sub subscription not found, skipping deletion", "subscription", subscriptionID) | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to delete Pub/Sub subscription %s: %w (output: %s)", subscriptionID, err, outputStr) | ||
| logger.Error("failed to delete Pub/Sub subscription", | ||
| "subscription", subscriptionID, | ||
| "project", projectID, | ||
| "error", err) | ||
| return fmt.Errorf("failed to delete Pub/Sub subscription %s: %w", subscriptionID, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wrap all Pub/Sub delete failures with project context.
Line 582 returns the factory error bare, and Line 595 omits projectID from the returned delete error. Callers only log the returned error, so ADC/client and permission failures lose the project needed to diagnose cleanup failures.
Proposed change
deleteFn, cleanup, err := newPubSubDeleteFunc(ctx, projectID, subscriptionID)
if err != nil {
- return err
+ return fmt.Errorf("failed to initialize Pub/Sub deletion for subscription %s in project %s: %w", subscriptionID, projectID, err)
}
defer cleanup()
@@
- return fmt.Errorf("failed to delete Pub/Sub subscription %s: %w", subscriptionID, err)
+ return fmt.Errorf("failed to delete Pub/Sub subscription %s in project %s: %w", subscriptionID, projectID, err)
}As per path instructions, “Wrap errors per Error Model Standard — no bare return err.”
📝 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
| deleteFn, cleanup, err := newPubSubDeleteFunc(ctx, projectID, subscriptionID) | |
| if err != nil { | |
| outputStr := string(output) | |
| if strings.Contains(outputStr, "NOT_FOUND") || strings.Contains(outputStr, "not found") { | |
| return err | |
| } | |
| defer cleanup() | |
| if err := deleteFn(ctx); err != nil { | |
| if status.Code(err) == codes.NotFound { | |
| logger.Info("Pub/Sub subscription not found, skipping deletion", "subscription", subscriptionID) | |
| return nil | |
| } | |
| return fmt.Errorf("failed to delete Pub/Sub subscription %s: %w (output: %s)", subscriptionID, err, outputStr) | |
| logger.Error("failed to delete Pub/Sub subscription", | |
| "subscription", subscriptionID, | |
| "project", projectID, | |
| "error", err) | |
| return fmt.Errorf("failed to delete Pub/Sub subscription %s: %w", subscriptionID, err) | |
| deleteFn, cleanup, err := newPubSubDeleteFunc(ctx, projectID, subscriptionID) | |
| if err != nil { | |
| return fmt.Errorf("failed to initialize Pub/Sub deletion for subscription %s in project %s: %w", subscriptionID, projectID, err) | |
| } | |
| defer cleanup() | |
| if err := deleteFn(ctx); err != nil { | |
| if status.Code(err) == codes.NotFound { | |
| logger.Info("Pub/Sub subscription not found, skipping deletion", "subscription", subscriptionID) | |
| return nil | |
| } | |
| logger.Error("failed to delete Pub/Sub subscription", | |
| "subscription", subscriptionID, | |
| "project", projectID, | |
| "error", err) | |
| return fmt.Errorf("failed to delete Pub/Sub subscription %s in project %s: %w", subscriptionID, projectID, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/helper/adapter.go` around lines 580 - 595, The Pub/Sub delete path in the
adapter currently returns errors without project context, which breaks the error
model for cleanup failures. Update the newPubSubDeleteFunc setup in adapter.go
so the factory error is wrapped with subscriptionID and projectID context
instead of returned bare, and also include projectID in the wrapped error
returned from the deleteFn(ctx) failure path. Keep the existing logger calls in
sync with the returned error context so callers of the delete helper can
diagnose ADC/client and permission issues from the error alone.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
gcloud pubsub subscriptions deleteshell-out inDeletePubSubSubscriptionwithcloud.google.com/go/pubsub/v2Go SDK using Application Default Credentialsexec: "gcloud": executable file not found in $PATH)PurgeAdapterQueue(googlepubsub path) now actually works — it dispatches to the fixedDeletePubSubSubscriptionWhat changed
pkg/helper/adapter.goexec.CommandContext("gcloud"...)withSubscriptionAdminClient.DeleteSubscription. Use gRPCcodes.NotFoundfor not-found detection (was string matching). SurfacePermissionDeniedand all other errors as hard failures. Package-levelnewPubSubDeleteFuncvar for test injection.pkg/helper/adapter_test.gogo.mod/go.sumcloud.google.com/go/pubsub/v2, upgradegoogle.golang.org/grpc1.59→1.80Design decisions
Test plan
make checkpasses (generate → fmt-check → vet → lint 0 issues → all tests with-race)make buildproduces binarycleanupCalledboolRelated