feat: Enhance API requests with additional parameters for cloud integration#3
Merged
Conversation
…ration
- Added `CloudProvider`, `Region`, and `Project` fields to various request structs including CreateRequest for autoscale, DNS, network, virtual router, VPC, and customer gateway.
- Updated command-line interface (CLI) commands to require new parameters for creating resources, ensuring proper cloud context is provided.
- Modified tests to accommodate changes in request structures and validate new parameters.
- Adjusted volume size type from string to interface{} to allow for more flexible size representation.
- Refactored ACL creation commands to support creating ACL lists instead of individual rules, streamlining network security management.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR releases v0.0.8: it adds infrastructure scoping flags ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/command-taxonomy.md (1)
108-126:⚠️ Potential issue | 🟡 MinorUpdate the top-level ACL taxonomy entry too.
Line 116 renames the VPC-side command to
acl-create, but the top-levelaclsection in this same document still advertisescreate-rule. That leaves the taxonomy inconsistent withinternal/commands/acl.go, which now exposesacl create.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/command-taxonomy.md` around lines 108 - 126, The docs are inconsistent: the VPC section uses the new command name `acl-create` while the top-level ACL taxonomy still lists `create-rule`; update the top-level ACL entry to match the actual command exposed by internal/commands/acl.go by renaming `create-rule` to `create` (or otherwise match `acl create`), ensuring the `acl` section's action names (`list`, `create`, `replace`) align with the implemented CLI commands.internal/api/vpn/vpn_test.go (1)
214-249:⚠️ Potential issue | 🟡 MinorAssert the new cloud-scoping fields in
TestVPNUserCreate.Now that
Createtakesvpn.UserCreateRequest, this test should include and verifycloud_provider,region, andprojecttoo. Right now a marshalling regression on any of those new required fields would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/vpn/vpn_test.go` around lines 214 - 249, TestVPNUserCreate currently posts a vpn.UserCreateRequest but doesn't assert the new cloud-scoping fields; update the test to send explicit cloud_provider, region, and project values in the vpn.UserCreateRequest passed to svc.Create and add assertions against gotBody["cloud_provider"], gotBody["region"], and gotBody["project"] to verify they were marshalled correctly. Locate the test function TestVPNUserCreate and the Create call on svc (vpn.UserCreateRequest) and add the new fields to the request payload and corresponding assertions after the existing username/password checks to fail if any of those required fields are omitted or mis-marshalled.internal/commands/acl.go (1)
90-105:⚠️ Potential issue | 🟡 MinorSend the success message through the printer, not stderr.
acl createnow writes its success path tocmd.ErrOrStderr(). That makes a normal success look like diagnostic output and bypasses the same printer/output path used by the other commands here.Suggested fix
func runACLCreate(cmd *cobra.Command, vpcSlug string, req acl.ACLCreateRequest) error { - _, client, _, err := buildClientAndPrinter(cmd) + _, client, printer, err := buildClientAndPrinter(cmd) if err != nil { return err } @@ if err := svc.Create(ctx, vpcSlug, req); err != nil { return fmt.Errorf("acl create: %w", err) } - fmt.Fprintf(cmd.ErrOrStderr(), "ACL %q created in VPC %q.\n", req.Name, vpcSlug) + printer.Fprintf("ACL %q created in VPC %q.\n", req.Name, vpcSlug) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/acl.go` around lines 90 - 105, The success message in runACLCreate is written to cmd.ErrOrStderr(), bypassing the shared printer; change the buildClientAndPrinter call to capture the printer (instead of discarding the third return), then send the success output through that printer (use the printer's appropriate print/printf method) rather than cmd.ErrOrStderr(), keeping the rest of the function and error handling (svc.Create, req.Name, vpcSlug) unchanged.
🧹 Nitpick comments (3)
internal/api/vpc/vpc.go (1)
54-59: Intentional duplication withaclpackage noted.
ACLListCreateRequestandCreateACLmirroracl.ACLCreateRequestandacl.Create. This provides both VPC-scoped (vpc.CreateACL) and standalone (acl.Create) entry points. If this is intentional for API ergonomics, consider adding a code comment to prevent future accidental divergence.Also applies to: 195-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/vpc/vpc.go` around lines 54 - 59, ACLListCreateRequest and the CreateACL method intentionally duplicate acl.ACLCreateRequest and acl.Create to offer both VPC-scoped and standalone entrypoints; add a short code comment above the type and above the CreateACL function stating this is deliberate (reference acl.ACLCreateRequest and acl.Create) and that any changes must be kept in sync to avoid accidental divergence, and optionally point to the canonical source (acl package) as the one to update first.internal/commands/vpc.go (1)
395-416: Function name doesn't match command name.
newVPCACLCreateRuleCmdcreates a command withUse: "acl-create", notacl-create-rule. Consider renaming the function tonewVPCACLCreateCmdfor clarity.♻️ Suggested rename
-func newVPCACLCreateRuleCmd() *cobra.Command { +func newVPCACLCreateCmd() *cobra.Command {Also update line 28:
- cmd.AddCommand(newVPCACLCreateRuleCmd()) + cmd.AddCommand(newVPCACLCreateCmd())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/vpc.go` around lines 395 - 416, The factory function name newVPCACLCreateRuleCmd does not match the command Use value "acl-create"; rename the function to newVPCACLCreateCmd (and update any callers) to restore consistency and clarity. Locate the function definition newVPCACLCreateRuleCmd and change its identifier to newVPCACLCreateCmd, then update any references/imports/usages that invoke newVPCACLCreateRuleCmd to use newVPCACLCreateCmd so the symbol matches the command it constructs.internal/api/acl/acl.go (1)
87-94: Confirmed: Duplicate ACL creation types and methods.
acl.ACLCreateRequestandvpc.ACLListCreateRequestare structurally identical (both withName,Description,VPCfields), and bothacl.Create()andvpc.CreateACL()have identical implementations that POST to/vpcs/{vpcSlug}/network-acl-listwith the same error handling. Consolidate to a single type and method to avoid future divergence during maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/acl/acl.go` around lines 87 - 94, Duplicate ACL creation types and methods exist: acl.ACLCreateRequest and vpc.ACLListCreateRequest are identical and acl.Create() and vpc.CreateACL() duplicate the same POST logic; consolidate by choosing one canonical type (e.g., acl.ACLCreateRequest) and one canonical method (e.g., acl.Create) to handle POSTing to "/vpcs/{vpcSlug}/network-acl-list", replace all usages of the duplicate type/method (vpc.ACLListCreateRequest and vpc.CreateACL) to reference the canonical ones, remove the redundant type and method declarations, and run/update any imports and tests to reflect the refactor so there are no remaining references to the removed symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 24-28: Update the CHANGELOG entry to use the correct CLI command
name used in code: replace the incorrect "zcp vpc acl-create-rule" with "zcp vpc
acl-create" (the actual command implemented in internal/commands/vpc.go as
acl-create) so the changelog matches the command defined in
internal/commands/vpc.go; ensure any other references in CHANGELOG.md or
RELEASE_NOTES.md use the same "acl-create" spelling.
In `@docs/roadmap.md`:
- Around line 19-28: Update the "## Coming in v0.0.8" heading and list items to
remove ambiguity: either change the heading to indicate these are pending items
for a future patch (e.g., "Planned for v0.0.8.x") or mark completed checklist
entries as done if this PR implements them; specifically update the header "##
Coming in v0.0.8" and the checklist lines for `network create` (both flag
additions and `--acl` resolution), `portforward create`, `instance
change-hostname`, `region` command `use` subcommand, and the defaulting of
`--cloud-provider`, `--region`, `--project` so the roadmap matches
CHANGELOG/RELEASE_NOTES.
In `@internal/api/acl/acl_test.go`:
- Around line 51-68: The test TestACLCreate's server handler must also decode
and assert the POST body to ensure the ACL payload is correct: inside the
httptest.NewServer handler (the anonymous function used by httptest.NewServer
that currently checks r.Method and r.URL.Path), read and json-decode the request
body into acl.ACLCreateRequest (or a local struct) and assert that Name ==
"web-acl" and VPC == "my-vpc" before writing the success response; this ensures
the call from svc := acl.NewService(newClient(srv.URL)) and err :=
svc.Create(context.Background(), "my-vpc", acl.ACLCreateRequest{...}) is
actually sending the expected JSON.
In `@internal/api/virtualrouter/virtualrouter_test.go`:
- Around line 108-109: The test's assertion message references the wrong payload
key: update the t.Errorf call that reports the mismatch for gotBody["vr_name"]
so its message references "body[vr_name]" (or "vr_name") instead of
"body[name]"; locate the failing assertion in virtualrouter_test.go where
gotBody["vr_name"] is checked and change the formatted message passed to
t.Errorf to match the checked key.
In `@internal/api/volume/volume.go`:
- Line 48: Change the Size fields from interface{} to json.Number to preserve
numeric precision: add "encoding/json" to imports and update Offering.Size and
Volume.Size to use json.Number instead of interface{} (the existing
fmt.Sprintf("%v", v.Size) usage will continue to work). Locate the Offering and
Volume structs in internal/api/volume/volume.go and replace the type for the
Size fields, ensuring the package imports include encoding/json.
In `@internal/api/vpc/vpc_test.go`:
- Around line 138-148: TestVPCCreate builds a richer vpc.CreateRequest but only
asserts cloud_provider, cidr, and size; update the test (TestVPCCreate) to
assert that the request/created VPC also contains Region, Project, Type,
BillingCycle, Plan, and StorageCategory (i.e., assert Region == "noida", Project
== "default-124", Type == "Vpc", BillingCycle == "hourly", Plan == "vpc-1",
StorageCategory == "nvme"), and repeat the same additional assertions for the
second assertion block referenced (lines ~161-169) so all newly added create
fields are covered.
In `@internal/api/vpn/customergateway.go`:
- Around line 46-48: The CustomerGatewayRequest struct's CloudProvider, Region,
and Project fields currently serialize empty strings during updates; update
their JSON tags in CustomerGatewayRequest to include ",omitempty" (e.g., change
`json:"cloud_provider"` to `json:"cloud_provider,omitempty"`) so these fields
are omitted when empty and won't be sent in update requests like
newVPNCGUpdateCmd.
In `@internal/commands/volume.go`:
- Line 59: Replace the ad-hoc fmt.Sprintf("%v", v.Size) usages with a single
helper formatSize(size interface{}) string that normalizes nil to "-" and
coerces numeric JSON types (float64, int, json.Number, string) into a consistent
numeric string (e.g., no trailing .0 for whole numbers, fixed formatting for
floats) so table columns render stably; add formatSize in the same package and
call it wherever v.Size is used in volume output (the existing spots using
fmt.Sprintf("%v", v.Size) such as the volume table builders referenced by
v.Size) to centralize formatting and avoid "<nil>" or inconsistent
representations.
In `@RELEASE_NOTES.md`:
- Around line 24-31: The release notes show the wrong CLI command name; update
the example to match the actual command defined in internal/commands/vpc.go (the
command whose Use field is "acl-create") by replacing any occurrences of
"acl-create-rule" with "acl-create" in the RELEASE_NOTES.md example so the
documented invocation matches the implemented command (and leave the second
example `zcp acl create` unchanged).
- Around line 9-20: The example in RELEASE_NOTES.md uses the wrong flag name —
replace the `--cidr 10.1.0.1` entry with `--network-address 10.1.0.1` so the
docs match the CLI flag defined in internal/commands/vpc.go (the flag registered
as `--network-address`); update that line in the RELEASE_NOTES.md snippet to use
`--network-address` and keep the rest of the example unchanged.
---
Outside diff comments:
In `@docs/command-taxonomy.md`:
- Around line 108-126: The docs are inconsistent: the VPC section uses the new
command name `acl-create` while the top-level ACL taxonomy still lists
`create-rule`; update the top-level ACL entry to match the actual command
exposed by internal/commands/acl.go by renaming `create-rule` to `create` (or
otherwise match `acl create`), ensuring the `acl` section's action names
(`list`, `create`, `replace`) align with the implemented CLI commands.
In `@internal/api/vpn/vpn_test.go`:
- Around line 214-249: TestVPNUserCreate currently posts a vpn.UserCreateRequest
but doesn't assert the new cloud-scoping fields; update the test to send
explicit cloud_provider, region, and project values in the vpn.UserCreateRequest
passed to svc.Create and add assertions against gotBody["cloud_provider"],
gotBody["region"], and gotBody["project"] to verify they were marshalled
correctly. Locate the test function TestVPNUserCreate and the Create call on svc
(vpn.UserCreateRequest) and add the new fields to the request payload and
corresponding assertions after the existing username/password checks to fail if
any of those required fields are omitted or mis-marshalled.
In `@internal/commands/acl.go`:
- Around line 90-105: The success message in runACLCreate is written to
cmd.ErrOrStderr(), bypassing the shared printer; change the
buildClientAndPrinter call to capture the printer (instead of discarding the
third return), then send the success output through that printer (use the
printer's appropriate print/printf method) rather than cmd.ErrOrStderr(),
keeping the rest of the function and error handling (svc.Create, req.Name,
vpcSlug) unchanged.
---
Nitpick comments:
In `@internal/api/acl/acl.go`:
- Around line 87-94: Duplicate ACL creation types and methods exist:
acl.ACLCreateRequest and vpc.ACLListCreateRequest are identical and acl.Create()
and vpc.CreateACL() duplicate the same POST logic; consolidate by choosing one
canonical type (e.g., acl.ACLCreateRequest) and one canonical method (e.g.,
acl.Create) to handle POSTing to "/vpcs/{vpcSlug}/network-acl-list", replace all
usages of the duplicate type/method (vpc.ACLListCreateRequest and vpc.CreateACL)
to reference the canonical ones, remove the redundant type and method
declarations, and run/update any imports and tests to reflect the refactor so
there are no remaining references to the removed symbols.
In `@internal/api/vpc/vpc.go`:
- Around line 54-59: ACLListCreateRequest and the CreateACL method intentionally
duplicate acl.ACLCreateRequest and acl.Create to offer both VPC-scoped and
standalone entrypoints; add a short code comment above the type and above the
CreateACL function stating this is deliberate (reference acl.ACLCreateRequest
and acl.Create) and that any changes must be kept in sync to avoid accidental
divergence, and optionally point to the canonical source (acl package) as the
one to update first.
In `@internal/commands/vpc.go`:
- Around line 395-416: The factory function name newVPCACLCreateRuleCmd does not
match the command Use value "acl-create"; rename the function to
newVPCACLCreateCmd (and update any callers) to restore consistency and clarity.
Locate the function definition newVPCACLCreateRuleCmd and change its identifier
to newVPCACLCreateCmd, then update any references/imports/usages that invoke
newVPCACLCreateRuleCmd to use newVPCACLCreateCmd so the symbol matches the
command it constructs.
🪄 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: ef2553d0-22ca-4cc1-aa72-493d6e3b9630
📒 Files selected for processing (26)
CHANGELOG.mdREADME.mdRELEASE_NOTES.mddocs/command-taxonomy.mddocs/roadmap.mdinternal/api/acl/acl.gointernal/api/acl/acl_test.gointernal/api/autoscale/autoscale.gointernal/api/dns/dns.gointernal/api/network/network.gointernal/api/virtualrouter/virtualrouter.gointernal/api/virtualrouter/virtualrouter_test.gointernal/api/volume/volume.gointernal/api/vpc/vpc.gointernal/api/vpc/vpc_test.gointernal/api/vpn/customergateway.gointernal/api/vpn/user.gointernal/api/vpn/vpn_test.gointernal/commands/acl.gointernal/commands/autoscale.gointernal/commands/dns.gointernal/commands/network.gointernal/commands/virtualrouter.gointernal/commands/volume.gointernal/commands/vpc.gointernal/commands/vpn.go
…nd VPC creation fixes refactor: Improve volume size formatting in commands and update CustomerGatewayRequest fields test: Enhance VPC and virtual router tests with additional assertions
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.
CloudProvider,Region, andProjectfields to various request structs including CreateRequest for autoscale, DNS, network, virtual router, VPC, and customer gateway.Summary by CodeRabbit
New Features
Bug Fixes
Changed