Add PublicIP and PublicIPAttachment CLI commands - #476
Conversation
|
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:
WalkthroughThis PR adds CLI commands for managing public IP addresses and their attachments to compute instances. It registers new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmd/cli/create/publicip/create_publicip_cmd.go`:
- Around line 84-90: The code dereferences cfg.Address after calling
config.Load(ctx) without checking for a nil cfg; update the create public IP
command to guard against a nil config by adding a nil check of cfg after the err
check (i.e., after config.Load returns) and return a clear error (similar to the
existing message) if cfg == nil before accessing cfg.Address; reference the
config.Load call and the cfg variable to locate where to add this check in
create_publicip_cmd.go.
In `@internal/cmd/cli/delete/publicip/delete_publicip_cmd.go`:
- Around line 63-69: The code assumes cfg is non-nil after calling
config.Load(ctx); add a nil-check for cfg before accessing cfg.Address to avoid
a potential panic: after "cfg, err := config.Load(ctx)" keep the existing err
check, then if cfg == nil return an error (e.g., fmt.Errorf("no configuration,
run the 'login' command")), and only then check cfg.Address == "" to return the
same user-facing error; update the checks in the delete public IP command (the
block around config.Load) to perform the nil guard first.
In `@internal/cmd/cli/describe/publicip/describe_publicip_cmd.go`:
- Around line 79-81: The List call is unbounded; change the
publicv1.PublicIPsListRequest_builder used in client.List to include a small
limit (e.g., PageSize/Limit = 2) so you only fetch enough results to distinguish
0, 1, or >1 matches for the describe-by-reference flow; update the request
builder invocation (the PublicIPsListRequest_builder passed to client.List) to
set that limit and keep the existing filter handling and downstream logic that
checks listResponse results.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57937b6e-c220-4665-b028-85aa700117cb
📒 Files selected for processing (10)
internal/cmd/cli/create/create_cmd.gointernal/cmd/cli/create/create_cmd_test.gointernal/cmd/cli/create/publicip/create_publicip_cmd.gointernal/cmd/cli/delete/delete_cmd.gointernal/cmd/cli/delete/publicip/delete_publicip_cmd.gointernal/cmd/cli/describe/describe_cmd.gointernal/cmd/cli/describe/describe_cmd_test.gointernal/cmd/cli/describe/publicip/describe_publicip_cmd.gointernal/cmd/cli/describe/publicip/describe_publicip_suite_test.gointernal/cmd/cli/describe/publicip/describe_publicip_test.go
|
|
||
| // Delete each resolved object: | ||
| // Delete each resolved object. Attempt all deletions and report errors at the end | ||
| var hadErrors bool |
There was a problem hiding this comment.
Following the kubectl/oc pattern, a failure on one item does
not prevent the remaining items from being attempted. All errors are
reported inline and the command exits with a non-zero code if any deletion
failed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmd/cli/create/publicip/create_publicip_cmd.go`:
- Around line 50-55: Add support for a --compute-instance flag and set
Spec.ComputeInstance in the create payload: add a new string field on
runner.args (e.g., computeInstance string) and register it with flags.StringVar
(similar to the existing flags.StringVar for runner.args.pool), update the
command's payload construction (where Spec.Pool is set) to also set
Spec.ComputeInstance (or the appropriate pointer/ID field) when
runner.args.computeInstance is non-empty, and add basic validation/error
handling for mutually exclusive or required fields if applicable (check the same
areas around the existing flags and the payload builder referenced by
runner.args and Spec.Pool).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c35c5de7-51fa-4bd6-b41d-7b549e1abad7
📒 Files selected for processing (10)
internal/cmd/cli/create/create_cmd.gointernal/cmd/cli/create/create_cmd_test.gointernal/cmd/cli/create/publicip/create_publicip_cmd.gointernal/cmd/cli/delete/delete_cmd.gointernal/cmd/cli/delete/publicip/delete_publicip_cmd.gointernal/cmd/cli/describe/describe_cmd.gointernal/cmd/cli/describe/describe_cmd_test.gointernal/cmd/cli/describe/publicip/describe_publicip_cmd.gointernal/cmd/cli/describe/publicip/describe_publicip_suite_test.gointernal/cmd/cli/describe/publicip/describe_publicip_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/cmd/cli/describe/describe_cmd.go
- internal/cmd/cli/create/create_cmd.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cmd/cli/describe/describe_cmd_test.go
- internal/cmd/cli/create/create_cmd_test.go
- internal/cmd/cli/describe/publicip/describe_publicip_suite_test.go
- internal/cmd/cli/describe/publicip/describe_publicip_cmd.go
- internal/cmd/cli/delete/publicip/delete_publicip_cmd.go
| flags.StringVar( | ||
| &runner.args.pool, | ||
| "pool", | ||
| "", | ||
| "ID of the parent PublicIPPool to allocate the address from.", | ||
| ) |
There was a problem hiding this comment.
Missing --compute-instance support in create flow
The command currently exposes only --name and --pool, and the create payload sets only Spec.Pool. That leaves the advertised optional compute-instance-at-create behavior unimplemented.
Also applies to: 60-63, 94-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmd/cli/create/publicip/create_publicip_cmd.go` around lines 50 -
55, Add support for a --compute-instance flag and set Spec.ComputeInstance in
the create payload: add a new string field on runner.args (e.g., computeInstance
string) and register it with flags.StringVar (similar to the existing
flags.StringVar for runner.args.pool), update the command's payload construction
(where Spec.Pool is set) to also set Spec.ComputeInstance (or the appropriate
pointer/ID field) when runner.args.computeInstance is non-empty, and add basic
validation/error handling for mutually exclusive or required fields if
applicable (check the same areas around the existing flags and the payload
builder referenced by runner.args and Spec.Pool).
There was a problem hiding this comment.
attaching a compute instance at creation time (via --compute-instance flag) is future scope. To attach a public IP to a compute instance
There was a problem hiding this comment.
@SiddarthR56, understood! The design intentionally separates concerns — osac create publicip only allocates the IP from a pool, and attaching it to a compute instance is handled separately via the dedicated osac create publicipattachment command. I'll make sure not to flag this pattern in future reviews.
( ´◔‸◔｀) 🐇📝
✏️ Learnings added
Learnt from: SiddarthR56
Repo: osac-project/fulfillment-service PR: 476
File: internal/cmd/cli/create/publicip/create_publicip_cmd.go:49-54
Timestamp: 2026-05-05T17:47:50.714Z
Learning: In the `osac-project/fulfillment-service` repository, attaching a PublicIP to a compute instance at creation time (via a hypothetical `--compute-instance` flag on `osac create publicip`) is intentionally out of scope. The `osac create publicip` command only allocates a PublicIP from a pool (`--pool` flag). Attaching/detaching a PublicIP to/from a compute instance is handled by the separate `osac create publicipattachment` and `osac delete publicipattachment` commands. Do not flag the absence of `--compute-instance` in `create publicip` as a missing feature.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: SiddarthR56
Repo: osac-project/fulfillment-service PR: 476
File: internal/cmd/cli/create/publicip/create_publicip_cmd.go:84-90
Timestamp: 2026-04-30T18:00:32.141Z
Learning: In this service, `config.Load` (from `internal/config/config.go`) uses named return values and guarantees `cfg` is non-nil (`cfg = &Config{}`) on every path that returns `nil` error. Therefore callers should not be flagged for missing `nil` guards on the returned config: it is sufficient to check `if err != nil` before using the config (i.e., `(nil, nil)` cannot occur).
Learnt from: SiddarthR56
Repo: osac-project/fulfillment-service PR: 476
File: internal/cmd/cli/delete/publicip/delete_publicip_cmd.go:63-69
Timestamp: 2026-04-30T18:00:39.749Z
Learning: When reviewing Go code that calls `config.Load` from `internal/config/config.go`, do not flag code for a missing nil-check on the returned `cfg` after verifying `err == nil`. The `config.Load` implementation uses named returns and assigns `cfg = &Config{}` on every successful (non-error) return path, so `(nil, nil)` is impossible; the `if err != nil { ... }` (or equivalent guard) is sufficient to guarantee `cfg` is non-nil when `err` is nil.
Learnt from: SiddarthR56
Repo: osac-project/fulfillment-service PR: 476
File: internal/cmd/cli/describe/publicip/describe_publicip_cmd.go:79-81
Timestamp: 2026-04-30T18:00:56.367Z
Learning: In this repository’s `config.Load` implementation (config.go), successful returns always provide a non-nil `*Config` (it assigns `cfg = &Config{}` along every success path, so a `(nil, nil)` is impossible). Therefore, in Go code that calls `config.Load`, the existing `if err != nil` guard is sufficient—do not add an extra `cfg == nil` nil-check after `config.Load` when handling the non-error path, unless you change/override the `config.Load` contract.
| @@ -44,6 +45,7 @@ func Cmd() *cobra.Command { | |||
| Short: "Delete objects", | |||
| RunE: runner.run, | |||
| } | |||
| result.AddCommand(publicip.Cmd()) | |||
There was a problem hiding this comment.
Why is publicip the only resource type registered here as a subcommand? Based off my cursory looking we use reflection to route delte commands, can we do the same for publicip or is there some specific logic we need that differs?
There was a problem hiding this comment.
@SiddarthR56 The delete/publicip/ package has a full implementation but is never imported or registered in delete_cmd.go. It's unreachable dead code. osac delete publicip currently falls through to the generic reflection-based handler, which already handles name/ID resolution, continue-on-error, and template-based error messages for all other resources (cluster, subnet, etc.).
I'd suggest removing the delete/publicip/ directory. Every other resource uses the generic handler for deletion, and it works fine for PublicIP too. The only typed delete subcommand (publicipattachment) exists because it's not a real delete, it's an Update RPC that clears a field, which reflection can't handle. PublicIP deletion is a standard Delete RPC, so the generic path covers it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cmd/cli/attach/publicip/attach_publicip_cmd.go`:
- Around line 100-104: Guard against a nil spec before mutating
compute_instance: replace the direct call pip.GetSpec().SetComputeInstance(...)
with a nil-safe pattern by capturing spec := pip.GetSpec(); if spec != nil {
spec.SetComputeInstance(c.args.computeInstance) } so you won't dereference a nil
pointer (apply the same fix in the detach command by using spec :=
pip.GetSpec(); if spec != nil { spec.ClearComputeInstance() }); update the code
around pip.GetSpec().SetComputeInstance and pip.GetSpec().ClearComputeInstance
accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 291326fc-2160-420a-ad28-0bbfacee544d
📒 Files selected for processing (15)
internal/cmd/cli/attach/attach_cmd.gointernal/cmd/cli/attach/publicip/attach_publicip_cmd.gointernal/cmd/cli/create/create_cmd.gointernal/cmd/cli/create/create_cmd_test.gointernal/cmd/cli/create/publicip/create_publicip_cmd.gointernal/cmd/cli/delete/delete_cmd.gointernal/cmd/cli/delete/publicip/delete_publicip_cmd.gointernal/cmd/cli/describe/describe_cmd.gointernal/cmd/cli/describe/describe_cmd_test.gointernal/cmd/cli/describe/publicip/describe_publicip_cmd.gointernal/cmd/cli/describe/publicip/describe_publicip_suite_test.gointernal/cmd/cli/describe/publicip/describe_publicip_test.gointernal/cmd/cli/detach/detach_cmd.gointernal/cmd/cli/detach/publicip/detach_publicip_cmd.gointernal/cmd/cli/root_cmd.go
✅ Files skipped from review due to trivial changes (3)
- internal/cmd/cli/describe/publicip/describe_publicip_suite_test.go
- internal/cmd/cli/describe/describe_cmd.go
- internal/cmd/cli/describe/publicip/describe_publicip_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cmd/cli/describe/describe_cmd_test.go
- internal/cmd/cli/create/publicip/create_publicip_cmd.go
- internal/cmd/cli/delete/publicip/delete_publicip_cmd.go
- internal/cmd/cli/describe/publicip/describe_publicip_cmd.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/cmd/cli/delete/publicipattachment/delete_publicipattachment_cmd.go (1)
81-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame nil
specpanic risk as in the create attachment command.
pip.GetSpec()can returnnilif the spec field is absent, causingClearComputeInstance()to panic. The past review explicitly flagged this for both the attach and detach paths.🛡️ Proposed fix
-pip.GetSpec().ClearComputeInstance() +spec := pip.GetSpec() +if spec == nil { + return fmt.Errorf("public IP '%s' has no spec field", args[0]) +} +spec.ClearComputeInstance()🤖 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/delete/publicipattachment/delete_publicipattachment_cmd.go` around lines 81 - 82, The delete path calls pip.GetSpec().ClearComputeInstance() which can panic if GetSpec() returns nil; update delete_publicipattachment_cmd.go to first capture spec := pip.GetSpec() and only call spec.ClearComputeInstance() when spec != nil (mirror the nil-check used in the create attachment command) so the detach path is safe when spec is absent.internal/cmd/cli/create/publicipattachment/create_publicipattachment_cmd.go (1)
100-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNil
specguard still missing — will panic ifGetSpec()returns nil.
pip.GetSpec()returns a nil*PublicIPSpecwhen the server omits thespecfield, and chaining.SetComputeInstance(...)on that nil pointer panics at runtime.🛡️ Proposed fix
-pip.GetSpec().SetComputeInstance(c.args.computeInstance) +spec := pip.GetSpec() +if spec == nil { + return fmt.Errorf("public IP '%s' has no spec field", c.args.publicIP) +} +spec.SetComputeInstance(c.args.computeInstance)🤖 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/create/publicipattachment/create_publicipattachment_cmd.go` around lines 100 - 101, The call chains pip := getResponse.GetObject(); pip.GetSpec().SetComputeInstance(...) will panic if pip.GetSpec() is nil; guard by checking spec := pip.GetSpec() and if spec == nil create a new PublicIPSpec (or call pip.SetSpec(new PublicIPSpec{})) before setting the compute instance, then call SetComputeInstance (or set the ComputeInstance field on the newly created spec) so pip always has a non-nil spec when you assign c.args.computeInstance.
🤖 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.
Duplicate comments:
In `@internal/cmd/cli/create/publicipattachment/create_publicipattachment_cmd.go`:
- Around line 100-101: The call chains pip := getResponse.GetObject();
pip.GetSpec().SetComputeInstance(...) will panic if pip.GetSpec() is nil; guard
by checking spec := pip.GetSpec() and if spec == nil create a new PublicIPSpec
(or call pip.SetSpec(new PublicIPSpec{})) before setting the compute instance,
then call SetComputeInstance (or set the ComputeInstance field on the newly
created spec) so pip always has a non-nil spec when you assign
c.args.computeInstance.
In `@internal/cmd/cli/delete/publicipattachment/delete_publicipattachment_cmd.go`:
- Around line 81-82: The delete path calls pip.GetSpec().ClearComputeInstance()
which can panic if GetSpec() returns nil; update
delete_publicipattachment_cmd.go to first capture spec := pip.GetSpec() and only
call spec.ClearComputeInstance() when spec != nil (mirror the nil-check used in
the create attachment command) so the detach path is safe when spec is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 689dda0e-897d-4ea1-9934-c7b65e17444f
📒 Files selected for processing (12)
internal/cmd/cli/create/create_cmd.gointernal/cmd/cli/create/create_cmd_test.gointernal/cmd/cli/create/publicip/create_publicip_cmd.gointernal/cmd/cli/create/publicipattachment/create_publicipattachment_cmd.gointernal/cmd/cli/delete/delete_cmd.gointernal/cmd/cli/delete/publicip/delete_publicip_cmd.gointernal/cmd/cli/delete/publicipattachment/delete_publicipattachment_cmd.gointernal/cmd/cli/describe/describe_cmd.gointernal/cmd/cli/describe/describe_cmd_test.gointernal/cmd/cli/describe/publicip/describe_publicip_cmd.gointernal/cmd/cli/describe/publicip/describe_publicip_suite_test.gointernal/cmd/cli/describe/publicip/describe_publicip_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/cmd/cli/describe/describe_cmd.go
- internal/cmd/cli/describe/publicip/describe_publicip_suite_test.go
- internal/cmd/cli/describe/publicip/describe_publicip_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cmd/cli/describe/describe_cmd_test.go
- internal/cmd/cli/describe/publicip/describe_publicip_cmd.go
- internal/cmd/cli/delete/publicip/delete_publicip_cmd.go
akshaynadkarni
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, 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 |
|
/unhold |
3 participants
Description
Added three osac CLI subcommands for managing PublicIP resources using the public tenant-facing API, following the existing Subnet CLI pattern.
spec.compute_instancevia a targeted field-maskupdate. Accepts the public IP and compute instance by ID or name.
Testing
Automated — unit tests
$ go run github.com/onsi/ginkgo/v2/ginkgo run
internal/cmd/cli/describe/publicip
internal/cmd/cli/describe
internal/cmd/cli/create
[Describe PublicIP Suite] 6/6 specs SUCCESS! PASS
[Describe command] 13/13 specs SUCCESS! PASS
[Create command] 8/8 specs SUCCESS! PASS
Ginkgo ran 3 suites in 2.929s ✅ Test Suite Passed
Manual — integration testing
Createed a PublicIP:
$ ./osac create publicip --name test-ip --pool 019ddf22-7b95-700c-80ee-0d302cb8019c
Created public IP 'test-ip' (ID: 019ddf23-4186-75a8-8f5e-8d1a79cabbdc).
Describe a PublicIP:
$ ./osac describe publicip test-ip
ID: 019ddf23-4186-75a8-8f5e-8d1a79cabbdc
Name: test-ip
Pool: 019ddf22-7b95-700c-80ee-0d302cb8019c
Compute Instance: -
Address: -
State: -
Message: -
Delete Multiple PublicIPs:
$ ./osac delete publicip test-ip test-ip-2
Deleted public IP 'test-ip'.
Deleted public IP 'test-ip-2'.
Create a publicIP attachemnt
$ ./osac create publicipattachment --publicip 019dfd74-29c5-7305-b163-5853bbb03e5c --compute-instance 019dfe1f-45de-7450-858f-5f9292521db4
Attached public IP '019dfd74-29c5-7305-b163-5853bbb03e5c' to compute instance '019dfe1f-45de-7450-858f-5f9292521db
Delelte a publicIP attachement
$ ./osac delete publicipattachment 019dfd74-29c5-7305-b163-5853bbb03e5c
Detached public IP '019dfd74-29c5-7305-b163-5853bbb03e5c' from its compute instance.
Summary by CodeRabbit
Release Notes
New Features
create,describe, anddeletecommandsImprovements