fix: add missing --network flag for ip static-nat enable command - #45
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughStatic NAT enablement now requires a network slug, includes it in the API request, returns the response envelope, and displays the resulting status and message in the CLI. ChangesStatic NAT network association
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/ipaddress/ipaddress_test.go (1)
191-198: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the response message used by the CLI.
The updated command renders
ip.Message, but this test only verifiesStatus; populateMessagein the fixture and assertresult.Messageso the response-envelope/output contract is covered.🤖 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 `@pkg/api/ipaddress/ipaddress_test.go` around lines 191 - 198, Update the response fixture used by this test to populate the Message field, then add an assertion for result.Message alongside the existing result.Status check. Verify the expected message emitted by the CLI response envelope while preserving the current body assertions.
🤖 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.
Nitpick comments:
In `@pkg/api/ipaddress/ipaddress_test.go`:
- Around line 191-198: Update the response fixture used by this test to populate
the Message field, then add an assertion for result.Message alongside the
existing result.Status check. Verify the expected message emitted by the CLI
response envelope while preserving the current body assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fef6a324-2442-4c88-8535-3a64fc8b855b
📒 Files selected for processing (3)
internal/commands/ip.gopkg/api/ipaddress/ipaddress.gopkg/api/ipaddress/ipaddress_test.go
|
We will also need some updates to the docs here https://github.com/zsoftly/zcp-cli/blob/main/docs/commands.md?plain=1#L233; The example here still omits --network. Update it if you have time. Otherwise we will fix it in a follow-up. |
Thanks for the fix. The flag, help text, and test line up with the bug in #44. Two items before merge:
|
| // vmSlug is the virtual machine slug. | ||
| func (s *Service) EnableStaticNAT(ctx context.Context, ipSlug, vmSlug string) (*IPAddress, error) { | ||
| body := StaticNATRequest{VirtualMachine: vmSlug} | ||
| func (s *Service) EnableStaticNAT(ctx context.Context, ipSlug, vmSlug, networkSlug string) (*singleResponse, error) { |
There was a problem hiding this comment.
EnableStaticNAT is exported but now returns the unexported singleResponse type. Callers outside the package cannot name it, and Go lint flags it. Other services here return exported result types. Please return an exported type. The shape is your call.
There was a problem hiding this comment.
got it, would make the update
| {"VM", ip.VirtualMachineName}, | ||
| {"Network ID", ip.NetworkID}, | ||
| } | ||
| headers := []string{"STATUS", "MESSAGE"} |
There was a problem hiding this comment.
Does the API return useful data on success? The error case in #44 shows data null. If success is the same, dropping the detail table is right. A quick confirmation here is enough.
There was a problem hiding this comment.
No, it doesn't return any useful data, only the "status" and "message" fields are returned in the response
Signed-off-by: Coker Richard <82083946+cokerrd@users.noreply.github.com>
630b766 to
2651dbf
Compare
|
@ditahkk changes have been pushed but am I to address the failing security analysis step ? or would you or the other maintainers address it? |
2 participants
Addresses #44
--networkflag which now passes the correct request to the apiPrevious output
corrected output
Demo
Screen.Recording.2026-07-23.at.00.36.53.mov
Summary by CodeRabbit
New Features
Improvements