OSAC-3781: Add lifecycle and delete row actions - #134
Conversation
|
@batzionb: This pull request references OSAC-3781 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 story 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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughInstance-type administration now supports lifecycle state changes and deletion through row action menus, confirmation dialogs, API mutations, query invalidation, localized errors, and integration tests. ChangesInstance-type management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Admin as AdminInstanceTypeActionsMenu
participant Lifecycle as useInstanceTypeLifecycleAction
participant Update as useUpdateInstanceType
participant Query as InstanceTypes query
Admin->>Lifecycle: Select lifecycle action
Lifecycle->>Update: Submit target state
Update->>Query: Invalidate instance-type queries
Query-->>Admin: Refresh instance-type list
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@libs/ui-components/src/api/v1/private/instance-type.test.ts`:
- Line 113: Remove the duplicate object declarations in the affected tests,
keeping exactly one declaration at each scope around the captured object
assertions. Preserve the existing type annotation and usages while resolving the
block-scoped redeclaration errors.
In
`@libs/ui-components/src/components/InstanceType/AdminInstanceTypeListPage.test.tsx`:
- Line 204: In the test scope containing the captured object assertion, remove
the duplicate declarations of object and retain a single typed declaration for
the captured object, avoiding the block-scoped redeclaration error.
🪄 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: 7eb9d07c-3ad2-4ec2-b10f-0a7927e54de6
📒 Files selected for processing (12)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/api/v1/private/instance-type.test.tslibs/ui-components/src/api/v1/private/instance-type.tslibs/ui-components/src/components/InstanceType/AdminInstanceTypeActionsMenu.test.tsxlibs/ui-components/src/components/InstanceType/AdminInstanceTypeActionsMenu.tsxlibs/ui-components/src/components/InstanceType/AdminInstanceTypeListPage.test.tsxlibs/ui-components/src/components/InstanceType/AdminInstanceTypeTable.tsxlibs/ui-components/src/components/InstanceType/InstanceTypeDeleteConfirmModal.tsxlibs/ui-components/src/components/InstanceType/instanceTypeLifecycleErrorTitle.test.tslibs/ui-components/src/components/InstanceType/instanceTypeLifecycleErrorTitle.tslibs/ui-components/src/components/InstanceType/useInstanceTypeLifecycleAction.tslibs/ui-components/src/test-utils/createMockConnectTransport.ts
| reactivate: InstanceTypeState.ACTIVE, | ||
| }; | ||
|
|
||
| const buildLifecycleStateBody = ( |
There was a problem hiding this comment.
this function doesnt contain any logic
There was a problem hiding this comment.
Agreed — this collapses once the hook takes a body directly (see the reply on the next comment).
| action: InstanceTypeLifecycleAction; | ||
| }; | ||
|
|
||
| export const useUpdateInstanceTypeState = () => { |
There was a problem hiding this comment.
instead of having a hook that handles single type of update (spec.status), i'd prefer to have a hook that handles updating the whole spec - ie useUpdateInstanceType that accepts
{
id: string;
body: MessageInitShape<typeof InstanceTypeSchema>;
}
The caller that passes the body that it wants to update. This should be a general pattern in our api hooks.
There was a problem hiding this comment.
Agreed — checked the other hooks and the generic { id, body } shape is actually the dominant pattern already (useUpdateIdentityProvider, useUpdateSecurityGroup, useUpdateStorageTier, useUpdateStorageBackend). Only usePatchComputeInstance uses the action-parameterized style this PR had mirrored. Switched useUpdateInstanceTypeState to a generic useUpdateInstanceType({ id, body }); the action→state mapping now lives in useInstanceTypeLifecycleAction, its only caller.
|
|
||
| import type { InstanceTypeLifecycleAction } from '../../api/v1/private/instance-type'; | ||
|
|
||
| export const getInstanceTypeLifecycleErrorTitle = ( |
There was a problem hiding this comment.
could we move this into useInstanceTypeLifecycleAction ? I dont think there will be any other consumer to justify having this function exported
There was a problem hiding this comment.
Agreed, no other consumer — moved it into useInstanceTypeLifecycleAction.ts as a local (unexported) helper. Deleted instanceTypeLifecycleErrorTitle.ts and its dedicated test, and added an equivalent behavioral test (obsolete-failure toast title) to AdminInstanceTypeActionsMenu.test.tsx so all three lifecycle failure titles stay covered through the public interface.
| const { addToast } = useToast(); | ||
| const updateInstanceType = useUpdateInstanceType(); | ||
|
|
||
| const runLifecycleAction = (instanceTypeId: string, action: InstanceTypeLifecycleAction) => { |
There was a problem hiding this comment.
one more question, why not simply
| const runLifecycleAction = (instanceTypeId: string, action: InstanceTypeLifecycleAction) => { | |
| const runLifecycleAction = (instanceTypeId: string, action: InstanceTypeState) => { |
do we need custom InstanceTypeLifecycleAction ?
There was a problem hiding this comment.
Good call — switched to InstanceTypeState directly: InstanceTypeLifecycleAction is now Exclude<InstanceTypeState, InstanceTypeState.UNSPECIFIED>, so it stays narrower than the raw enum (UNSPECIFIED still isn't a valid action) while dropping the LIFECYCLE_ACTION_STATE lookup table entirely, since the action is the target state now.
Add useUpdateInstanceTypeState and useDeleteInstanceType, parameterized by an InstanceTypeLifecycleAction union mirroring usePatchComputeInstance, plus mock transport support for the InstanceTypes Update/Delete RPCs. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
…mation Add AdminInstanceTypeActionsMenu with state-gated Deprecate/Obsolete/ Reactivate/Delete row actions: lifecycle transitions mutate directly and surface failures via toast (mirroring useVmPowerAction), while Delete is gated behind a confirm modal with an inline error (mirroring VmDeleteConfirmModal). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
…pe table Add an Actions column rendering AdminInstanceTypeActionsMenu per row, and add integration tests verifying the update/delete request sent from the row menu and that the admin instance types list re-fetches after a successful mutation. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Treat an unset (UNSPECIFIED) instance type state the same as ACTIVE when gating row actions, matching the backend, which always defaults state to ACTIVE on creation. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Replace the action-parameterized useUpdateInstanceTypeState with a
generic useUpdateInstanceType({ id, body }), matching the dominant
existing pattern (useUpdateIdentityProvider, useUpdateSecurityGroup,
useUpdateStorageTier, useUpdateStorageBackend) instead of the
compute-instance power-action outlier this PR had mirrored. The
action->state mapping and error-title lookup move into
useInstanceTypeLifecycleAction, the hook's only consumer, removing
the now-pointless single-line buildLifecycleStateBody wrapper and the
separately exported instanceTypeLifecycleErrorTitle module.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: batzionb <brotman@redhat.com>
Completes the previous commit — the source changes for the useUpdateInstanceType generalization were left unstaged. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Drop the string-literal InstanceTypeLifecycleAction union in favor of Exclude<InstanceTypeState, InstanceTypeState.UNSPECIFIED>, removing the LIFECYCLE_ACTION_STATE lookup table entirely — the action is the target state. Excluding UNSPECIFIED keeps the type narrower than the raw enum, so it can't represent a non-action, and getLifecycleErrorTitle's switch stays exhaustively checked. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
abe4ffd to
f25d7b8
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb, rawagner 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 |
3 participants
OSAC-3781: Add lifecycle and delete row actions
Jira: OSAC-3781
Story type: [UI]
Depends on #129 (OSAC-3780) — already merged, so this PR
targets
maindirectly.Summary
Adds per-row lifecycle actions (Deprecate, Obsolete, Reactivate) and a
state-gated Delete action to the provider-admin instance type list page,
following the same state-machine and error-handling contracts as the VM
power-action and delete precedents already in the codebase.
Changes
libs/ui-components/src/api/v1/private/instance-type.ts): addedInstanceTypeLifecycleActionunion,useUpdateInstanceTypeState(single mutation parameterized by action, mirroringusePatchComputeInstance),useDeleteInstanceType, and a sharedinvalidateInstanceTypesQuerieshelper.libs/ui-components/src/components/InstanceType/): newAdminInstanceTypeActionsMenu(kebab menu, action availability computed from current state),InstanceTypeDeleteConfirmModal(delete confirmation with inline error alert),useInstanceTypeLifecycleAction+instanceTypeLifecycleErrorTitle(toast-based failure surfacing for lifecycle actions, mirroringuseVmPowerAction).AdminInstanceTypeTable.tsx): added an Actions column rendering the new per-row menu.libs/ui-components/src/test-utils/createMockConnectTransport.ts): addedupdate/delete/listoverride support for the privateInstanceTypesmock service.Testing
.artifacts/implement/OSAC-3781/05-validation-report.md.Acceptance Criteria
mainafter this story's design doc was written, for consistency with VM power actions; delete failures use an inline alert in the confirm modal with the exact required title.)Video
video.webm
Summary by CodeRabbit