Expose available IP count in public pools CLI & add describe for publicipattachment - #650
Conversation
Assisted-by: Cursor/Claude
|
Warning Review limit reached
More reviews will be available in 6 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR exposes public IP pool availability capacity through a new ChangesPool Availability Exposure
Sequence DiagramNot applicable—straightforward schema extension and CLI consumption without multi-component interaction flows. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/cli/get/publicippool/get_publicippool_cmd.go (1)
107-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor: propagate CLI write/flush failures instead of silently succeeding
renderPoolTable/renderPoolDetailignore theerrorreturns fromfmt.Fprintln,fmt.Fprintf, andwriter.Flush(), soosac get publicippoolcan output partial/empty tables on broken pipes/terminal write failures while still returning success.Suggested fix
- renderPoolTable(c.console, resp.GetItems()) - return nil + if err := renderPoolTable(c.console, resp.GetItems()); err != nil { + return err + } + return nil @@ - renderPoolDetail(c.console, pool) - return nil + return renderPoolDetail(c.console, pool) } // renderPoolTable writes a compact table of pools — used when listing all pools. -func renderPoolTable(w *terminal.Console, pools []*publicv1.PublicIPPool) { +func renderPoolTable(w *terminal.Console, pools []*publicv1.PublicIPPool) error { writer := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) - fmt.Fprintln(writer, "ID\tNAME\tCIDRS\tIP-FAMILY\tAVAILABLE") + if _, err := fmt.Fprintln(writer, "ID\tNAME\tCIDRS\tIP-FAMILY\tAVAILABLE"); err != nil { + return err + } for _, p := range pools { @@ ipFamily := strings.TrimPrefix(p.GetSpec().GetIpFamily().String(), "IP_FAMILY_") available := fmt.Sprintf("%d", p.GetStatus().GetAvailable()) - fmt.Fprintf(writer, "%s\t%s\t%s\t%s\t%s\n", p.GetId(), name, cidrs, ipFamily, available) + if _, err := fmt.Fprintf(writer, "%s\t%s\t%s\t%s\t%s\n", p.GetId(), name, cidrs, ipFamily, available); err != nil { + return err + } } - writer.Flush() + return writer.Flush() } // renderPoolDetail writes a detailed key-value view of a single pool — used when getting by name/id. -func renderPoolDetail(w *terminal.Console, p *publicv1.PublicIPPool) { +func renderPoolDetail(w *terminal.Console, p *publicv1.PublicIPPool) error { writer := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) @@ - fmt.Fprintf(writer, "ID:\t%s\n", p.GetId()) - fmt.Fprintf(writer, "Name:\t%s\n", name) - fmt.Fprintf(writer, "CIDRs:\t%s\n", cidrs) - fmt.Fprintf(writer, "IP Family:\t%s\n", ipFamily) - fmt.Fprintf(writer, "Available:\t%d\n", p.GetStatus().GetAvailable()) - writer.Flush() + if _, err := fmt.Fprintf(writer, "ID:\t%s\n", p.GetId()); err != nil { + return err + } + if _, err := fmt.Fprintf(writer, "Name:\t%s\n", name); err != nil { + return err + } + if _, err := fmt.Fprintf(writer, "CIDRs:\t%s\n", cidrs); err != nil { + return err + } + if _, err := fmt.Fprintf(writer, "IP Family:\t%s\n", ipFamily); err != nil { + return err + } + if _, err := fmt.Fprintf(writer, "Available:\t%d\n", p.GetStatus().GetAvailable()); err != nil { + return err + } + return writer.Flush() }🤖 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/cmd/cli/get/publicippool/get_publicippool_cmd.go` around lines 107 - 123, renderPoolTable (and similarly renderPoolDetail) currently ignore errors from fmt.Fprintln/fmt.Fprintf and writer.Flush, causing commands to return success on write failures; change renderPoolTable to return error, check and propagate the error returned by each fmt.Fprintln/fmt.Fprintf call and by writer.Flush(), and update callers to handle/return that error; reference function renderPoolTable, the tabwriter.Writer variable "writer", and the calls to fmt.Fprintln, fmt.Fprintf, and writer.Flush to locate where to add error checks and returns.
🤖 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.
Outside diff comments:
In `@internal/cmd/cli/get/publicippool/get_publicippool_cmd.go`:
- Around line 107-123: renderPoolTable (and similarly renderPoolDetail)
currently ignore errors from fmt.Fprintln/fmt.Fprintf and writer.Flush, causing
commands to return success on write failures; change renderPoolTable to return
error, check and propagate the error returned by each fmt.Fprintln/fmt.Fprintf
call and by writer.Flush(), and update callers to handle/return that error;
reference function renderPoolTable, the tabwriter.Writer variable "writer", and
the calls to fmt.Fprintln, fmt.Fprintf, and writer.Flush to locate where to add
error checks and returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f0b88748-ea82-424e-9f53-7a08363c7c34
⛔ Files ignored due to path filters (2)
internal/api/osac/public/v1/public_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
internal/cmd/cli/get/publicippool/get_publicippool_cmd.goproto/public/osac/public/v1/public_ip_pool_type.proto
SiddarthR56
changed the title
Assisted-by: Cursor/Claude
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, SiddarthR56 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| result.AddCommand(cluster.Cmd()) | ||
| result.AddCommand(computeinstance.Cmd()) | ||
| result.AddCommand(publicip.Cmd()) | ||
| result.AddCommand(publicipattachment.Cmd()) |
There was a problem hiding this comment.
Do you think we should add publicipattachment to the alias table and subcommands assertion in describe_cmd_test.go? Something like:
Entry("publicipattachment", publicipattachment.Cmd, "publicipattachments")in the alias table"publicipattachment"to theContainElementslist
| } | ||
| ipFamily := strings.TrimPrefix(p.GetSpec().GetIpFamily().String(), "IP_FAMILY_") | ||
| fmt.Fprintf(writer, "%s\t%s\t%s\t%s\n", p.GetId(), name, cidrs, ipFamily) | ||
| available := fmt.Sprintf("%d", p.GetStatus().GetAvailable()) |
There was a problem hiding this comment.
Will this return 0 instead of - when a pool has no status? That could be misleading since 0 suggests no IPs are available rather than status not being populated yet. Same thing for line 146.
Maybe we should do something like this for consistency with the other columns?
available := "-"
if p.GetStatus() \!= nil {
available = fmt.Sprintf("%d", p.GetStatus().GetAvailable())
}WDYT?
There was a problem hiding this comment.
The status will never be nil for pools exposed via the public API (the server only returns pools that are READY with available > 0). That said, I can add a defensive guard if you'd prefer.
| // Capacity information for a PublicIPPool exposed to tenants. | ||
| message PublicIPPoolStatus { | ||
| // Number of IP addresses available for new allocations from this pool. | ||
| int64 available = 6 [(google.api.field_behavior) = OUTPUT_ONLY]; |
There was a problem hiding this comment.
Curious: you're using field number 6 here, is this intentional to match the private proto's field numbering?
There was a problem hiding this comment.
Yes, intentional — matches the private proto's field number for readability.
3 participants
Summary
Add a status field with available count to the public PublicIPPool proto, allowing tenants to see how many IPs are free for allocation
Display an "AVAILABLE" column in osac get publicippool table output and detail view
Add osac describe publicipattachment command for inspecting attachment details by ID or name
Changes
Proto (proto/public/osac/public/v1/public_ip_pool_type.proto):
CLI — get publicippool (internal/cmd/cli/get/publicippool/get_publicippool_cmd.go):
CLI — describe publicipattachment (new):
Naming rationale
Public CLI uses "AVAILABLE" (standalone column, consistent with AWS AvailableIpAddressCount)
Private rendering table keeps "FREE" (alongside TOTAL/ALLOCATED, consistent with kubectl-view-allocations)
Testing
Summary by CodeRabbit