Add object storage management commands and tests#6
Open
ditahkk wants to merge 3 commits into
Open
Conversation
- Implemented commands for managing Ceph object storage instances, including listing, getting, creating, deleting, and resizing storage. - Added subcommands for managing buckets and objects within storage instances. - Created comprehensive unit tests for object storage API interactions. - Updated default API URL to include the correct path for API requests.
📝 WalkthroughWalkthroughAdds an object-storage API client with types, tests, and full Cobra CLI commands (instances, buckets, objects). Aligns DefaultAPIURL to include /api and updates docs and installer URLs. Applies API model changes (billing, instance, kubernetes, project dashboard) and wires new CLI flags. ChangesAPI URL Path Fix
Object Storage Service and CLI
API surface & CLI flag updates
Sequence Diagram(s)sequenceDiagram
participant CLI as zcp CLI
participant Client as httpclient.Client
participant API as ZCP API (/object-storages)
CLI->>Client: POST /object-storages {create request JSON}
Client->>API: HTTP POST /object-storages
API-->>Client: 201 {envelope with object_storage}
Client-->>CLI: parsed ObjectStorage
CLI->>CLI: printer.PrintTable(instance summary)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (3)
internal/api/objectstorage/objectstorage_test.go (2)
86-93: ⚡ Quick winAssert HTTP method in read-path handlers.
These tests validate paths, but they should also assert
GETto catch accidental method regressions in client wiring.Also applies to: 210-217, 236-243
🤖 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 `@internal/api/objectstorage/objectstorage_test.go` around lines 86 - 93, The test HTTP handlers that validate read paths should also assert the request method is GET; update the anonymous handler passed to httptest.NewServer (the http.HandlerFunc that checks r.URL.Path == "/object-storages/my-storage-1") to first verify r.Method == "GET" and return http.Error or http.NotFound if it isn't, and do the same change for the other read-path test handlers in the file (the other httptest.NewServer handlers used for GET/read assertions) so any accidental method regressions in the client are caught.
50-305: ⚡ Quick winAdd tests for the remaining public service methods.
The suite currently skips
UpdateBucket,SetBucketACL,ListObjects, andGetObject, which are part of the new API surface. Please add parity tests for path/method/payload/response behavior for these methods too.🤖 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 `@internal/api/objectstorage/objectstorage_test.go` around lines 50 - 305, Tests are missing coverage for the new service methods UpdateBucket, SetBucketACL, ListObjects, and GetObject; add unit tests mirroring the existing patterns (e.g., TestCreateBucket/TestGetBucket) that register httptest servers, assert request URL path and HTTP method, decode and validate request bodies (where applicable), and encode appropriate JSON responses so svc.UpdateBucket, svc.SetBucketACL, svc.ListObjects, and svc.GetObject (on objectstorage.NewService with newTestClient) are exercised; for each test verify returned values (IDs, names, object lists) and that payload fields (e.g., ACL settings, object listing params) are sent correctly using the corresponding response structs and objectstorage types.internal/commands/objectstorage.go (1)
481-483: ⚡ Quick winValidate
--aclagainst the documented allowed values.The command currently accepts any non-empty ACL string; validating against the supported set would catch typos before making the API call.
Also applies to: 503-504
🤖 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 `@internal/commands/objectstorage.go` around lines 481 - 483, The ACL check currently only rejects empty strings (if acl == "") but must validate against the allowed set ("private", "public-read", "public-read-write", "authenticated-read"); update the validation in the command handler that reads the acl variable (the branch containing if acl == "") to compare acl against this whitelist and return a descriptive fmt.Errorf when acl is not one of the allowed values; apply the same whitelist validation for the other occurrence around the acl handling at the other noted location.
🤖 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 `@internal/commands/objectstorage.go`:
- Around line 151-159: The validation currently allows negative --storage-gb
values to bypass plan checks; add an explicit check for storageGB < 0 and return
a clear error (e.g. "--storage-gb cannot be negative") before the existing
mutually-exclusive/minimum checks, referencing the storageGB and
minObjectStorageGB variables in the create validation block; apply the same
negative-value check to the other validation block around the storageGB checks
(the block at the later occurrence) so both create and update code paths reject
negative storage sizes.
---
Nitpick comments:
In `@internal/api/objectstorage/objectstorage_test.go`:
- Around line 86-93: The test HTTP handlers that validate read paths should also
assert the request method is GET; update the anonymous handler passed to
httptest.NewServer (the http.HandlerFunc that checks r.URL.Path ==
"/object-storages/my-storage-1") to first verify r.Method == "GET" and return
http.Error or http.NotFound if it isn't, and do the same change for the other
read-path test handlers in the file (the other httptest.NewServer handlers used
for GET/read assertions) so any accidental method regressions in the client are
caught.
- Around line 50-305: Tests are missing coverage for the new service methods
UpdateBucket, SetBucketACL, ListObjects, and GetObject; add unit tests mirroring
the existing patterns (e.g., TestCreateBucket/TestGetBucket) that register
httptest servers, assert request URL path and HTTP method, decode and validate
request bodies (where applicable), and encode appropriate JSON responses so
svc.UpdateBucket, svc.SetBucketACL, svc.ListObjects, and svc.GetObject (on
objectstorage.NewService with newTestClient) are exercised; for each test verify
returned values (IDs, names, object lists) and that payload fields (e.g., ACL
settings, object listing params) are sent correctly using the corresponding
response structs and objectstorage types.
In `@internal/commands/objectstorage.go`:
- Around line 481-483: The ACL check currently only rejects empty strings (if
acl == "") but must validate against the allowed set ("private", "public-read",
"public-read-write", "authenticated-read"); update the validation in the command
handler that reads the acl variable (the branch containing if acl == "") to
compare acl against this whitelist and return a descriptive fmt.Errorf when acl
is not one of the allowed values; apply the same whitelist validation for the
other occurrence around the acl handling at the other noted location.
🪄 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: 07118169-a430-47b5-94eb-487b9f0415ed
📒 Files selected for processing (9)
README.mdRELEASE_NOTES.mdcmd/zcp/root/root.godocs/command-taxonomy.mddocs/configuration.mdinternal/api/objectstorage/objectstorage.gointernal/api/objectstorage/objectstorage_test.gointernal/commands/objectstorage.gointernal/config/config.go
Comment on lines
+151
to
+159
| if plan == "" && storageGB == 0 { | ||
| return fmt.Errorf("either --plan or --storage-gb is required") | ||
| } | ||
| if plan != "" && storageGB != 0 { | ||
| return fmt.Errorf("--plan and --storage-gb are mutually exclusive") | ||
| } | ||
| if storageGB > 0 && storageGB < minObjectStorageGB { | ||
| return fmt.Errorf("--storage-gb must be at least %d", minObjectStorageGB) | ||
| } |
There was a problem hiding this comment.
Block negative --storage-gb in create validation.
A negative value bypasses the current plan/storage checks and can send a request with neither valid plan nor custom_plan. This is a correctness bug in argument validation.
Suggested fix
+ if storageGB < 0 {
+ return fmt.Errorf("--storage-gb must be a positive integer")
+ }
if plan == "" && storageGB == 0 {
return fmt.Errorf("either --plan or --storage-gb is required")
}Also applies to: 179-181
🤖 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 `@internal/commands/objectstorage.go` around lines 151 - 159, The validation
currently allows negative --storage-gb values to bypass plan checks; add an
explicit check for storageGB < 0 and return a clear error (e.g. "--storage-gb
cannot be negative") before the existing mutually-exclusive/minimum checks,
referencing the storageGB and minObjectStorageGB variables in the create
validation block; apply the same negative-value check to the other validation
block around the storageGB checks (the block at the later occurrence) so both
create and update code paths reject negative storage sizes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/commands/kubernetes.go (1)
106-129:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCloud provider setup is labeled required but not validated
Line 172 says
--cloud-provider-setupis required, but there is no corresponding check in the required-flag validation block (Line 106-129). Please either enforce it or make the help text optional to avoid misleading users.Suggested minimal fix (help text alignment)
- cmd.Flags().StringVar(&cloudProviderSetup, "cloud-provider-setup", "", "Cloud provider setup slug, e.g. zcp-apc (required for quota resolution)") + cmd.Flags().StringVar(&cloudProviderSetup, "cloud-provider-setup", "", "Cloud provider setup slug, e.g. zcp-apc (optional)")Also applies to: 172-172
🤖 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 `@internal/commands/kubernetes.go` around lines 106 - 129, The help text claims --cloud-provider-setup is required but there is no runtime check; add validation in the same required-flag block (the one that checks name, version, plan, cloudProvider, region, project, billingCycle) to enforce it by validating the cloudProviderSetup variable (e.g., resolve or check cloudProviderSetup and return fmt.Errorf("--cloud-provider-setup is required") if empty), or alternatively update the CLI help/usage text to remove the "required" wording for --cloud-provider-setup so the documentation matches the current behavior.
🧹 Nitpick comments (1)
internal/commands/project.go (1)
258-262: ⚡ Quick winStabilize dashboard row ordering for deterministic CLI output.
Line 260 ranges a Go map directly, so output order is non-deterministic. Sort service names before building rows.
Proposed fix
import ( "context" "fmt" + "sort" "strconv" "time" @@ headers := []string{"SERVICE", "COUNT"} rows := make([][]string, 0, len(counts)) - for name, count := range counts { - rows = append(rows, []string{name, strconv.Itoa(count)}) - } + names := make([]string, 0, len(counts)) + for name := range counts { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + rows = append(rows, []string{name, strconv.Itoa(counts[name])}) + }🤖 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 `@internal/commands/project.go` around lines 258 - 262, The loop over the map variable counts produces non-deterministic row ordering; to fix, collect the map keys into a slice (e.g., serviceNames), sort them with sort.Strings, then iterate over the sorted serviceNames to build rows (the variables headers, rows and counts are the relevant identifiers) so CLI output is deterministic.
🤖 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 `@internal/api/project/project.go`:
- Around line 135-146: The Dashboard method on Service was changed to return
DashboardCounts (a map) but tests/callers still expect the old slice type
(project.DashboardService) and use slice-style indexing; update those tests and
any callers to use the new map contract: replace references to
project.DashboardService and any slice loops/index access with lookups like
counts["serviceName"] (using the DashboardCounts type returned by
Service.Dashboard), and adjust assertions to check map key presence and values
accordingly (e.g., value, ok := counts["svc"]; compare value). Ensure
project_test.go and any other callers import/expect DashboardCounts and perform
map-based assertions instead of slice-based ones.
In `@internal/commands/billing.go`:
- Around line 636-637: The flag currently defaults to true and the code always
sends a non-nil pointer for billing.CancelServiceRequest.DeletePublicIP, causing
implicit deletion; change the flag handling so DeletePublicIP is only set when
the user explicitly provides the flag. Keep the existing deletePublicIP bool
flag on the Cobra command, set its default to false, and when building the
request in runBillingCancelService check
cmd.Flags().Changed("delete-public-ip"); only if Changed(...) is true allocate a
bool pointer (e.g. bp := deletePublicIP; req.DeletePublicIP = &bp) otherwise
leave req.DeletePublicIP nil so the field is omitted from the JSON.
---
Outside diff comments:
In `@internal/commands/kubernetes.go`:
- Around line 106-129: The help text claims --cloud-provider-setup is required
but there is no runtime check; add validation in the same required-flag block
(the one that checks name, version, plan, cloudProvider, region, project,
billingCycle) to enforce it by validating the cloudProviderSetup variable (e.g.,
resolve or check cloudProviderSetup and return
fmt.Errorf("--cloud-provider-setup is required") if empty), or alternatively
update the CLI help/usage text to remove the "required" wording for
--cloud-provider-setup so the documentation matches the current behavior.
---
Nitpick comments:
In `@internal/commands/project.go`:
- Around line 258-262: The loop over the map variable counts produces
non-deterministic row ordering; to fix, collect the map keys into a slice (e.g.,
serviceNames), sort them with sort.Strings, then iterate over the sorted
serviceNames to build rows (the variables headers, rows and counts are the
relevant identifiers) so CLI output is deterministic.
🪄 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: 2042a49e-a381-4b94-93a0-b59bcffd259e
📒 Files selected for processing (9)
go.modinternal/api/billing/billing.gointernal/api/instance/instance.gointernal/api/kubernetes/kubernetes.gointernal/api/project/project.gointernal/commands/billing.gointernal/commands/instance.gointernal/commands/kubernetes.gointernal/commands/project.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
Comment on lines
+135
to
146
| // Dashboard returns a map of service name → count for a project's dashboard. | ||
| func (s *Service) Dashboard(ctx context.Context, slug string) (DashboardCounts, error) { | ||
| var raw json.RawMessage | ||
| if err := s.client.Get(ctx, "/projects/dashboard/"+slug+"/services", nil, &raw); err != nil { | ||
| return nil, fmt.Errorf("getting project dashboard %s: %w", slug, err) | ||
| } | ||
| var services []DashboardService | ||
| if err := decode(raw, &services); err != nil { | ||
| var counts DashboardCounts | ||
| if err := decode(raw, &counts); err != nil { | ||
| return nil, fmt.Errorf("decoding dashboard services: %w", err) | ||
| } | ||
| return services, nil | ||
| return counts, nil | ||
| } |
There was a problem hiding this comment.
Update dashboard tests/callers to the new map contract before merge.
Line 136 changes Dashboard from a slice model to DashboardCounts, but typecheck is currently broken in internal/api/project/project_test.go (undefined: project.DashboardService, and slice-style access on a map). This is a merge blocker until those assertions are migrated to map-based checks.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 142-142: : # github.com/zsoftly/zcp-cli/internal/api/project_test [github.com/zsoftly/zcp-cli/internal/api/project.test]
internal/api/project/project_test.go:142:24: undefined: project.DashboardService
internal/api/project/project_test.go:164:14: cannot use 0 (untyped int constant) as string value in map index
internal/api/project/project_test.go:164:17: services[0].Name undefined (type int has no field or method Name)
internal/api/project/project_test.go:165:55: cannot use 0 (untyped int constant) as string value in map index
internal/api/project/project_test.go:165:58: services[0].Name undefined (type int has no field or method Name)
(typecheck)
🤖 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 `@internal/api/project/project.go` around lines 135 - 146, The Dashboard method
on Service was changed to return DashboardCounts (a map) but tests/callers still
expect the old slice type (project.DashboardService) and use slice-style
indexing; update those tests and any callers to use the new map contract:
replace references to project.DashboardService and any slice loops/index access
with lookups like counts["serviceName"] (using the DashboardCounts type returned
by Service.Dashboard), and adjust assertions to check map key presence and
values accordingly (e.g., value, ok := counts["svc"]; compare value). Ensure
project_test.go and any other callers import/expect DashboardCounts and perform
map-based assertions instead of slice-based ones.
Comment on lines
+636
to
637
| var deletePublicIP bool | ||
| cmd := &cobra.Command{ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate billing.go and show relevant line ranges around the reported lines.
FILE="internal/commands/billing.go"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- billing.go lines 600-720 ----"
nl -ba "$FILE" | sed -n '600,720p'
echo "---- billing.go occurrences of delete-public-ip ----"
rg -n --fixed-string "delete-public-ip" "$FILE" || true
echo "---- billing.go occurrences of DeletePublicIP ----"
rg -n --fixed-string "DeletePublicIP" "$FILE" || true
# Search for CancelServiceRequest definition in repo (limit to likely directories).
echo "---- Search for CancelServiceRequest struct ----"
rg -n "type[[:space:]]+CancelServiceRequest" . || true
echo "---- Search for DeletePublicIP field in request model ----"
rg -n "DeletePublicIP" . | head -n 50Repository: zsoftly/zcp-cli
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/commands/billing.go"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- billing.go lines 600-720 (raw) ----"
sed -n '600,720p' "$FILE"
echo "---- billing.go occurrences of delete-public-ip ----"
rg -n --fixed-string "delete-public-ip" "$FILE" || true
echo "---- billing.go occurrences of DeletePublicIP ----"
rg -n --fixed-string "DeletePublicIP" "$FILE" || true
echo "---- Search for CancelServiceRequest definition ----"
rg -n "type[[:space:]]+CancelServiceRequest" . || true
echo "---- Search for DeletePublicIP field in request model ----"
rg -n "DeletePublicIP" . | head -n 50Repository: zsoftly/zcp-cli
Length of output: 5680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show billing request model around CancelServiceRequest
FILE="internal/api/billing/billing.go"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
echo "---- internal/api/billing/billing.go lines 320-390 ----"
sed -n '320,390p' "$FILE"
echo "---- Find CancelService method ----"
rg -n "func \(.*\) CancelService\(" internal/api/billing/billing.go || true
echo "---- Show area around CancelService method ----"
# Try to capture a window around the match
MATCH_LINE=$(rg -n "func \(.*\) CancelService\(" internal/api/billing/billing.go | head -n1 | cut -d: -f1 || true)
if [ -n "${MATCH_LINE:-}" ]; then
START=$((MATCH_LINE-30))
END=$((MATCH_LINE+120))
sed -n "${START},${END}p" "$FILE"
fi
echo "---- Search DeletePublicIP usage in api billing ----"
rg -n "DeletePublicIP" internal/api/billing/billing.go || true
echo "---- Re-run searches in commands/billing.go without --fixed-string ----"
CMD_FILE="internal/commands/billing.go"
rg -n "delete-public-ip" "$CMD_FILE" || true
rg -n "DeletePublicIP" "$CMD_FILE" || trueRepository: zsoftly/zcp-cli
Length of output: 6979
Fix delete-public-ip to not default to destructive behavior
internal/commands/billing.go sets the Cobra --delete-public-ip flag default to true and runBillingCancelService always populates billing.CancelServiceRequest.DeletePublicIP via DeletePublicIP: &deletePublicIP. Since the request model defines DeletePublicIP *bool \json:"delete_public_ip,omitempty"`, passing a non-nil pointer means delete_public_ipis always sent in the JSON body (withtrue` by default), turning an optional behavior into an implicit deletion default. Change construction so the field is omitted unless the user explicitly sets the flag.
🤖 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 `@internal/commands/billing.go` around lines 636 - 637, The flag currently
defaults to true and the code always sends a non-nil pointer for
billing.CancelServiceRequest.DeletePublicIP, causing implicit deletion; change
the flag handling so DeletePublicIP is only set when the user explicitly
provides the flag. Keep the existing deletePublicIP bool flag on the Cobra
command, set its default to false, and when building the request in
runBillingCancelService check cmd.Flags().Changed("delete-public-ip"); only if
Changed(...) is true allocate a bool pointer (e.g. bp := deletePublicIP;
req.DeletePublicIP = &bp) otherwise leave req.DeletePublicIP nil so the field is
omitted from the JSON.
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
Documentation
Tests
Chores