This repository was archived by the owner on Sep 9, 2026. It is now read-only.
Repository navigation
OSAC-1484: Add CLI commands and table rendering for ExternalIP resources - #839
Merged
openshift-merge-bot[bot] merged 8 commits intoJul 5, 2026
Conversation
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Supports three target types (--compute-instance, --cluster, --baremetal-instance) and --target-endpoint for cluster targets. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
|
@danmanor: This pull request references OSAC-1484 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
WalkthroughThis PR adds new CLI commands for ExternalIP, ExternalIPAttachment, and ExternalIPPool resources: create commands for ExternalIP and ExternalIPAttachment, describe commands for both, and a get command for ExternalIPPool. It also adds corresponding YAML rendering table definitions and wires all new subcommands into parent CLI commands. ChangesExternalIP CLI Commands and Rendering
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as externalipattachment Cmd
participant Runner as runnerContext.run
participant EIPAPI as ExternalIPsClient
participant TargetAPI as Target List Client
participant AttachAPI as ExternalIPAttachmentsClient
CLI->>Runner: run(cmd, args)
Runner->>EIPAPI: List external IPs
Runner->>Runner: lookup.Find(externalip)
Runner->>TargetAPI: List target resource (compute/cluster/baremetal)
Runner->>Runner: lookup.Find(target)
Runner->>AttachAPI: Create(ExternalIPAttachmentSpec)
AttachAPI-->>Runner: created attachment
Runner-->>CLI: print attachment ID, external IP, target
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
danmanor
marked this pull request as ready for review
July 5, 2026 10:25
danmanor
changed the title
danmanor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/create/externalipattachment/create_externalipattachment_cmd.go`:
- Around line 79-82: Move the “at least one target required” validation out of
the gRPC path by adding `MarkFlagsOneRequired` alongside the existing
`MarkFlagRequired`/`MarkFlagsMutuallyExclusive` setup in
`create_externalipattachment_cmd.go`, and remove the manual target check from
`CreateExternalIPAttachmentCmd` so invalid input is rejected during flag parsing
before any dial happens. Use the existing flag names (`compute-instance`,
`cluster`, `baremetal-instance`, `externalip`) in the command builder and keep
the runtime logic focused on constructing the request after validation succeeds.
- Around line 176-182: The `--target-endpoint` flag is currently accepted even
when the target is not a cluster, but `createExternalIPAttachment` silently
drops it for `--compute-instance` and `--baremetal-instance`. Update the
argument handling around `parseTargetEndpoint` and `spec.TargetEndpoint` to
validate that `targetEndpoint` is only allowed for cluster targets, and return a
clear error when it is used with non-cluster targets.
In
`@internal/cmd/cli/describe/externalipattachment/describe_externalipattachment_cmd.go`:
- Around line 92-144: RenderExternalIPAttachment is still printing raw enum
names for the status and target endpoint. Update the state and target endpoint
rendering to trim the EXTERNAL_IP_ATTACHMENT_STATE_ and
EXTERNAL_IP_ATTACHMENT_ENDPOINT_ prefixes before writing them to the tabwriter,
using the existing strings.TrimPrefix logic around
a.GetStatus().GetState().String() and a.GetSpec().GetTargetEndpoint().String().
Keep the fallback values intact so missing fields still render as "-".
In `@internal/rendering/tables/osac.private.v1.ExternalIP.yaml`:
- Around line 32-33: The ADDRESS field uses a different empty-value fallback
idiom than the other columns in this table. Update the value expression in the
ExternalIP table definition to match the existing has(...) pattern used for
PROJECT/NAME, and keep the fallback to '-' when the status address is unset or
empty. Use the ADDRESS entry in this YAML and the other column expressions as
the reference for the consistency change.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d04ad739-be42-4cb7-82fe-76800d282bbb
📒 Files selected for processing (14)
internal/cmd/cli/create/create_cmd.gointernal/cmd/cli/create/externalip/create_externalip_cmd.gointernal/cmd/cli/create/externalipattachment/create_externalipattachment_cmd.gointernal/cmd/cli/describe/describe_cmd.gointernal/cmd/cli/describe/externalip/describe_externalip_cmd.gointernal/cmd/cli/describe/externalipattachment/describe_externalipattachment_cmd.gointernal/cmd/cli/get/externalippool/get_externalippool_cmd.gointernal/cmd/cli/get/get_cmd.gointernal/rendering/tables/osac.private.v1.ExternalIP.yamlinternal/rendering/tables/osac.private.v1.ExternalIPAttachment.yamlinternal/rendering/tables/osac.private.v1.ExternalIPPool.yamlinternal/rendering/tables/osac.public.v1.ExternalIP.yamlinternal/rendering/tables/osac.public.v1.ExternalIPAttachment.yamlinternal/rendering/tables/osac.public.v1.ExternalIPPool.yaml
Comment on lines
+79
to
+82
| result.MarkFlagRequired("externalip") //nolint:errcheck | ||
| result.MarkFlagsMutuallyExclusive("compute-instance", "cluster", "baremetal-instance") | ||
| return result | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Validate target flags before dialing gRPC; prefer MarkFlagsOneRequired.
The "at least one target required" check (Lines 114-116) runs only after the gRPC connection is already established (Lines 108-112), wasting a dial on invalid input. Cobra already provides MarkFlagsOneRequired, which enforces this at parse time (like MarkFlagsMutuallyExclusive above it) and removes the need for the manual check entirely.
♻️ Proposed fix
result.MarkFlagRequired("externalip") //nolint:errcheck
result.MarkFlagsMutuallyExclusive("compute-instance", "cluster", "baremetal-instance")
+ result.MarkFlagsOneRequired("compute-instance", "cluster", "baremetal-instance")
return result
} defer conn.Close()
- if c.args.computeInstance == "" && c.args.cluster == "" && c.args.baremetalInstance == "" {
- return fmt.Errorf("exactly one target is required: --compute-instance, --cluster, or --baremetal-instance")
- }
-
eipClient := publicv1.NewExternalIPsClient(conn)Also applies to: 108-116
🤖 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/externalipattachment/create_externalipattachment_cmd.go`
around lines 79 - 82, Move the “at least one target required” validation out of
the gRPC path by adding `MarkFlagsOneRequired` alongside the existing
`MarkFlagRequired`/`MarkFlagsMutuallyExclusive` setup in
`create_externalipattachment_cmd.go`, and remove the manual target check from
`CreateExternalIPAttachmentCmd` so invalid input is rejected during flag parsing
before any dial happens. Use the existing flag names (`compute-instance`,
`cluster`, `baremetal-instance`, `externalip`) in the command builder and keep
the runtime logic focused on constructing the request after validation succeeds.
Comment on lines
+176
to
+182
| if c.args.targetEndpoint != "" { | ||
| endpoint, err := parseTargetEndpoint(c.args.targetEndpoint) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| spec.TargetEndpoint = endpoint | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--target-endpoint silently ignored for non-cluster targets.
If a user passes --target-endpoint together with --compute-instance or --baremetal-instance, it's silently dropped with no error — likely confusing since the help text implies it's cluster-specific.
🐛 Proposed fix
+ if c.args.targetEndpoint != "" && c.args.cluster == "" {
+ return fmt.Errorf("--target-endpoint is only valid together with --cluster")
+ }
+
switch {
case c.args.computeInstance != "":📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if c.args.targetEndpoint != "" { | |
| endpoint, err := parseTargetEndpoint(c.args.targetEndpoint) | |
| if err != nil { | |
| return err | |
| } | |
| spec.TargetEndpoint = endpoint | |
| } | |
| if c.args.targetEndpoint != "" && c.args.cluster == "" { | |
| return fmt.Errorf("--target-endpoint is only valid together with --cluster") | |
| } | |
| if c.args.targetEndpoint != "" { | |
| endpoint, err := parseTargetEndpoint(c.args.targetEndpoint) | |
| if err != nil { | |
| return err | |
| } | |
| spec.TargetEndpoint = endpoint | |
| } |
🤖 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/externalipattachment/create_externalipattachment_cmd.go`
around lines 176 - 182, The `--target-endpoint` flag is currently accepted even
when the target is not a cluster, but `createExternalIPAttachment` silently
drops it for `--compute-instance` and `--baremetal-instance`. Update the
argument handling around `parseTargetEndpoint` and `spec.TargetEndpoint` to
validate that `targetEndpoint` is only allowed for cluster targets, and return a
clear error when it is used with non-cluster targets.
Comment on lines
+92
to
+144
| func RenderExternalIPAttachment(w io.Writer, a *publicv1.ExternalIPAttachment) { | ||
| writer := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) | ||
|
|
||
| name := "-" | ||
| if v := a.GetMetadata().GetName(); v != "" { | ||
| name = v | ||
| } | ||
|
|
||
| externalIP := "-" | ||
| if v := a.GetSpec().GetExternalIp(); v != "" { | ||
| externalIP = v | ||
| } | ||
|
|
||
| targetType := "-" | ||
| targetID := "-" | ||
| if v := a.GetSpec().GetComputeInstance(); v != "" { | ||
| targetType = "ComputeInstance" | ||
| targetID = v | ||
| } else if v := a.GetSpec().GetCluster(); v != "" { | ||
| targetType = "Cluster" | ||
| targetID = v | ||
| } else if v := a.GetSpec().GetBaremetalInstance(); v != "" { | ||
| targetType = "BaremetalInstance" | ||
| targetID = v | ||
| } | ||
|
|
||
| state := "-" | ||
| externalIPAddress := "-" | ||
| message := "-" | ||
| if a.GetStatus() != nil { | ||
| state = strings.TrimPrefix(a.GetStatus().GetState().String(), "EXTERNAL_IP_ATTACHMENT_STATE_") | ||
| if v := a.GetStatus().GetExternalIpAddress(); v != "" { | ||
| externalIPAddress = v | ||
| } | ||
| if v := a.GetStatus().GetMessage(); v != "" { | ||
| message = v | ||
| } | ||
| } | ||
|
|
||
| fmt.Fprintf(writer, "ID:\t%s\n", a.GetId()) | ||
| fmt.Fprintf(writer, "Name:\t%s\n", name) | ||
| fmt.Fprintf(writer, "External IP:\t%s\n", externalIP) | ||
| fmt.Fprintf(writer, "Target Type:\t%s\n", targetType) | ||
| fmt.Fprintf(writer, "Target:\t%s\n", targetID) | ||
| if a.GetSpec().GetCluster() != "" { | ||
| endpoint := strings.TrimPrefix(a.GetSpec().GetTargetEndpoint().String(), "EXTERNAL_IP_ATTACHMENT_ENDPOINT_") | ||
| fmt.Fprintf(writer, "Target Endpoint:\t%s\n", endpoint) | ||
| } | ||
| fmt.Fprintf(writer, "External IP Address:\t%s\n", externalIPAddress) | ||
| fmt.Fprintf(writer, "State:\t%s\n", state) | ||
| fmt.Fprintf(writer, "Message:\t%s\n", message) | ||
| writer.Flush() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e go -p 'internal/api' | xargs rg -n 'ExternalIPAttachmentState_EXTERNAL_IP_ATTACHMENT_STATE|ExternalIPAttachmentEndpoint_EXTERNAL_IP_ATTACHMENT_ENDPOINT' -A1Repository: osac-project/fulfillment-service
Length of output: 11331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the enum stringers and related generated code for the public API.
file=internal/api/osac/public/v1/external_ip_attachment_type.pb.go
rg -n 'func \(x ExternalIPAttachment(State|Endpoint)\) String\(\)|String\(\) string|ExternalIPAttachment(State|Endpoint)_EXTERNAL_IP_ATTACHMENT_' "$file" -A6 -B2Repository: osac-project/fulfillment-service
Length of output: 5158
Trim the enum prefixes before rendering
ExternalIPAttachmentState.String() and ExternalIPAttachmentEndpoint.String() return names like EXTERNAL_IP_ATTACHMENT_STATE_READY and EXTERNAL_IP_ATTACHMENT_ENDPOINT_API, so these prefixes need to be removed here; otherwise the CLI prints the raw constant names.
🤖 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/describe/externalipattachment/describe_externalipattachment_cmd.go`
around lines 92 - 144, RenderExternalIPAttachment is still printing raw enum
names for the status and target endpoint. Update the state and target endpoint
rendering to trim the EXTERNAL_IP_ATTACHMENT_STATE_ and
EXTERNAL_IP_ATTACHMENT_ENDPOINT_ prefixes before writing them to the tabwriter,
using the existing strings.TrimPrefix logic around
a.GetStatus().GetState().String() and a.GetSpec().GetTargetEndpoint().String().
Keep the fallback values intact so missing fields still render as "-".
Comment on lines
+32
to
+33
| - header: ADDRESS | ||
| value: "this.status.address != ''? this.status.address: '-'" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Inconsistent CEL idiom for empty-value fallback.
ADDRESS uses != '' while PROJECT/NAME elsewhere in this file use has(...). Based on learnings, has() on a proto3 non-optional string scalar already evaluates to false for the empty default, so this could be unified with the has() idiom used elsewhere for readability/consistency.
♻️ Optional consistency fix
-- header: ADDRESS
- value: "this.status.address != ''? this.status.address: '-'"
+- header: ADDRESS
+ value: "has(this.status.address)? this.status.address: '-'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - header: ADDRESS | |
| value: "this.status.address != ''? this.status.address: '-'" | |
| - header: ADDRESS | |
| value: "has(this.status.address)? this.status.address: '-'" |
🤖 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/rendering/tables/osac.private.v1.ExternalIP.yaml` around lines 32 -
33, The ADDRESS field uses a different empty-value fallback idiom than the other
columns in this table. Update the value expression in the ExternalIP table
definition to match the existing has(...) pattern used for PROJECT/NAME, and
keep the fallback to '-' when the status address is unset or empty. Use the
ADDRESS entry in this YAML and the other column expressions as the reference for
the consistency change.
Source: Learnings
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: danmanor 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 |
This branch was previously deployed
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
OSAC-1484: Add CLI commands and table rendering for ExternalIP resources
Jira: https://redhat.atlassian.net/browse/OSAC-1484
Epic: OSAC-1442 — ExternalIP Resources
Summary
Adds CLI commands and table rendering definitions for ExternalIP, ExternalIPPool, and ExternalIPAttachment resources. This enables tenants to manage external IP allocation and attachment through the CLI, supporting all three target types (ComputeInstance, Cluster, BaremetalInstance) for ExternalIPAttachment.
Changes
Table rendering (6 YAML files):
CLI create commands:
osac create externalip --name --pool— allocate an external IP from a poolosac create externalipattachment --externalip --compute-instance|--cluster|--baremetal-instance [--target-endpoint api|ingress] [--name]— attach an external IP to a target resourceCLI describe commands:
osac describe externalip— key-value detail viewosac describe externalipattachment— key-value detail with oneof target type renderingCLI get subcommand:
osac get externalippool— list (table) and detail (key-value) viewsRegistration:
create_cmd.go,describe_cmd.go,get_cmd.goto register new subcommandsTesting
Acceptance Criteria
osac create externalipcommand with--nameand--poolflagsosac create externalipattachmentcommand with multi-target support (compute-instance, cluster, baremetal-instance) and--target-endpointfor cluster targetsosac describe externalipcommand with key-value renderingosac describe externalipattachmentcommand with oneof target renderingosac get externalippoolsubcommand with list and detail viewsget,delete, andeditwork via reflection (no code needed)Summary by CodeRabbit