fix: Unify code responsible for upserting resources from sync subcommand#53
fix: Unify code responsible for upserting resources from sync subcommand#53
sync subcommand#53Conversation
📝 WalkthroughWalkthroughConsolidates per-command resource upsert logic by removing local Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI command
participant CTRL as ctrlp.UpsertResources
participant API as CtrlPlane API (client)
CLI->>CTRL: UpsertResources(ctx, resources, name)
CTRL->>API: build client (url, api-key, workspace)
CTRL->>API: perform UpsertResources request
API-->>CTRL: response (status / error)
CTRL-->>CLI: return error or nil
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 3
🤖 Fix all issues with AI agents
In `@cmd/ctrlc/root/sync/google/cloudrun/cloudrun.go`:
- Around line 126-156: This code inlines resource upsert logic in cloudrun.go
(using resourceprovider.New and rp.UpsertResource) instead of using the unified
ctrlp.UpsertResources helper and also calls
cliutil.HandleResponseOutput/Println; fix by migrating this command to call
ctrlp.UpsertResources like the other sync commands (remove the manual ctrlplane
client creation, resourceprovider.New, rp.UpsertResource, fmt.Println and direct
logging) or alternatively update ctrlp.UpsertResources to return the full
response so this command can call ctrlp.UpsertResources and then pass that
response to cliutil.HandleResponseOutput; choose one approach and update usages
accordingly to keep behavior consistent with other sync commands.
- Line 154: Remove the debug print that dumps the HTTP response by deleting the
fmt.Println(upsertResp) call; rely on the existing structured logger that
already records the status of the upsert response (leave the structured log call
intact) so no raw response is printed to stdout and sensitive/internal fields
aren't exposed.
In `@internal/common/common.go`:
- Around line 13-16: The function UpsertResources dereferences the pointer
parameter name without a nil check causing a potential panic; modify
UpsertResources to first check if name == nil and return a descriptive error
(e.g., "name is nil") before checking *name == "" so you never dereference a nil
pointer, and keep the existing empty-string error path (check name == nil ||
*name == "" and return appropriate errors) to locate the change look for the
UpsertResources(ctx context.Context, resources []api.ResourceProviderResource,
name *string) signature.
🧹 Nitpick comments (4)
internal/common/common.go (1)
18-20: No validation of required Viper config values.If
url,api-key, orworkspaceare empty/unset, the function will proceed and fail with a potentially confusing downstream error. Consider validating these early with a clear message, especially since this is the centralized path for all providers now.cmd/ctrlc/root/sync/aws/networks/networks.go (1)
70-72: Remove leftover commented-out code.These commented-out viper lines are remnants of the old upsert logic and are no longer needed.
- //apiURL := viper.GetString("url") - //apiKey := viper.GetString("api-key") - //workspaceId := viper.GetString("workspace") -cmd/ctrlc/root/sync/google/cloudsql/cloudsql.go (1)
222-228: Inconsistent default provider name format across sync commands.This file uses
google-cloudsql-{project}(noproject-infix), while other Google sync commands (networks, vms, bigtable, gke, buckets) use the patterngoogle-{service}-project-{project}. Consider aligning for consistency.Suggested fix
- *providerName = fmt.Sprintf("google-cloudsql-%s", *project) + *providerName = fmt.Sprintf("google-cloudsql-project-%s", *project)cmd/ctrlc/root/sync/kubernetes/kubernetes.go (1)
122-136: Inconsistent context usage:context.Background()vsctx.Lines 124 and 134 pass
context.Background()toprocessNamespaceandprocessDeployment, while the rest of the function consistently uses thectxvariable from line 81. Usectxeverywhere for consistency, even though the parameter is currently unused in those functions.♻️ Proposed fix
for _, namespace := range namespaces.Items { - resource := processNamespace(context.Background(), clusterName, namespace) + resource := processNamespace(ctx, clusterName, namespace) resources = append(resources, resource) }for _, deployment := range deployments.Items { - resource := processDeployment(context.Background(), clusterName, deployment) + resource := processDeployment(ctx, clusterName, deployment) resources = append(resources, resource) }
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/common/common.go`:
- Around line 32-38: The UpsertResource caller currently ignores non-2xx HTTP
responses: after calling SetResourceProviderResources and getting upsertResp,
check the HTTP status code on upsertResp (e.g., upsertResp.StatusCode or
upsertResp.Status) and return an error for unexpected codes (match the existing
pattern used in internal/api/providers/resource.go — for example if
resp.StatusCode() != http.StatusAccepted { return fmt.Errorf("upsert resources
failed: status %d", resp.StatusCode()) }); keep the logging of the response but
ensure non-success status codes cause UpsertResource to return a descriptive
error instead of nil.
| upsertResp, err := rp.UpsertResource(ctx, resources) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to upsert resources: %w", err) | ||
| } | ||
|
|
||
| log.Info("Response from upserting resources", "status", upsertResp.Status) | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
ast-grep --pattern $'func ($_, $_) UpsertResource($_, $_) ($_, $_) {
$$$
}'Repository: ctrlplanedev/cli
Length of output: 42
🏁 Script executed:
rg -A 10 "func.*UpsertResource" --type goRepository: ctrlplanedev/cli
Length of output: 1589
🏁 Script executed:
sed -n '1,150p' pkg/resourceprovider/resourceprovider.go | tail -100Repository: ctrlplanedev/cli
Length of output: 2794
🏁 Script executed:
rg -A 20 "func (r \*ResourceProvider) UpsertResource" pkg/resourceprovider/resourceprovider.goRepository: ctrlplanedev/cli
Length of output: 42
🏁 Script executed:
rg -B 5 -A 15 "SetResourceProviderResources" pkg/resourceprovider/resourceprovider.goRepository: ctrlplanedev/cli
Length of output: 958
🏁 Script executed:
rg "SetResourceProviderResources" --type goRepository: ctrlplanedev/cli
Length of output: 6003
🏁 Script executed:
rg -B 2 -A 5 "SetResourceProviderResourcesWithResponse" internal/api/providers/resource.go cmd/ctrlc/root/sync/salesforce/common/util.goRepository: ctrlplanedev/cli
Length of output: 1305
🏁 Script executed:
sed -n '1,50p' internal/common/common.goRepository: ctrlplanedev/cli
Length of output: 1110
Check HTTP status code before returning nil.
SetResourceProviderResources returns (*http.Response, error) without validating status codes. Non-2xx responses won't set err, so UpsertResource returns them unchanged. The caller logs the status but always returns nil, causing 4xx/5xx responses to silently succeed.
Other code in the repo (e.g., internal/api/providers/resource.go) validates status codes explicitly: if resp.StatusCode() != http.StatusAccepted { return fmt.Errorf(...) }. Apply the same validation in UpsertResource.
🤖 Prompt for AI Agents
In `@internal/common/common.go` around lines 32 - 38, The UpsertResource caller
currently ignores non-2xx HTTP responses: after calling
SetResourceProviderResources and getting upsertResp, check the HTTP status code
on upsertResp (e.g., upsertResp.StatusCode or upsertResp.Status) and return an
error for unexpected codes (match the existing pattern used in
internal/api/providers/resource.go — for example if resp.StatusCode() !=
http.StatusAccepted { return fmt.Errorf("upsert resources failed: status %d",
resp.StatusCode()) }); keep the logging of the response but ensure non-success
status codes cause UpsertResource to return a descriptive error instead of nil.
1 participant
Summary by CodeRabbit