Repository navigation
OSAC-3829: add GPU flags to CLI create instancetype command - #230
Conversation
|
@Tzif-Morgen: This pull request references OSAC-3829 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe instance type creation command now accepts GPU PCI selector, resource name, and count flags. It requires the flags together, validates a positive count, and adds GPU configuration to the instance type specification. ChangesGPU instance type creation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
fulfillment-service/internal/cmd/cli/create/instancetype/create_instancetype_cmd_test.go (1)
64-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new behavior, not only flag registration.
These tests verify that each flag exists and that one help string contains a keyword. They do not prove that partial GPU flag groups are rejected, zero or negative counts fail, empty values are handled, or valid flags populate
GpuSpec.Add focused command tests for these cases. The supplied
fulfillment-service/internal/servers/private_instance_types_server_test.gotest verifies downstreamGpuSpecconstruction, but it does not exercise this CLI builder and validation path.🤖 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/create/instancetype/create_instancetype_cmd_test.go` around lines 64 - 89, Extend the create command tests beyond registration to exercise the GPU flag validation and construction path in Cmd. Add focused cases covering partial GPU flag groups, zero and negative gpu-count values, empty GPU flag values, and a valid combination that verifies GpuSpec is populated correctly; use the command’s existing execution/build symbols and preserve the downstream server test’s scope.
🤖 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/create/instancetype/create_instancetype_cmd.go`:
- Around line 137-140: Move the gpuCount validation for non-empty
gpuPCIDeviceSelector into the command’s parameter-validation block before
cfg.Connect is invoked. Preserve the existing error for gpuCount <= 0 and remove
the later duplicate check so invalid input is rejected before any gRPC
connection or authentication setup.
- Around line 216-218: Update the gpuCountFlagHelp text for the gpu-count flag
to explicitly state that the number of GPU devices must be greater than zero,
matching the validation that rejects gpu-count <= 0.
- Around line 137-146: Update the GPU validation in the create instancetype
command to use flag presence rather than only gpuPCIDeviceSelector’s value when
deciding whether GPU configuration was supplied. Before building
specBuilder.Gpu, reject an empty gpuPCIDeviceSelector, empty gpuResourceName, or
non-positive gpuCount; preserve the no-GPU path when none of the GPU flags are
present.
---
Nitpick comments:
In
`@fulfillment-service/internal/cmd/cli/create/instancetype/create_instancetype_cmd_test.go`:
- Around line 64-89: Extend the create command tests beyond registration to
exercise the GPU flag validation and construction path in Cmd. Add focused cases
covering partial GPU flag groups, zero and negative gpu-count values, empty GPU
flag values, and a valid combination that verifies GpuSpec is populated
correctly; use the command’s existing execution/build symbols and preserve the
downstream server test’s scope.
🪄 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: 6cac9b29-b9ce-4af7-9cae-f45dadd983e1
📒 Files selected for processing (2)
fulfillment-service/internal/cmd/cli/create/instancetype/create_instancetype_cmd.gofulfillment-service/internal/cmd/cli/create/instancetype/create_instancetype_cmd_test.go
Add --gpu-pci-device-selector, --gpu-resource-name, and --gpu-count flags to `osac create instancetype`. Uses cobra MarkFlagsRequiredTogether for all-or-nothing validation. When all three flags are provided, the created InstanceType includes a populated GpuSpec. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tzif <tmorgens@redhat.com>
f15e199 to
e146fa3
Compare
|
[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 |
This branch was previously deployed
4 participants
OSAC-3829: add GPU flags to CLI create instancetype command
Jira: OSAC-3829
Story type: [DEV]
Summary
Adds
--gpu-pci-device-selector,--gpu-resource-name, and--gpu-countflags to theosac create instancetypeCLI command, enabling Cloud Provider Admins to create GPU-enabled InstanceTypes directly from the CLI. All three GPU flags are required together (all-or-nothing via cobra'sMarkFlagsRequiredTogether), and client-side validation ensuresgpu-countis greater than zero.Changes
runnerContextand registered corresponding cobra flagsMarkFlagsRequiredTogetherfor all-or-nothing GPU flag validationgpu-count > 0validation matching existingcores/memory-gibpatternInstanceTypeSpec_builderviaGpuSpec_builder(conditional — only when GPU flags are provided)longHelpand flag help text constantsTesting
--gpu-pci-device-selector,--gpu-resource-name,--gpu-count(8/8 pass)get instancetypes -o jsonCmd()at 100%;run()at 0% (pre-existing — requires live gRPC server)Acceptance Criteria
osac create instancetypeaccepts--gpu-pci-device-selector,--gpu-resource-name, and--gpu-countflagsosac create instancetype --helpdocuments the new GPU flagsRelated PRs
Summary by CodeRabbit