feat: add get/delete subcommand for interacting with custom resources.#69
Merged
feat: add get/delete subcommand for interacting with custom resources.#69
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces resource management commands ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Get Resource Command
participant Service as APIResourceService
participant API as API Client
User->>CLI: get resource <identifier>
CLI->>Service: NewAPIResourceService(viper config)
Service->>API: Create authenticated client
Service->>API: Resolve workspace ID
API-->>Service: workspace ID
CLI->>Service: GetByIdentifier(ctx, identifier)
Service->>API: GetResourceByIdentifierWithResponse()
API-->>Service: Resource data (JSON200)
Service-->>CLI: *api.Resource
CLI->>CLI: HandleAnyOutput(format: json/yaml)
CLI-->>User: Serialized resource
sequenceDiagram
actor User
participant CLI as Delete Resource Command
participant Prompt as ConfirmAction
participant Service as APIResourceService
participant API as API Client
User->>CLI: delete resource <identifier>
CLI->>Service: NewAPIResourceService(viper config)
Service->>API: Create authenticated client
Service->>API: Resolve workspace ID
API-->>Service: workspace ID
CLI->>Service: GetByIdentifier(ctx, identifier)
Service->>API: GetResourceByIdentifierWithResponse()
API-->>Service: Resource data
Service-->>CLI: *api.Resource
alt auto-accept flag not set
CLI->>Prompt: ConfirmAction("Delete resource?")
Prompt-->>User: Display confirmation dialog
User-->>Prompt: User choice
Prompt-->>CLI: bool (confirmed/not)
alt not confirmed
CLI-->>User: Aborted.
end
end
CLI->>Service: DeleteByIdentifier(ctx, identifier)
Service->>API: RequestResourceDeletionByIdentifierWithResponse()
API-->>Service: ResourceRequestAccepted (202 or 200)
Service-->>CLI: *api.ResourceRequestAccepted
CLI-->>User: Success (or silent mode)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
cmd/ctrlc/root/get/resources/resources.go (2)
3-7: Unused import:net/url.If the double-encoding fix is applied (removing
url.QueryEscape), thenet/urlimport will become unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/get/resources/resources.go` around lines 3 - 7, The import list in resources.go includes an unused net/url package; remove "net/url" from the imports in cmd/ctrlc/root/get/resources/resources.go and ensure any previous references to url.QueryEscape were removed as part of the double-encoding fix (search for QueryEscape and delete or replace usages) so the import is no longer required.
75-75: Consider using the interface type for better testability.
handleJQaccepts a concrete*resources.APIResourceServiceinstead of theresources.ResourceServiceinterface. Using the interface would allow easier unit testing with mocks.Proposed change
-func handleJQ(cmd *cobra.Command, svc *resources.APIResourceService, jqExpr string, autoAccept bool, output string) error { +func handleJQ(cmd *cobra.Command, svc resources.ResourceService, jqExpr string, autoAccept bool, output string) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/get/resources/resources.go` at line 75, Change handleJQ to accept the ResourceService interface instead of the concrete *resources.APIResourceService so callers and unit tests can pass mocks; update the signature func handleJQ(cmd *cobra.Command, svc resources.ResourceService, jqExpr string, autoAccept bool, output string) error and adjust any callsites that pass *resources.APIResourceService (e.g., where handleJQ is invoked) to pass the value as the interface; ensure any method calls inside handleJQ use the interface methods and run tests to verify no concrete-specific members are referenced.cmd/ctrlc/root/delete/resource/resource.go (1)
41-45: Consider output timing for silent mode.When
outputis not"silent", the resource details are displayed before the confirmation prompt. This is reasonable for showing what will be deleted. However, consider whether this might confuse users who see output before being asked to confirm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/delete/resource/resource.go` around lines 41 - 45, The current code calls cliutil.HandleAnyOutput(cmd, resource, output) before prompting for confirmation which can show details prematurely; move the call so that when output != "silent" you invoke cliutil.HandleAnyOutput(cmd, resource, output) only after the user has confirmed the deletion (i.e., after the confirmation prompt/handler returns true), and keep output == "silent" behavior unchanged; update the block around the existing call to defer display until post-confirmation while preserving cmd, resource, and output usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ctrlc/root/delete/resource/resource.go`:
- Around line 59-64: After a successful call to
svc.DeleteByIdentifier(cmd.Context(), identifier) the code currently returns nil
with no user feedback; update the block that handles the delete result to emit a
success acknowledgement when output is "silent" (the default) by
printing/logging a concise confirmation including the identifier (e.g., "deleted
<identifier>" or similar). Use the existing command/context printing utilities
available in this file (refer to cmd, output, and identifier) so the message is
shown to the user only on success and does not affect error handling.
In `@cmd/ctrlc/root/get/resources/resources.go`:
- Around line 51-55: The code is double-URL-encoding the CEL query by calling
url.QueryEscape() before passing it into svc.List(); remove the manual encoding
so the generated API client can handle encoding. In the block that builds celPtr
(variable cel and celPtr), stop using url.QueryEscape and instead set celPtr to
the address of cel (e.g., celPtr = &cel when cel != ""), remove the temporary
encoded variable, and also remove the unused net/url import if it becomes
unused; the call site to update is the cel/celPtr preparation before invoking
svc.List().
In `@internal/cliutil/output.go`:
- Around line 182-193: The switch on format currently treats any unknown value
as JSON; change it to explicitly handle "yaml" and "json" cases (use
yaml.Marshal for "yaml" and json.MarshalIndent for "json") and make the default
branch return an error like fmt.Errorf("unsupported output format: %s", format)
so typos (e.g. "yml") fail fast; update the code in internal/cliutil/output.go
around the switch handling the variable format and the output/error assignment
to implement this validation.
In `@internal/resources/service.go`:
- Around line 28-34: The code currently ignores workspace resolution failures by
not checking the result of client.GetWorkspaceID(ctx, workspace); update the
logic in the constructor that returns *APIResourceService to validate that the
returned workspaceID is not uuid.Nil (from the uuid package) and, if it is
uuid.Nil, return a descriptive error (e.g., "failed to resolve workspace ID for
workspace: <workspace>") instead of constructing APIResourceService with the
zero UUID; ensure the uuid package is imported and refer to
client.GetWorkspaceID, workspaceID, and APIResourceService when making this
change.
---
Nitpick comments:
In `@cmd/ctrlc/root/delete/resource/resource.go`:
- Around line 41-45: The current code calls cliutil.HandleAnyOutput(cmd,
resource, output) before prompting for confirmation which can show details
prematurely; move the call so that when output != "silent" you invoke
cliutil.HandleAnyOutput(cmd, resource, output) only after the user has confirmed
the deletion (i.e., after the confirmation prompt/handler returns true), and
keep output == "silent" behavior unchanged; update the block around the existing
call to defer display until post-confirmation while preserving cmd, resource,
and output usage.
In `@cmd/ctrlc/root/get/resources/resources.go`:
- Around line 3-7: The import list in resources.go includes an unused net/url
package; remove "net/url" from the imports in
cmd/ctrlc/root/get/resources/resources.go and ensure any previous references to
url.QueryEscape were removed as part of the double-encoding fix (search for
QueryEscape and delete or replace usages) so the import is no longer required.
- Line 75: Change handleJQ to accept the ResourceService interface instead of
the concrete *resources.APIResourceService so callers and unit tests can pass
mocks; update the signature func handleJQ(cmd *cobra.Command, svc
resources.ResourceService, jqExpr string, autoAccept bool, output string) error
and adjust any callsites that pass *resources.APIResourceService (e.g., where
handleJQ is invoked) to pass the value as the interface; ensure any method calls
inside handleJQ use the interface methods and run tests to verify no
concrete-specific members are referenced.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26e6308b-af79-4f4d-8c82-920f0534a529
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignorecmd/ctrlc/root/delete/delete.gocmd/ctrlc/root/delete/resource/resource.gocmd/ctrlc/root/get/get.gocmd/ctrlc/root/get/resource/resource.gocmd/ctrlc/root/get/resources/resources.gocmd/ctrlc/root/root.gogo.modinternal/cliutil/jq.gointernal/cliutil/output.gointernal/cliutil/prompt.gointernal/resources/interface.gointernal/resources/service.go
Comment on lines
+59
to
+64
| _, err = svc.DeleteByIdentifier(cmd.Context(), identifier) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil |
Contributor
There was a problem hiding this comment.
Missing success feedback after deletion.
When output is "silent" (the default), the user receives no confirmation that the deletion succeeded. After a destructive operation, users typically expect some acknowledgment.
Proposed fix to add success message
_, err = svc.DeleteByIdentifier(cmd.Context(), identifier)
if err != nil {
return err
}
+ fmt.Fprintf(cmd.OutOrStdout(), "Resource %q deleted successfully.\n", identifier)
return nil📝 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
| _, err = svc.DeleteByIdentifier(cmd.Context(), identifier) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| _, err = svc.DeleteByIdentifier(cmd.Context(), identifier) | |
| if err != nil { | |
| return err | |
| } | |
| fmt.Fprintf(cmd.OutOrStdout(), "Resource %q deleted successfully.\n", identifier) | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ctrlc/root/delete/resource/resource.go` around lines 59 - 64, After a
successful call to svc.DeleteByIdentifier(cmd.Context(), identifier) the code
currently returns nil with no user feedback; update the block that handles the
delete result to emit a success acknowledgement when output is "silent" (the
default) by printing/logging a concise confirmation including the identifier
(e.g., "deleted <identifier>" or similar). Use the existing command/context
printing utilities available in this file (refer to cmd, output, and identifier)
so the message is shown to the user only on success and does not affect error
handling.
Comment on lines
+28
to
+34
| workspaceID := client.GetWorkspaceID(ctx, workspace) | ||
| log.Debug("resolved workspace", "input", workspace, "workspaceID", workspaceID.String()) | ||
|
|
||
| return &APIResourceService{ | ||
| Client: client, | ||
| WorkspaceID: workspaceID.String(), | ||
| }, nil |
Contributor
There was a problem hiding this comment.
Workspace resolution failure is silently ignored.
client.GetWorkspaceID() returns uuid.Nil on failure (as shown in internal/api/client.go:23-39), but this error case isn't checked. If the workspace slug is invalid or the API call fails, WorkspaceID will be set to "00000000-0000-0000-0000-000000000000", causing all subsequent API calls to fail with confusing "not found" errors instead of a clear workspace resolution error.
Proposed fix to validate workspace resolution
workspaceID := client.GetWorkspaceID(ctx, workspace)
+ if workspaceID == uuid.Nil {
+ return nil, fmt.Errorf("failed to resolve workspace: %q", workspace)
+ }
log.Debug("resolved workspace", "input", workspace, "workspaceID", workspaceID.String())You'll also need to add the uuid import:
import (
"context"
"fmt"
"time"
"github.com/charmbracelet/log"
"github.com/ctrlplanedev/cli/internal/api"
+ "github.com/google/uuid"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/resources/service.go` around lines 28 - 34, The code currently
ignores workspace resolution failures by not checking the result of
client.GetWorkspaceID(ctx, workspace); update the logic in the constructor that
returns *APIResourceService to validate that the returned workspaceID is not
uuid.Nil (from the uuid package) and, if it is uuid.Nil, return a descriptive
error (e.g., "failed to resolve workspace ID for workspace: <workspace>")
instead of constructing APIResourceService with the zero UUID; ensure the uuid
package is imported and refer to client.GetWorkspaceID, workspaceID, and
APIResourceService when making this change.
…the api for deletes, additional logging
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/resources/service.go (1)
29-35:⚠️ Potential issue | 🔴 CriticalValidate workspace resolution before constructing the service.
Line 29 resolves the workspace but never checks for a zero UUID, so the constructor can return a service bound to
"00000000-0000-0000-0000-000000000000"and fail later with misleading API errors.Proposed fix
import ( "context" "encoding/json" "fmt" "time" "github.com/charmbracelet/log" "github.com/ctrlplanedev/cli/internal/api" + "github.com/google/uuid" ) @@ workspaceID := client.GetWorkspaceID(ctx, workspace) + if workspaceID == uuid.Nil { + return nil, fmt.Errorf("failed to resolve workspace ID for workspace: %q", workspace) + } log.Debug("resolved workspace", "input", workspace, "workspaceID", workspaceID.String())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/resources/service.go` around lines 29 - 35, The code constructs an APIResourceService using the workspaceID returned by client.GetWorkspaceID(ctx, workspace) without validating it; add a check after resolving workspaceID (the variable named workspaceID from GetWorkspaceID) to detect a zero/empty UUID (e.g., uuid.Nil / IsZero()) and return a descriptive error instead of proceeding, so APIResourceService is not created with "00000000-0000-0000-0000-000000000000". Ensure the error message clearly names the workspace input and that this validation happens before instantiating APIResourceService (the struct constructor referenced as APIResourceService).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/resources/service.go`:
- Around line 29-35: The code constructs an APIResourceService using the
workspaceID returned by client.GetWorkspaceID(ctx, workspace) without validating
it; add a check after resolving workspaceID (the variable named workspaceID from
GetWorkspaceID) to detect a zero/empty UUID (e.g., uuid.Nil / IsZero()) and
return a descriptive error instead of proceeding, so APIResourceService is not
created with "00000000-0000-0000-0000-000000000000". Ensure the error message
clearly names the workspace input and that this validation happens before
instantiating APIResourceService (the struct constructor referenced as
APIResourceService).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8384d675-0ae4-4e1b-9d75-1f647369fdb2
📒 Files selected for processing (4)
cmd/ctrlc/root/get/resources/resources.gointernal/api/client.gen.gointernal/resources/interface.gointernal/resources/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/resources/interface.go
- cmd/ctrlc/root/get/resources/resources.go
Collaborator
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/resources/service.go (1)
29-35:⚠️ Potential issue | 🔴 CriticalFail fast when workspace resolution returns nil UUID.
Line 29 resolves workspace ID but never validates it. If resolution fails, the service is created with
00000000-0000-0000-0000-000000000000, which causes misleading downstream API failures.Suggested patch
import ( "context" "encoding/json" "fmt" "time" "github.com/charmbracelet/log" "github.com/ctrlplanedev/cli/internal/api" + "github.com/google/uuid" ) @@ workspaceID := client.GetWorkspaceID(ctx, workspace) + if workspaceID == uuid.Nil { + return nil, fmt.Errorf("failed to resolve workspace ID for workspace: %q", workspace) + } log.Debug("resolved workspace", "input", workspace, "workspaceID", workspaceID.String())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/resources/service.go` around lines 29 - 35, The code calls client.GetWorkspaceID(ctx, workspace) and assigns workspaceID without validating it; update the logic in the constructor creating APIResourceService to detect a nil/zero UUID (workspaceID == uuid.Nil or workspaceID.IsZero()) after calling GetWorkspaceID and return an error instead of continuing; reference the workspaceID variable, the GetWorkspaceID call, and the APIResourceService{WorkspaceID: ...} creation so you add a guard that returns a clear error if resolution failed, preventing construction with "00000000-0000-0000-0000-000000000000".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 14-15: The Makefile defines a command target named update-openapi
but does not include it in the .PHONY list, which can cause make to skip the
recipe if a file named update-openapi exists; update the .PHONY declaration (the
.PHONY symbol) to include update-openapi so that the update-openapi target is
always treated as a phony command.
---
Duplicate comments:
In `@internal/resources/service.go`:
- Around line 29-35: The code calls client.GetWorkspaceID(ctx, workspace) and
assigns workspaceID without validating it; update the logic in the constructor
creating APIResourceService to detect a nil/zero UUID (workspaceID == uuid.Nil
or workspaceID.IsZero()) after calling GetWorkspaceID and return an error
instead of continuing; reference the workspaceID variable, the GetWorkspaceID
call, and the APIResourceService{WorkspaceID: ...} creation so you add a guard
that returns a clear error if resolution failed, preventing construction with
"00000000-0000-0000-0000-000000000000".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d157ab9-8bdc-447c-ade8-2f22c651d3aa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
Makefilecmd/ctrlc/root/get/resources/resources.gogo.modinternal/resources/interface.gointernal/resources/service.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- internal/resources/interface.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ctrlc/root/get/resources/resources.go
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 by CodeRabbit
New Features
Chores