Skip to content

OSAC-3849: add GPU fields to describe instancetype output #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
omer-vishlitzky merged 2 commits into from
Aug 11, 2026
Merged

OSAC-3849: add GPU fields to describe instancetype output #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
963540d
OSAC-3849: add GPU fields to describe instancetype output
Tzif-Morgen Aug 10, 2026
cea0bcc
OSAC-3849: reorder describe fields — GPU before state
Tzif-Morgen Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions fulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ func renderInstanceType(w io.Writer, it *publicv1.InstanceType) {
fmt.Fprintf(writer, "Cores:\t%d\n", spec.GetCores())
fmt.Fprintf(writer, "Memory (GiB):\t%d\n", spec.GetMemoryGib())

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())
}
Comment on lines +108 to +112

@coderabbitai coderabbitai Bot Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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

@Tzif-Morgen Tzif-Morgen Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing pattern across all describe commands - not in scope for this PR.

@coderabbitai coderabbitai Bot Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


// State with prefix stripping (D-02):
state := strings.TrimPrefix(spec.GetState().String(), "INSTANCE_TYPE_STATE_")
fmt.Fprintf(writer, "State:\t%s\n", state)
Expand Down
41 changes: 41 additions & 0 deletions fulfillment-service/internal/cmd/cli/describe/instancetype/describe_instancetype_cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,4 +148,45 @@ var _ = Describe("Rendering tests", func() {
output := formatInstanceType(it)
Expect(output).To(MatchRegexp(`Description:\s+High memory`))
})

It("should display GPU fields when GpuSpec is present", func() {
it := publicv1.InstanceType_builder{
Id: "gpu-a100-4-16",
Metadata: publicv1.Metadata_builder{
Name: "gpu-a100-4-16",
}.Build(),
Spec: publicv1.InstanceTypeSpec_builder{
Cores: 4,
MemoryGib: 16,
State: publicv1.InstanceTypeState_INSTANCE_TYPE_STATE_ACTIVE,
Gpu: publicv1.GpuSpec_builder{
PciDeviceSelector: "10DE:20B0",
ResourceName: "nvidia.com/A100",
Count: 2,
}.Build(),
}.Build(),
}.Build()
output := formatInstanceType(it)
Expect(output).To(MatchRegexp(`GPU PCI Device Selector:\s+10DE:20B0`))
Expect(output).To(MatchRegexp(`GPU Resource Name:\s+nvidia.com/A100`))
Expect(output).To(MatchRegexp(`GPU Count:\s+2`))
})

It("should omit GPU fields when GpuSpec is absent", func() {
it := publicv1.InstanceType_builder{
Id: "standard-4-16",
Metadata: publicv1.Metadata_builder{
Name: "standard-4-16",
}.Build(),
Spec: publicv1.InstanceTypeSpec_builder{
Cores: 4,
MemoryGib: 16,
State: publicv1.InstanceTypeState_INSTANCE_TYPE_STATE_ACTIVE,
}.Build(),
}.Build()
output := formatInstanceType(it)
Expect(output).NotTo(ContainSubstring("GPU PCI Device Selector:"))
Expect(output).NotTo(ContainSubstring("GPU Resource Name:"))
Expect(output).NotTo(ContainSubstring("GPU Count:"))
})
})
Loading