OSAC-1359: add create and describe instancetype CLI commands - #723
Conversation
|
@ygalblum: This pull request references OSAC-1359 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: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds ChangesInstance Type CLI Commands
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
5747f6e to
7cb1432
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
7cb1432 to
e7f38a7
Compare
- Create instancetype command with --name, --cores, --memory-gib, --description flags - Uses private API client (privatev1.NewInstanceTypesClient) for admin-only creation - Sets both Id and Metadata.Name to name value (name-as-PK per Phase 1 D-01) - Validates name non-empty, cores > 0, memory-gib > 0 before API call - Registers subcommand in parent create_cmd.go via AddCommand - Adds Ginkgo suite and flag registration tests (5 specs) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
- Describe instancetype command using public API (publicv1.NewInstanceTypesClient) - Resolves instance type via lookup.Find helper with empty FindOptions (no soft delete) - Renders base fields: name, cores, memory (GiB), state, description - Strips INSTANCE_TYPE_STATE_ prefix from state display (D-02) - Conditional deprecation section: replacement, deprecated at, obsolete at (D-01) - Timestamps formatted as RFC3339 (D-03) - No --include-deleted flag (state-based lifecycle, not soft delete) - Registers subcommand in parent describe_cmd.go via AddCommand - Adds Ginkgo suite and 6 rendering tests covering all display scenarios Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
e7f38a7 to
f7829cf
Compare
| } | ||
|
|
||
| // Conditional deprecation section (D-01): | ||
| if dep := spec.GetDeprecation(); dep != nil { | ||
| hasContent := dep.GetReplacement() != "" || | ||
| dep.GetDeprecationTimestamp() != nil || | ||
| dep.GetObsolescenceTimestamp() != nil | ||
| if hasContent { | ||
| if dep.GetReplacement() != "" { | ||
| fmt.Fprintf(writer, "Replacement:\t%s\n", dep.GetReplacement()) | ||
| } | ||
| if ts := dep.GetDeprecationTimestamp(); ts != nil { | ||
| fmt.Fprintf(writer, "Deprecated At:\t%s\n", ts.AsTime().Format(time.RFC3339)) | ||
| } | ||
| if ts := dep.GetObsolescenceTimestamp(); ts != nil { | ||
| fmt.Fprintf(writer, "Obsolete At:\t%s\n", ts.AsTime().Format(time.RFC3339)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: The deprecation block accesses spec outside the if spec != nil guard above. Safe at runtime (protobuf getters handle nil receivers), but structurally confusing since everything else that depends on spec is inside the guard. Consider moving it in:
| } | |
| // Conditional deprecation section (D-01): | |
| if dep := spec.GetDeprecation(); dep != nil { | |
| hasContent := dep.GetReplacement() != "" || | |
| dep.GetDeprecationTimestamp() != nil || | |
| dep.GetObsolescenceTimestamp() != nil | |
| if hasContent { | |
| if dep.GetReplacement() != "" { | |
| fmt.Fprintf(writer, "Replacement:\t%s\n", dep.GetReplacement()) | |
| } | |
| if ts := dep.GetDeprecationTimestamp(); ts != nil { | |
| fmt.Fprintf(writer, "Deprecated At:\t%s\n", ts.AsTime().Format(time.RFC3339)) | |
| } | |
| if ts := dep.GetObsolescenceTimestamp(); ts != nil { | |
| fmt.Fprintf(writer, "Obsolete At:\t%s\n", ts.AsTime().Format(time.RFC3339)) | |
| } | |
| } | |
| } | |
| // Conditional deprecation section (D-01): | |
| if dep := spec.GetDeprecation(); dep != nil { | |
| hasContent := dep.GetReplacement() != "" || | |
| dep.GetDeprecationTimestamp() != nil || | |
| dep.GetObsolescenceTimestamp() != nil | |
| if hasContent { | |
| if dep.GetReplacement() != "" { | |
| fmt.Fprintf(writer, "Replacement:\t%s\n", dep.GetReplacement()) | |
| } | |
| if ts := dep.GetDeprecationTimestamp(); ts != nil { | |
| fmt.Fprintf(writer, "Deprecated At:\t%s\n", ts.AsTime().Format(time.RFC3339)) | |
| } | |
| if ts := dep.GetObsolescenceTimestamp(); ts != nil { | |
| fmt.Fprintf(writer, "Obsolete At:\t%s\n", ts.AsTime().Format(time.RFC3339)) | |
| } | |
| } | |
| } | |
| } |
| writer := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) | ||
|
|
||
| // Base fields (always shown): | ||
| fmt.Fprintf(writer, "Name:\t%s\n", it.GetMetadata().GetName()) |
There was a problem hiding this comment.
question: Other describe commands (cluster, computeinstance, securitygroup) show Id as the first field. This one shows Name instead. Since the server sets id = name for instance types, the values are the same, so this isn't wrong. Was this intentional, or should it match the other describe commands?
There was a problem hiding this comment.
Yes it was intentional because we want users to use name instead of forcing them to look for the ID
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
LGTM, just a couple of minor comments.
Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, 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 |
|
🏗️ CI Triage: Root cause: The OpenShift CI platform failed to import the latest release image stream tag due to a timeout/context deadline exceeded. Explanation: The build log shows that the job failed during the initial graph execution phase before any E2E steps could run. Specifically, the step Evidence: Suggestion: retrigger — known flake Prow job | Build For deeper investigation, use the |
|
/retest |
|
🏗️ CI Triage: Root cause: The vmaas-helm cluster snapshot flavor contains an invalid osac-operator image reference (ghcr.io/osac-project/osac-operator:sha-08991cc) that does not exist in the GHCR registry Causal chain:
Evidence:
Suggestion: Rebuild the vmaas-helm cluster snapshot with a valid osac-operator image reference. The snapshot build process should verify that all referenced image tags actually exist in their registries before publishing the snapshot. Alternatively, use immutable flavor tags (e.g., vmaas-helm-20260625-v3) instead of mutable tags to avoid mid-day snapshot changes breaking CI runs. Prow job | Build For deeper investigation, use the |
|
/retest |
4 participants
Summary
create instancetypeCLI command with--name,--cores,--memory-gib,--descriptionflags using the private API for admin-only creationdescribe instancetypeCLI command using the public API withlookup.Find, rendering base fields (name, cores, memory, state, description) and conditional deprecation data (replacement, deprecated at, obsolete at)INSTANCE_TYPE_STATE_prefix; timestamps formatted as RFC3339Test plan
go build ./...passesginkgo run internal/cmd/cli/create/instancetype— 5 specs pass (flag registration)ginkgo run internal/cmd/cli/describe/instancetype— 6 specs pass (rendering: base fields, state stripping, conditional deprecation, RFC3339 timestamps, description presence/absence)ginkgo run -r internal— full unit test suite passes (no regressions)🤖 Generated with Claude Code
Summary by CodeRabbit
create instancetypeCLI command to create instance types with--name,--cores,--memory-gib, and--description.describe instancetypeCLI command to display instance type details by ID or name, including cores, memory, state, description, and conditional deprecation info with RFC3339 timestamps.create instancetypeflag/usage behavior anddescribe instancetypeoutput rendering (state formatting, description, and deprecation sections).