Repository navigation
OSAC-3849: add GPU fields to describe instancetype output - #232
omer-vishlitzky merged 2 commits into
Conversation
Display GPU PCI device selector, resource name, and count in the describe instancetype output when a GpuSpec is present. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tzif <tmorgens@redhat.com>
|
@Tzif-Morgen: This pull request references OSAC-3849 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. |
WalkthroughThe instance type description command now renders GPU count, resource name, and PCI device selector when ChangesGPU instance type description
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 review |
|
Move GPU fields after memory and before state/description to group hardware specs together. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tzif <tmorgens@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@fulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd.go`:
- Around line 108-112: Update renderInstanceType to return the error from its
tabwriter.Writer Flush call, propagate that error from run when invoking
renderInstanceType(c.console, matched), and adjust the test helper to handle the
new return value.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 42d56f5d-8779-4285-89ce-a4506dedc85a
📒 Files selected for processing (2)
fulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd.gofulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd_test.go
| if gpu := spec.GetGpu(); gpu != nil { | ||
| fmt.Fprintf(writer, "GPU Count:\t%d\n", gpu.GetCount()) | ||
| fmt.Fprintf(writer, "GPU Resource Name:\t%s\n", gpu.GetResourceName()) | ||
| fmt.Fprintf(writer, "GPU PCI Device Selector:\t%s\n", gpu.GetPciDeviceSelector()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 '\brenderInstanceType\s*\(' fulfillment-service/internal/cmd/cli/describe/instancetype --glob '*.go'
rg -n -C 2 'fmt\.Fprintf|\.Flush\(\)' fulfillment-service/internal/cmd/cli/describe/instancetype --glob '*.go'Repository: osac-project/osac
Length of output: 8157
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the full function and call sites without modifying repository files.
cat -n fulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd.go | sed -n '1,170p'
echo '--- tests ---'
cat -n fulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd_test.go | sed -n '1,120p'Repository: osac-project/osac
Length of output: 11265
Propagate tabwriter flush errors.
renderInstanceType creates a *tabwriter.Writer, fills it with output, and calls Flush() while ignoring its error. That can silently emit incomplete CLI output. Make renderInstanceType return error, propagate it from renderInstanceType(c.console, matched) in run, and update the test helper accordingly.
🤖 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
`@fulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd.go`
around lines 108 - 112, Update renderInstanceType to return the error from its
tabwriter.Writer Flush call, propagate that error from run when invoking
renderInstanceType(c.console, matched), and adjust the test helper to handle the
new return value.
Sources: Path instructions, Learnings
There was a problem hiding this comment.
Pre-existing pattern across all describe commands - not in scope for this PR.
There was a problem hiding this comment.
@Tzif-Morgen, acknowledged. This is a pre-existing pattern and is outside the scope of this GPU rendering PR. I will not require a change here.
Would you like me to open a follow-up GitHub issue to track consistent error handling for output writers, tabwriter.Flush(), and connection closure across the describe commands?
🧠 Learnings used
Learnt from: ajamias
Repo: osac-project/osac PR: 156
File: fulfillment-service/internal/cmd/cli/describe/baremetalinstancetype/describe_baremetalinstancetype_cmd.go:67-71
Timestamp: 2026-08-05T18:46:46.995Z
Learning: Across the fulfillment-service CLI, consistently handle errors from output writers, tabwriter.Flush(), and gRPC ClientConn.Close() rather than addressing them only in a new command. Follow the codebase’s established error-reporting or propagation conventions so I/O and connection-close failures are not silently ignored.
You are interacting with an AI system.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Tzif-Morgen, ygalblum 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 |
…ectrpc-connect-web-2.x-lockfile NO-ISSUE: Update dependency @connectrpc/connect-web to v2.2.0
This branch was previously deployed
4 participants
Summary
describe instancetypeoutput when a GpuSpec is presentJira
https://redhat.atlassian.net/browse/OSAC-3849
Test plan
osac describe instancetype gpu-a100-testginkgo run -r internal/cmd/cli/describe/instancetype/)uv run dev.py lint)Summary by CodeRabbit
New Features
Bug Fixes