Add object storage management commands and tests#6
Open
ditahkk wants to merge 12 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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an object-storage API client and S3 data-plane methods, a new ChangesSingle cohesive change set
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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
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
- Introduced `lib.sh` to provide shared functions for running smoke tests, including result tracking, command execution, and resource cleanup. - Created `smoke.sh` to execute live smoke tests against the ZCP API, with options for lifecycle testing and service selection. - Implemented logging and error handling for better visibility during test execution. - Added environment variable checks and usage instructions for running the smoke tests.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
internal/api/network/network_test.go (1)
277-281: ⚡ Quick winAssert
/networkspath in the new compatibility tests.These handlers currently return payloads for any path, so endpoint regressions in
Service.Listcould still pass unnoticed.Suggested fix pattern (apply to each new test server handler)
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/networks" { + http.NotFound(w, r) + return + } w.Header().Set("Content-Type", "application/json") fmt.Fprint(w, payload) }))Also applies to: 304-308, 328-332
🤖 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/network/network_test.go` around lines 277 - 281, The test HTTP handlers in internal/api/network/network_test.go currently return the same payload for any request path, which can hide endpoint regressions in Service.List; update each httptest.NewServer handler (the anonymous http.HandlerFunc used in the tests around lines creating srv := httptest.NewServer(...)) to assert the incoming request path (e.g., check r.URL.Path == "/networks") and fail the test (t.Fatalf or t.Error) if it differs, optionally also assert the HTTP method, so the handler only returns the payload for the intended /networks endpoint used by Service.List.
🤖 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 @.github/workflows/smoke.yml:
- Line 16: Add an explicit least-privilege permissions block to the workflow by
adding a top-level permissions: mapping (targeting only the scopes this pipeline
actually needs, e.g., contents: read, packages: read, id-token: write, checks:
write — adjust to exact usages) instead of relying on defaults; also add
job-level permissions overrides for any jobs that require broader access so you
don’t grant more than necessary (apply to the jobs referenced later in the
file), ensuring each job uses the narrowest required scopes.
- Around line 59-61: Update the workflow to harden action usage: replace both
occurrences of actions/checkout@v4 with SHA-pinned versions and add with:
persist-credentials: false to both checkout steps (so subsequent git commands
won’t inherit runner credentials), and replace actions/setup-go@v5 with a
commit-SHA pinned ref; locate the two checkout steps and the setup-go step by
their action references (actions/checkout@v4 and actions/setup-go@v5) and change
them to pinned SHAs and disable credential persistence on checkout.
In `@internal/api/network/network.go`:
- Around line 82-84: The code silently drops json.Unmarshal errors when decoding
typeRaw into n.NetworkType; change the unchecked call to capture and handle the
error (e.g., err := json.Unmarshal(typeRaw, &n.NetworkType)) and return or
propagate a descriptive error instead of ignoring it so malformed
`type`/`network_type` payloads fail fast; update the surrounding function (where
typeRaw and n.NetworkType are used) to check the err and return an appropriate
error (including context like "decoding network type") rather than discarding
it.
In `@internal/api/plan/plan.go`:
- Around line 18-21: FlexNumber.UnmarshalJSON currently accepts any JSON token;
change it to only allow JSON null, numeric tokens, or quoted numeric strings and
return an error for everything else: inspect the raw bytes in UnmarshalJSON
(FlexNumber.UnmarshalJSON), handle "null" by leaving value zero, if the first
non-space char is a quote unquote and validate the inner string parses as a
number, if it's a digit/- then parse as a number, and on any other token
(objects, arrays, true/false, non-numeric strings) return a descriptive error;
then add negative tests alongside TestNetworkRateAsNumber that POST/parse
network_rate with tokens like {}, [], true and a non-numeric string and assert
UnmarshalJSON (or the unmarshaled struct) fails.
In `@internal/commands/instance.go`:
- Around line 195-197: Ensure we validate explicitly-provided custom-plan flags
(cpu, memory, disk and other custom plan fields used when building custom_plan)
and fail fast on non-positive values: for each flag (e.g., the cpu, memory, disk
variables and the other fields referenced around the custom_plan construction),
detect whether the user explicitly set the flag and if so return an error if its
value <= 0 instead of omitting the field; update the flag-parsing/validation
logic in the command handler that builds custom_plan to check "flag was set" +
value > 0 and produce a clear validation error (e.g., "invalid value for --cpu:
must be > 0") when violated.
In `@tests/smoke/cases.sh`:
- Around line 143-145: The teardown uses a fragile fallback that assigns FX_IP
from a broad zcp ip list query and then always defers cancel(FX_IP), which can
delete unrelated pre-existing resources; update the logic so defer cancel is
only registered when the IP slug was obtained from the immediate create output
(i.e., when the create/creation parsing succeeded) rather than from the fallback
zcp query—specifically change the FX_IP assignment and subsequent [[ -n "$FX_IP"
]] defer cancel usage so the defer is conditional on a successful create parse
flag or the exact create response variable, and remove or guard the fallback zcp
ip list -> network_slug path from registering teardown; apply this same fix
pattern to the other similar teardown sites that use the fallback-list+defer
pattern.
- Around line 390-400: The current kubernetes create block relies on output text
to decide success; capture the create command's exit code and use it first to
decide failure/success: after running out="$(zcp kubernetes create ... 2>&1)"
save the exit status (e.g., rc=$?), treat non-zero rc as a failure by calling
_mark_fail "kubernetes create" and printing the first lines of $out, then only
if rc==0 continue to check for quota message (grep -qiE 'quota not found'
<<<"$out") and otherwise mark success with _mark_pass and defer cancel using the
slug found by zcp kubernetes list; update this logic around the existing out
assignment and subsequent if/elif/else that reference _mark_fail, _mark_pass,
and defer cancel.
In `@tests/smoke/lib.sh`:
- Around line 80-86: capture() currently merges stderr into the captured output
(stdout+stderr) which breaks JSON parsing; change it so __out captures only
stdout and capture stderr separately (e.g., populate __err by redirecting stdout
away: run the command with 2>&1 >/dev/null to collect stderr into __err, and run
normally to collect stdout into __out), preserve the exit code in __rc, and keep
printing only __out into the caller variable (printf -v "$__var" '%s' "$__out");
refer to the capture() function and variables __out, __err and __rc when making
the change.
In `@tests/smoke/README.md`:
- Around line 69-81: Replace the two plain fenced code blocks in README.md with
language-tagged fences to satisfy markdown linting: change the
environment-variable block starting with the ZCP_SMOKE_* lines to use ```text
and likewise change the file-tree block that begins with "tests/smoke/" (the
block listing smoke.sh, lib.sh, cases.sh, affected.sh, README.md) to use
```text; ensure both opening fences include the language token and keep the
inner content unchanged.
In `@tests/smoke/smoke.sh`:
- Around line 49-50: The CLI flag --read-only currently gets overwritten by the
environment variable ZCP_SMOKE_LIFECYCLE; change parsing so explicit flags take
precedence: when handling the --read-only/--lifecycle options set MODE_LIFECYCLE
(and mark a marker like LIFECYCLE_SET=1) and then when you later read
ZCP_SMOKE_LIFECYCLE only assign MODE_LIFECYCLE if LIFECYCLE_SET is empty (i.e.,
not set by a command-line option). Update the parsing logic around the
--read-only case and the code that reads ZCP_SMOKE_LIFECYCLE (also applied at
the similar block at lines 58-60) to respect the marker so the explicit
--read-only flag cannot be overridden by the env var.
- Around line 62-69: Add a fast preflight check for the ssh-keygen binary when
lifecycle mode is enabled: after the existing require_jq/require_curl checks and
before using ZCP_BIN, detect the lifecycle mode env/flag your tests use (e.g.
the lifecycle-related variable in this script) and if enabled ensure command -v
ssh-keygen >/dev/null 2>&1 (or [[ -x "$(command -v ssh-keygen)" ]]) succeeds; if
not, print a clear error (similar style to the ZCP_BEARER_TOKEN message) and
exit with non-zero so missing ssh-keygen fails fast during setup.
---
Nitpick comments:
In `@internal/api/network/network_test.go`:
- Around line 277-281: The test HTTP handlers in
internal/api/network/network_test.go currently return the same payload for any
request path, which can hide endpoint regressions in Service.List; update each
httptest.NewServer handler (the anonymous http.HandlerFunc used in the tests
around lines creating srv := httptest.NewServer(...)) to assert the incoming
request path (e.g., check r.URL.Path == "/networks") and fail the test (t.Fatalf
or t.Error) if it differs, optionally also assert the HTTP method, so the
handler only returns the payload for the intended /networks endpoint used by
Service.List.
🪄 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: ed78e639-5180-462a-bba5-560f7f85cf9f
📒 Files selected for processing (24)
.github/workflows/smoke.ymlinternal/api/billing/billing.gointernal/api/billing/billing_test.gointernal/api/dns/dns.gointernal/api/dns/dns_test.gointernal/api/iso/iso.gointernal/api/iso/iso_test.gointernal/api/network/network.gointernal/api/network/network_test.gointernal/api/plan/plan.gointernal/api/plan/plan_test.gointernal/api/portforward/portforward.gointernal/api/portforward/portforward_test.gointernal/commands/billing.gointernal/commands/dns.gointernal/commands/instance.gointernal/commands/network.gointernal/commands/plan.gointernal/commands/portforward.gotests/smoke/README.mdtests/smoke/affected.shtests/smoke/cases.shtests/smoke/lib.shtests/smoke/smoke.sh
✅ Files skipped from review due to trivial changes (1)
- internal/commands/dns.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/commands/billing.go
… capabilities - Introduced new methods for uploading (PutObject) and deleting (DeleteObject) objects in the object storage service. - Updated Object struct to include Permission and modified Size to be a string. - Enhanced ListObjects to handle pagination and return a complete list of objects. - Added IsPublic method to Object for permission checks. - Updated API responses and tests to reflect changes in object structure and functionality. - Implemented new CLI commands for managing objects, including listing, uploading, and deleting. - Added comprehensive tests for new S3 client and object management functionalities.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/commands/objectstorage.go (2)
669-677: ⚡ Quick winUse consistent confirmation input pattern.
Object delete confirmation (lines 669-677) uses
fmt.Scanlnwhile storage delete (line 267) and bucket delete (line 485) usebufio.Scanner. Althoughfmt.Scanlnworks here, the inconsistency makes the codebase harder to maintain and reason about. Preferbufio.Scannerfor consistency and robustness with multi-word or whitespace-padded input.♻️ Align with existing pattern
if !autoApproved(cmd) { - fmt.Fprintf(os.Stdout, "Delete object %q from bucket %q? [y/N]: ", objectKey, bucketSlug) - var answer string - fmt.Scanln(&answer) - if strings.ToLower(strings.TrimSpace(answer)) != "y" { - fmt.Fprintln(os.Stdout, "Aborted.") + fmt.Fprintf(os.Stderr, "Delete object %q from bucket %q? [y/N]: ", objectKey, bucketSlug) + scanner := bufio.NewScanner(os.Stdin) + scanner.Scan() + answer := strings.TrimSpace(strings.ToLower(scanner.Text())) + if answer != "y" && answer != "yes" { + fmt.Fprintln(os.Stderr, "Aborted.") return nil } }(Also moves prompt to stderr to match the other delete commands.)
🤖 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 669 - 677, Replace the fmt.Scanln confirmation in the object delete flow with the same bufio.Scanner pattern used by the other delete commands: write the prompt to os.Stderr instead of os.Stdout, create a bufio.NewScanner(os.Stdin), call scanner.Scan() and use strings.ToLower(strings.TrimSpace(scanner.Text())) to check for "y", and preserve the existing early return and "Aborted." output if the answer is not "y"; update the block that references autoApproved(cmd), objectKey and bucketSlug accordingly so the behavior matches storage/bucket delete routines.
303-303: ⚡ Quick winSimplify redundant validation condition.
The condition
storageGB <= 0 || storageGB < minObjectStorageGBis redundant becauseminObjectStorageGBis 60, which is greater than 0. The first check is subsumed by the second.♻️ Simplified condition
- if storageGB <= 0 || storageGB < minObjectStorageGB { + if storageGB < minObjectStorageGB { return fmt.Errorf("--storage-gb must be at least %d", minObjectStorageGB) }🤖 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` at line 303, The validation condition in internal/commands/objectstorage.go redundantly checks "storageGB <= 0 || storageGB < minObjectStorageGB"; since minObjectStorageGB is 60 (>0), remove the first clause and replace the condition with "storageGB < minObjectStorageGB" so the check uses only storageGB and minObjectStorageGB (referencing the storageGB variable and minObjectStorageGB constant) to validate the value.internal/api/objectstorage/objectstorage.go (1)
379-393: ⚖️ Poor tradeoffGetObject inefficiency is documented but consider caching.
The implementation fetches all objects and filters client-side because the API's GET endpoint returns only the key string. For buckets with thousands of objects this will be slow. While the comment explains the limitation, consider whether the CLI should cache the object list for a short TTL when multiple
getoperations occur in quick succession, or document the performance trade-off in user-facing help text.🤖 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.go` around lines 379 - 393, GetObject is inefficient for large buckets because it always calls ListObjects; add a short in-memory TTL cache on Service (e.g., map keyed by slug+bucketSlug + timestamp protected by a mutex) and change Service.GetObject to consult the cache first, populate it when calling ListObjects, and honor the TTL to avoid stale data; also ensure writes that modify bucket contents (upload/delete methods) invalidate the corresponding cache entry. Use the existing Service type and the ListObjects and GetObject method names to locate where to add the cache fields and invalidation logic.
🤖 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/objectstorage/objectstorage.go`:
- Around line 398-419: In PutObject, validate that the localPath file exists and
is readable before calling mc.FPutObject: use os.Stat (or try os.Open) on
localPath inside the Service.PutObject function to detect missing or
permission-denied errors and return a clear, early error (e.g., "local file not
found or not readable: <localPath>") instead of relying on mc.FPutObject; keep
the rest of the logic (objectKey defaulting, PutObjectOptions, calling
mc.FPutObject, and returning info.Size) unchanged.
In `@internal/api/objectstorage/s3client.go`:
- Around line 31-34: The current fallback that sets host := u.Host; if host ==
"" { host = u.Path } silently accepts endpoint strings without a URL scheme;
update the parsing logic around url.Parse and the variables u and host to
validate that the parsed URL includes a non-empty Scheme (either "http" or
"https") and return an explicit error if Scheme is empty or invalid instead of
using u.Path as host; ensure downstream S3 client initialization (the code that
uses host) only proceeds when the endpoint has a proper scheme and include a
clear error message referencing the original endpoint value.
---
Nitpick comments:
In `@internal/api/objectstorage/objectstorage.go`:
- Around line 379-393: GetObject is inefficient for large buckets because it
always calls ListObjects; add a short in-memory TTL cache on Service (e.g., map
keyed by slug+bucketSlug + timestamp protected by a mutex) and change
Service.GetObject to consult the cache first, populate it when calling
ListObjects, and honor the TTL to avoid stale data; also ensure writes that
modify bucket contents (upload/delete methods) invalidate the corresponding
cache entry. Use the existing Service type and the ListObjects and GetObject
method names to locate where to add the cache fields and invalidation logic.
In `@internal/commands/objectstorage.go`:
- Around line 669-677: Replace the fmt.Scanln confirmation in the object delete
flow with the same bufio.Scanner pattern used by the other delete commands:
write the prompt to os.Stderr instead of os.Stdout, create a
bufio.NewScanner(os.Stdin), call scanner.Scan() and use
strings.ToLower(strings.TrimSpace(scanner.Text())) to check for "y", and
preserve the existing early return and "Aborted." output if the answer is not
"y"; update the block that references autoApproved(cmd), objectKey and
bucketSlug accordingly so the behavior matches storage/bucket delete routines.
- Line 303: The validation condition in internal/commands/objectstorage.go
redundantly checks "storageGB <= 0 || storageGB < minObjectStorageGB"; since
minObjectStorageGB is 60 (>0), remove the first clause and replace the condition
with "storageGB < minObjectStorageGB" so the check uses only storageGB and
minObjectStorageGB (referencing the storageGB variable and minObjectStorageGB
constant) to validate the value.
🪄 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: 88e5b385-2523-4b05-a789-e693faf0023a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
README.mddocs/api-inventory.mddocs/command-taxonomy.mddocs/roadmap.mdgo.modinternal/api/objectstorage/objectstorage.gointernal/api/objectstorage/objectstorage_test.gointernal/api/objectstorage/s3client.gointernal/api/objectstorage/s3client_test.gointernal/api/project/project.gointernal/commands/billing.gointernal/commands/firewall.gointernal/commands/helpers.gointernal/commands/kubernetes.gointernal/commands/objectstorage.gointernal/commands/portforward.gointernal/commands/project.gotests/smoke/cases.sh
💤 Files with no reviewable changes (3)
- internal/commands/helpers.go
- internal/commands/firewall.go
- internal/commands/portforward.go
✅ Files skipped from review due to trivial changes (3)
- internal/api/project/project.go
- docs/roadmap.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/commands/kubernetes.go
- tests/smoke/cases.sh
…g, and update documentation - Implemented `zcp instance delete` command to permanently delete virtual machines. - Added `Delete` method in the instance service to handle VM deletion with optional immediate expunge. - Updated instance tests to cover deletion scenarios, including success, force deletion, and not found cases. - Enhanced error handling in network unmarshalling and object storage listing. - Improved README and command taxonomy documentation for clarity and accuracy. - Updated smoke test scripts for better error reporting and lifecycle management. - Refactored plan service to include storage category information in the output.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/objectstorage/objectstorage.go (1)
255-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequest the nested region setup in these fetches.
ListObjects,GetObject,PutObject, andDeleteObjectall depend onGet, andNewS3Clientrequiresregion.cloud_provider_setupto deriveS3Endpoint(). These queries only requestregion, so the live response can omit the endpoint and break every S3 operation withhas no S3 endpoint.💡 Minimal fix
q := url.Values{ - "include": {"cloud_provider,region,project,offering"}, + "include": {"cloud_provider,region,region.cloud_provider_setup,project,offering"}, }Also applies to: 267-269
🤖 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.go` around lines 255 - 257, The requests in ListObjects, GetObject, PutObject, DeleteObject (which call Get) currently only include "region" and thus miss region.cloud_provider_setup required by NewS3Client->S3Endpoint; update the url.Values include parameter (the q variable used in these fetches/Get) to request the nested field (e.g., "cloud_provider,region,region.cloud_provider_setup,project,offering") so the returned region includes cloud_provider_setup before NewS3Client/S3Endpoint is invoked.
🧹 Nitpick comments (3)
internal/api/instance/instance.go (1)
493-505: 💤 Low valueConsider initializing
qunconditionally for clarity.When
expungeis false,qremainsnil. While most HTTP clients handlenilquery parameters correctly, explicitly initializingq := url.Values{}would make the intent clearer and avoid potential nil-related surprises in future refactoring.♻️ Optional refactor for consistency
func (s *Service) Delete(ctx context.Context, slug string, expunge bool) error { - var q url.Values + q := url.Values{} if expunge { - q = url.Values{"expunge": {"true"}} + q.Set("expunge", "true") } if err := s.client.Delete(ctx, "/virtual-machines/"+slug, q); err != nil {🤖 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/instance/instance.go` around lines 493 - 505, The Delete method's local q variable can be nil when expunge is false which may cause surprises later; in Service.Delete initialize q := url.Values{} unconditionally before the if expunge block, then set q["expunge"] = []string{"true"} (or assign url.Values{"expunge":{"true"}}) when expunge is true, leaving the subsequent s.client.Delete(ctx, "/virtual-machines/"+slug, q) call to always receive a non-nil url.Values.internal/api/objectstorage/objectstorage_test.go (1)
593-613: ⚡ Quick winHave the fake management API enforce the include contract.
This helper always returns
region.cloud_provider_setup, so the S3 tests still pass even ifService.Getstops requesting the nested include thatNewS3Clientdepends on. As written, the suite won’t catch the missing-include regression in the live path.💡 Suggested assertion
mgmt = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != "/object-storages/my-storage-1" { + http.Error(w, "unexpected management request", http.StatusBadRequest) + return + } + include := r.URL.Query().Get("include") + if !strings.Contains(include, "region.cloud_provider_setup") { + http.Error(w, "missing region.cloud_provider_setup include", http.StatusBadRequest) + return + } + store := objectstorage.ObjectStorage{ ID: "os-1", Slug: "my-storage-1",🤖 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 593 - 613, The fake management API handler currently always returns region.cloud_provider_setup; modify the httptest handler inside the test so it inspects r.URL.Query().Get("include") and only populates store.Region.CloudProviderSetup (or store.Region) when the include query contains "region.cloud_provider_setup" (otherwise return Region without CloudProviderSetup or nil), thereby enforcing the include contract and ensuring Service.Get/NewS3Client actually request the nested include.internal/api/objectstorage/objectstorage.go (1)
404-414: Replace recursive list/scan inGetObjectwith a directStatObjectmetadata lookup
internal/api/objectstorage/objectstorage.go(lines 404-414) currently callss.ListObjects(...)and linearly scans results to findobjectKey, making lookups proportional to bucket size.github.com/minio/minio-go/v7v7.2.0 providesStatObject(ctx, bucketName, objectName, opts) (ObjectInfo, error)to fetch a single object’s metadata directly (e.g., fields such asSize,LastModified,ContentType,ETag, and metadata/tags). UseStatObjectinstead of listing to avoid command timeouts on large buckets.🤖 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.go` around lines 404 - 414, Replace the current ListObjects scan in Service.GetObject with a direct StatObject call: call the MinIO client's StatObject(ctx, bucketSlug, objectKey, minio.StatObjectOptions{}) inside GetObject (instead of s.ListObjects(...)), convert the returned minio.ObjectInfo fields (Size, LastModified, ContentType, ETag and any user metadata/tags) into your internal Object struct and return it, and map/return StatObject errors (wrapping them similarly to the existing fmt.Errorf messages) so missing-object or other failures are reported consistently; remove the loop over objects and ensure imports/ctx usage accommodate minio.StatObjectOptions and error wrapping in GetObject.
🤖 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/objectstorage/objectstorage.go`:
- Around line 390-395: The code is incorrectly synthesizing Permission:
"Private" for every item; instead, when ACL/permission info is not provided by
the S3 list response, set the Object.Permission field to an empty string (or
"Unknown") so you don't report wrong metadata. Update the object construction
(the Object literal where Key/Name/Size/LastModified/Permission are set) to
derive Permission only if a real ACL/permission field exists on obj (e.g.,
obj.ACL or retrieved metadata); otherwise assign "" (or "Unknown") and avoid
hardcoding "Private".
---
Outside diff comments:
In `@internal/api/objectstorage/objectstorage.go`:
- Around line 255-257: The requests in ListObjects, GetObject, PutObject,
DeleteObject (which call Get) currently only include "region" and thus miss
region.cloud_provider_setup required by NewS3Client->S3Endpoint; update the
url.Values include parameter (the q variable used in these fetches/Get) to
request the nested field (e.g.,
"cloud_provider,region,region.cloud_provider_setup,project,offering") so the
returned region includes cloud_provider_setup before NewS3Client/S3Endpoint is
invoked.
---
Nitpick comments:
In `@internal/api/instance/instance.go`:
- Around line 493-505: The Delete method's local q variable can be nil when
expunge is false which may cause surprises later; in Service.Delete initialize q
:= url.Values{} unconditionally before the if expunge block, then set
q["expunge"] = []string{"true"} (or assign url.Values{"expunge":{"true"}}) when
expunge is true, leaving the subsequent s.client.Delete(ctx,
"/virtual-machines/"+slug, q) call to always receive a non-nil url.Values.
In `@internal/api/objectstorage/objectstorage_test.go`:
- Around line 593-613: The fake management API handler currently always returns
region.cloud_provider_setup; modify the httptest handler inside the test so it
inspects r.URL.Query().Get("include") and only populates
store.Region.CloudProviderSetup (or store.Region) when the include query
contains "region.cloud_provider_setup" (otherwise return Region without
CloudProviderSetup or nil), thereby enforcing the include contract and ensuring
Service.Get/NewS3Client actually request the nested include.
In `@internal/api/objectstorage/objectstorage.go`:
- Around line 404-414: Replace the current ListObjects scan in Service.GetObject
with a direct StatObject call: call the MinIO client's StatObject(ctx,
bucketSlug, objectKey, minio.StatObjectOptions{}) inside GetObject (instead of
s.ListObjects(...)), convert the returned minio.ObjectInfo fields (Size,
LastModified, ContentType, ETag and any user metadata/tags) into your internal
Object struct and return it, and map/return StatObject errors (wrapping them
similarly to the existing fmt.Errorf messages) so missing-object or other
failures are reported consistently; remove the loop over objects and ensure
imports/ctx usage accommodate minio.StatObjectOptions and error wrapping in
GetObject.
🪄 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: 4226c73e-25b5-45af-bc92-34151d6953a0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.github/workflows/build.yml.github/workflows/smoke.ymlREADME.mddocs/command-taxonomy.mdgo.modinternal/api/instance/instance.gointernal/api/instance/instance_test.gointernal/api/network/network.gointernal/api/network/network_test.gointernal/api/objectstorage/objectstorage.gointernal/api/objectstorage/objectstorage_test.gointernal/api/objectstorage/s3client.gointernal/api/objectstorage/s3client_test.gointernal/api/plan/plan.gointernal/api/plan/plan_test.gointernal/commands/instance.gointernal/commands/plan.gotests/smoke/README.mdtests/smoke/cases.shtests/smoke/lib.shtests/smoke/smoke.sh
✅ Files skipped from review due to trivial changes (2)
- tests/smoke/README.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/api/network/network_test.go
- go.mod
- tests/smoke/smoke.sh
- internal/api/objectstorage/s3client.go
- docs/command-taxonomy.md
- internal/api/objectstorage/s3client_test.go
- internal/api/plan/plan.go
- internal/api/network/network.go
- tests/smoke/cases.sh
Comment on lines
+390
to
+395
| all = append(all, Object{ | ||
| Key: obj.Key, | ||
| Name: filepath.Base(obj.Key), | ||
| Size: strconv.FormatInt(obj.Size, 10), | ||
| LastModified: obj.LastModified.UTC().Format(time.RFC3339), | ||
| Permission: "Private", |
There was a problem hiding this comment.
Don’t synthesize every object’s permission as Private.
S3 list responses do not carry ACLs, so this now makes object list and object get report false permissions for public buckets/objects. If ACLs are unavailable here, return an empty/unknown value instead of incorrect metadata. As per coding guidelines, validate the actual values of configurations/contracts across layers rather than just their presence.
🤖 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.go` around lines 390 - 395, The code
is incorrectly synthesizing Permission: "Private" for every item; instead, when
ACL/permission info is not provided by the S3 list response, set the
Object.Permission field to an empty string (or "Unknown") so you don't report
wrong metadata. Update the object construction (the Object literal where
Key/Name/Size/LastModified/Permission are set) to derive Permission only if a
real ACL/permission field exists on obj (e.g., obj.ACL or retrieved metadata);
otherwise assign "" (or "Unknown") and avoid hardcoding "Private".
Source: Coding guidelines
… resources - Implemented `delete` command for networks, allowing users to delete isolated networks with confirmation prompts. - Added `delete` command for snapshots, enabling permanent deletion of block storage snapshots with user confirmation. - Introduced `delete` command for VM backups, allowing users to delete VM backups with a confirmation prompt. - Created `delete` command for volumes, enabling permanent deletion of block storage volumes with user confirmation. - Added `delete` command for VPCs, allowing users to delete VPCs with a confirmation prompt. - Updated example usage in various commands to reflect real-world usage scenarios.
… balancers, snapshots, volumes, and VM backups with corresponding tests
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
CLI Enhancements
Documentation
Tests
Chores