RFE-9410: Add UI option to preserve managed clusters during cluster deletion from ACM#6482
Open
mihirlele wants to merge 2 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mihirlele The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe PR adds a preserve-infrastructure option to bulk and single-cluster destruction flows. The option updates modal UI, action wiring, Hive ClusterDeployment patching, deletion handling, and tests for supported and excluded cluster types. ChangesPreserve Infrastructure Deletion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BulkActionModal
participant deleteCluster
participant ClusterDeploymentAPI
participant deleteResources
User->>BulkActionModal: Select preserve infrastructure
BulkActionModal->>deleteCluster: Submit preserveOnDelete=true
deleteCluster->>ClusterDeploymentAPI: Patch spec.preserveOnDelete=true
ClusterDeploymentAPI-->>deleteCluster: Patch completed
deleteCluster->>deleteResources: Delete cluster resources
deleteResources-->>BulkActionModal: Return deletion result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
frontend/src/components/BulkActionModal.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. frontend/src/components/Clusters/ClustersTableHelper.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. frontend/src/lib/delete-cluster.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
When destroying a Hive-managed cluster, users can now check a "Preserve cluster infrastructure" checkbox to set preserveOnDelete on the ClusterDeployment before deletion. This tells Hive to skip cloud resource deprovisioning, useful for backup/restore and DR workflows. Includes warning alert about ongoing charges, dynamic button relabel, updated modal copy for destroy and detach flows, and unit tests for the new preserve logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mihir Lele <mlele@redhat.com>
mihirlele
force-pushed
the
claude/feature/preserve-cluster-infra-on-destroy
branch
from
July 16, 2026 08:31
1e3f472 to
1c74e2d
Compare
![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)
frontend/src/lib/delete-cluster.test.ts (1)
609-655: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage proving a failed patch aborts deletion.
The key safety contract is that deletion must not start when the preserve patch fails. Add a failing
nockPatch, await rejection fromdeleteCluster(...).promise, and assert the delete nocks remain pending.🤖 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 `@frontend/src/lib/delete-cluster.test.ts` around lines 609 - 655, The deleteCluster tests lack coverage for patch failure preventing deletion. Add a test near the preserveOnDelete cases using a failing nockPatch, await rejection from deleteCluster(...).promise, and verify the delete nocks remain pending rather than being consumed.
🤖 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 `@frontend/src/components/BulkActionModal.tsx`:
- Around line 22-29: Update the AlertProps import in BulkActionModal.tsx to be
type-only, keeping the existing runtime imports from `@patternfly/react-core`
unchanged.
- Around line 219-231: Add an accessible name to the icon-only Button rendering
HelpIcon by setting aria-label with t('More info'), remove the inline padding
style, and pass the icon through PatternFly’s icon prop while preserving the
existing Popover behavior.
In `@frontend/src/components/Clusters/ClustersTableHelper.tsx`:
- Line 775: Gate preserve-on-delete using the exact predicate honored by
deleteCluster: cluster.isHive && !cluster.isHypershift &&
!cluster.isHostedCluster. Update ClusterActionDropdown’s option visibility and
ClustersTableHelper’s bulk-destroy configuration so preserveOnDelete is enabled
only when all selected clusters satisfy this predicate; otherwise disable or
filter the option.
---
Nitpick comments:
In `@frontend/src/lib/delete-cluster.test.ts`:
- Around line 609-655: The deleteCluster tests lack coverage for patch failure
preventing deletion. Add a test near the preserveOnDelete cases using a failing
nockPatch, await rejection from deleteCluster(...).promise, and verify the
delete nocks remain pending rather than being consumed.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ae74712-d8bc-4004-88bd-961856e1172e
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (6)
frontend/src/components/BulkActionModal.tsxfrontend/src/components/Clusters/ClustersTableHelper.tsxfrontend/src/lib/delete-cluster.test.tsfrontend/src/lib/delete-cluster.tsfrontend/src/resources/cluster-deployment.tsfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx
Comment on lines
+22
to
29
| Popover, | ||
| Progress, | ||
| ProgressMeasureLocation, | ||
| Stack, | ||
| StackItem, | ||
| Checkbox, | ||
| AlertProps, | ||
| } from '@patternfly/react-core' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the component and inspect the relevant section.
ast-grep outline frontend/src/components/BulkActionModal.tsx --view expanded || true
printf '\n--- file excerpt ---\n'
sed -n '1,120p' frontend/src/components/BulkActionModal.tsx | cat -n
printf '\n--- alertprops usages ---\n'
rg -n "\bAlertProps\b" frontend/src/components/BulkActionModal.tsx frontend/src/components -g '!**/node_modules/**' || true
printf '\n--- type-only import patterns nearby ---\n'
sed -n '1,80p' frontend/src/components/BulkActionModal.tsx | grep -n "import" || trueRepository: stolostron/console
Length of output: 5763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact import statement and any JSX/props related to the other noted concerns.
sed -n '1,80p' frontend/src/components/BulkActionModal.tsx | nl -ba
printf '\n--- later excerpt ---\n'
sed -n '200,260p' frontend/src/components/BulkActionModal.tsx | nl -baRepository: stolostron/console
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('frontend/src/components/BulkActionModal.tsx')
text = path.read_text()
lines = text.splitlines()
for i, line in enumerate(lines[:90], 1):
if 'AlertProps' in line or 'import' in line:
print(f"{i}: {line}")
print("\n--- context 1-90 ---")
for i, line in enumerate(lines[:90], 1):
print(f"{i:>4}: {line}")
PYRepository: stolostron/console
Length of output: 4095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba frontend/src/components/BulkActionModal.tsx | sed -n '1,80p'
printf '\n---\n'
nl -ba frontend/src/components/BulkActionModal.tsx | sed -n '210,250p'Repository: stolostron/console
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba frontend/src/components/BulkActionModal.tsx | sed -n '1,80p'
printf '\n---\n'
nl -ba frontend/src/components/BulkActionModal.tsx | sed -n '210,250p'Repository: stolostron/console
Length of output: 196
Import AlertProps as a type-only dependency. It’s only used in a type position, so switch to import type to avoid a runtime import.
🤖 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 `@frontend/src/components/BulkActionModal.tsx` around lines 22 - 29, Update the
AlertProps import in BulkActionModal.tsx to be type-only, keeping the existing
runtime imports from `@patternfly/react-core` unchanged.
Source: Coding guidelines
Comment on lines
+219
to
+231
| label={ | ||
| <span> | ||
| {t('Preserve cluster infrastructure')}{' '} | ||
| <Popover | ||
| bodyContent={t( | ||
| 'preserve.cluster.infrastructure.tooltip', | ||
| 'Removes the cluster from the hub without deprovisioning OpenShift or cloud provider resources. Useful for backup, restore, or disaster recovery workflows.' | ||
| )} | ||
| > | ||
| <Button variant="plain" isInline style={{ padding: 0 }}> | ||
| <HelpIcon /> | ||
| </Button> | ||
| </Popover> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/src/components/BulkActionModal.tsx"
echo "=== File size ==="
wc -l "$FILE"
echo "=== Relevant excerpt ==="
sed -n '190,250p' "$FILE"
echo "=== Search for same HelpIcon/button pattern in repo ==="
rg -n "HelpIcon|aria-label|isInline style=\{\{ padding: 0 \}\}" frontend/src/components frontend/src -g '*.tsx' -g '*.ts'Repository: stolostron/console
Length of output: 33814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/src/components/BulkActionModal.tsx"
echo "=== Relevant excerpt ==="
sed -n '210,240p' "$FILE"
echo "=== PatternFly Button references in repo ==="
rg -n "<Button|variant=\"plain\"|aria-label=" frontend/src -g '*.tsx' -g '*.ts' | sed -n '1,120p'Repository: stolostron/console
Length of output: 14078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '20,50p' frontend/src/components/TemplateEditor/controls/ControlPanelFormGroup.tsx
echo
sed -n '1450,1490p' frontend/src/components/AcmDataForm.tsxRepository: stolostron/console
Length of output: 2370
Add an accessible name to the help button. This icon-only Button needs aria-label={t('More info')}; remove style={{ padding: 0 }} and use PatternFly’s icon prop instead of an inline spacing override.
🤖 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 `@frontend/src/components/BulkActionModal.tsx` around lines 219 - 231, Add an
accessible name to the icon-only Button rendering HelpIcon by setting aria-label
with t('More info'), remove the inline padding style, and pass the icon through
PatternFly’s icon prop while preserving the existing Popover behavior.
Source: Coding guidelines
| confirmText: t('confirm'), | ||
| isValidError: errorIsNot([ResourceErrorCode.NotFound]), | ||
| enableDeletePullSecret: true, | ||
| enablePreserveOnDelete: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files frontend/src/components/Clusters/ClustersTableHelper.tsx frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsxRepository: stolostron/console
Length of output: 310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ClustersTableHelper.tsx (around preserve-on-delete) ==\n'
rg -n -C 8 'enablePreserveOnDelete|deleteCluster|preserveOnDelete' frontend/src/components/Clusters/ClustersTableHelper.tsx
printf '\n== ClusterActionDropdown.tsx (around preserve-on-delete) ==\n'
rg -n -C 10 'enablePreserveOnDelete|deleteCluster|preserveOnDelete|cluster\.hive|isHive' frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx
printf '\n== deleteCluster definition/usages ==\n'
rg -n -C 10 'function deleteCluster|const deleteCluster|deleteCluster\(' frontend/srcRepository: stolostron/console
Length of output: 27177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== delete-cluster.ts (preserve logic) ==\n'
rg -n -C 20 'preserveOnDelete|clusterDestroyable|isHypershift|cluster\.hive|HostedCluster|ClusterDeployment' frontend/src/lib/delete-cluster.ts
printf '\n== clusterDestroyable helper ==\n'
rg -n -C 20 'function clusterDestroyable|const clusterDestroyable|export .*clusterDestroyable' frontend/src/routes/Infrastructure/Clusters/ManagedClusters/utils/cluster-actions.ts
printf '\n== bulk action modal usage in ClusterActionDropdown.tsx ==\n'
rg -n -C 12 'enablePreserveOnDelete|emptyState|items: \[cluster\]|clusterDestroyable|isValid|selected' frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx
printf '\n== bulk action modal usage in ClustersTableHelper.tsx ==\n'
rg -n -C 12 'enablePreserveOnDelete|emptyState|items:|clusterDestroyable|isValid|selected|destroy' frontend/src/components/Clusters/ClustersTableHelper.tsxRepository: stolostron/console
Length of output: 30923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== delete-cluster.ts (preserve logic) ==\n'
sed -n '1,220p' frontend/src/lib/delete-cluster.ts
printf '\n== cluster-actions.ts ==\n'
sed -n '1,260p' frontend/src/routes/Infrastructure/Clusters/ManagedClusters/utils/cluster-actions.tsRepository: stolostron/console
Length of output: 11880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== delete-cluster.ts (first 220 lines) ==\n'
cat -n frontend/src/lib/delete-cluster.ts | sed -n '1,220p'
printf '\n== cluster-actions.ts (first 260 lines) ==\n'
cat -n frontend/src/routes/Infrastructure/Clusters/ManagedClusters/utils/cluster-actions.ts | sed -n '1,260p'Repository: stolostron/console
Length of output: 14370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== delete-cluster.ts (preserve logic) ==\n'
rg -n -C 25 'preserveOnDelete|clusterDestroyable|isHypershift|cluster\.hive|HostedCluster|ClusterDeployment' frontend/src/lib/delete-cluster.ts
printf '\n== cluster-actions.ts (destroy eligibility) ==\n'
rg -n -C 25 'clusterDestroyable|isHypershift|isHive|clusterPool|hosted|preserveOnDelete' frontend/src/routes/Infrastructure/Clusters/ManagedClusters/utils/cluster-actions.ts
printf '\n== bulk action modal configuration in ClusterActionDropdown.tsx ==\n'
rg -n -C 20 'BulkActionModal|enablePreserveOnDelete|emptyState|items: \[cluster\]|clusterDestroyable|isValid|enableDeletePullSecret|actionWhenPreserve' frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx
printf '\n== bulk action modal configuration in ClustersTableHelper.tsx ==\n'
rg -n -C 20 'BulkActionModal|enablePreserveOnDelete|emptyState|items:|clusterDestroyable|isValid|enableDeletePullSecret|actionWhenPreserve' frontend/src/components/Clusters/ClustersTableHelper.tsxRepository: stolostron/console
Length of output: 42668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== usages of cluster.isHive and cluster.hive.clusterPool ==\n'
rg -n -C 4 'isHive|clusterPool|isHypershift|hive\?' frontend/src | head -n 200
printf '\n== delete-cluster.test.ts preserve/hypershift cases ==\n'
rg -n -C 8 'preserveOnDelete|hypershift|Hosted cluster|Hive cluster|clusterPool' frontend/src/lib/delete-cluster.test.ts
printf '\n== cluster type shape ==\n'
rg -n -C 8 'type Cluster|interface Cluster' frontend/src/resources/utils.ts frontend/src/resources/*.ts frontend/src/resources/**/*.tsRepository: stolostron/console
Length of output: 46525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== delete-cluster.ts relevant section ==\n'
nl -ba frontend/src/lib/delete-cluster.ts | sed -n '1,180p'Repository: stolostron/console
Length of output: 238
Gate preserve-on-delete on the same Hive predicate.
frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx#L415:!cluster.hive?.clusterPoolstill exposes the option for destroyable non-Hive/Hypershift clusters, butdeleteClusteronly honorspreserveOnDeleteforcluster.isHive && !cluster.isHypershift && !cluster.isHostedCluster.frontend/src/components/Clusters/ClustersTableHelper.tsx#L775: bulk destroy enables preserve unconditionally; disable or filter it when the selection includes clusters outside that predicate.
📍 Affects 2 files
frontend/src/components/Clusters/ClustersTableHelper.tsx#L775-L775(this comment)frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx#L415-L415
🤖 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 `@frontend/src/components/Clusters/ClustersTableHelper.tsx` at line 775, Gate
preserve-on-delete using the exact predicate honored by deleteCluster:
cluster.isHive && !cluster.isHypershift && !cluster.isHostedCluster. Update
ClusterActionDropdown’s option visibility and ClustersTableHelper’s bulk-destroy
configuration so preserveOnDelete is enabled only when all selected clusters
satisfy this predicate; otherwise disable or filter the option.
Match the guard in deleteCluster: cluster.isHive && !cluster.isHypershift && !cluster.isHostedCluster. Single destroy no longer exposes the option for non-Hive destroyable clusters, and bulk destroy only shows it when at least one selected cluster satisfies the predicate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Mihir Lele <mlele@redhat.com>
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 `@frontend/src/lib/delete-cluster.ts`:
- Around line 148-169: Update resolveDeleteResult to inspect every entry in
promisesSettledResult, rejecting on any rejected deletion result. Preserve the
special case that ignores only ResourceError NotFound failures when
ignoreClusterDeploymentNotFound applies to the ClusterDeployment, and ensure
non-ResourceError rejections—including at index 0—are propagated.
In
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx`:
- Around line 415-416: Update the preservation-control configuration near
enablePreserveOnDelete and actionWhenPreserve to perform a separate
ClusterDeployment PATCH permission check using createSubjectAccessReview with
ResourceAttributes. Disable or hide only preservation when PATCH access is
unavailable, while keeping ordinary destruction gated solely by the existing
delete permission.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 921a80be-b296-4eb4-a847-1ee31408f5f3
⛔ Files ignored due to path filters (1)
frontend/public/locales/en/translation.jsonis excluded by!frontend/public/locales/**
📒 Files selected for processing (6)
frontend/src/components/BulkActionModal.tsxfrontend/src/components/Clusters/ClustersTableHelper.tsxfrontend/src/lib/delete-cluster.test.tsfrontend/src/lib/delete-cluster.tsfrontend/src/resources/cluster-deployment.tsfrontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/resources/cluster-deployment.ts
- frontend/src/lib/delete-cluster.test.ts
- frontend/src/components/Clusters/ClustersTableHelper.tsx
- frontend/src/components/BulkActionModal.tsx
Comment on lines
+148
to
+169
| function resolveDeleteResult(deleteResourcesResult: ReturnType<typeof deleteResources>) { | ||
| return { | ||
| promise: new Promise((resolve, reject) => { | ||
| deleteResourcesResult.promise.then((promisesSettledResult) => { | ||
| if (promisesSettledResult[0]?.status === 'rejected') { | ||
| const error = promisesSettledResult[0].reason | ||
| if (error instanceof ResourceError) { | ||
| if (ignoreClusterDeploymentNotFound && error.code === ResourceErrorCode.NotFound) { | ||
| // DO NOTHING | ||
| } else { | ||
| reject(promisesSettledResult[0].reason) | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if (promisesSettledResult[1]?.status === 'rejected') { | ||
| reject(promisesSettledResult[1].reason) | ||
| return | ||
| } | ||
| resolve(promisesSettledResult) | ||
| }) | ||
| }), | ||
| abort: deleteResourcesResult.abort, | ||
| if (promisesSettledResult[1]?.status === 'rejected') { | ||
| reject(promisesSettledResult[1].reason) | ||
| return | ||
| } | ||
| resolve(promisesSettledResult) | ||
| }) | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate failures from every deleted resource.
Only indexes 0 and 1 are checked. Failures deleting later resources—such as ClusterClaims, secrets, NodePools, or InfraEnvs—resolve as success. Additionally, a non-ResourceError rejection at index 0 is ignored.
Reject every failed result except an explicitly ignored NotFound for the ClusterDeployment.
Proposed fix
- deleteResourcesResult.promise.then((promisesSettledResult) => {
- if (promisesSettledResult[0]?.status === 'rejected') {
- const error = promisesSettledResult[0].reason
- if (error instanceof ResourceError) {
- if (ignoreClusterDeploymentNotFound && error.code === ResourceErrorCode.NotFound) {
- // DO NOTHING
- } else {
- reject(promisesSettledResult[0].reason)
- return
- }
- }
- }
- if (promisesSettledResult[1]?.status === 'rejected') {
- reject(promisesSettledResult[1].reason)
- return
- }
+ deleteResourcesResult.promise.then((promisesSettledResult) => {
+ const failure = promisesSettledResult.find((result, index) => {
+ if (result.status !== 'rejected') return false
+
+ const resource = resources[index]
+ return !(
+ ignoreClusterDeploymentNotFound &&
+ resource?.apiVersion === ClusterDeploymentApiVersion &&
+ resource?.kind === ClusterDeploymentKind &&
+ result.reason instanceof ResourceError &&
+ result.reason.code === ResourceErrorCode.NotFound
+ )
+ })
+
+ if (failure?.status === 'rejected') {
+ reject(failure.reason)
+ return
+ }
resolve(promisesSettledResult)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function resolveDeleteResult(deleteResourcesResult: ReturnType<typeof deleteResources>) { | |
| return { | |
| promise: new Promise((resolve, reject) => { | |
| deleteResourcesResult.promise.then((promisesSettledResult) => { | |
| if (promisesSettledResult[0]?.status === 'rejected') { | |
| const error = promisesSettledResult[0].reason | |
| if (error instanceof ResourceError) { | |
| if (ignoreClusterDeploymentNotFound && error.code === ResourceErrorCode.NotFound) { | |
| // DO NOTHING | |
| } else { | |
| reject(promisesSettledResult[0].reason) | |
| return | |
| } | |
| } | |
| } | |
| } | |
| if (promisesSettledResult[1]?.status === 'rejected') { | |
| reject(promisesSettledResult[1].reason) | |
| return | |
| } | |
| resolve(promisesSettledResult) | |
| }) | |
| }), | |
| abort: deleteResourcesResult.abort, | |
| if (promisesSettledResult[1]?.status === 'rejected') { | |
| reject(promisesSettledResult[1].reason) | |
| return | |
| } | |
| resolve(promisesSettledResult) | |
| }) | |
| }), | |
| function resolveDeleteResult(deleteResourcesResult: ReturnType<typeof deleteResources>) { | |
| return { | |
| promise: new Promise((resolve, reject) => { | |
| deleteResourcesResult.promise.then((promisesSettledResult) => { | |
| const failure = promisesSettledResult.find((result, index) => { | |
| if (result.status !== 'rejected') return false | |
| const resource = resources[index] | |
| return !( | |
| ignoreClusterDeploymentNotFound && | |
| resource?.apiVersion === ClusterDeploymentApiVersion && | |
| resource?.kind === ClusterDeploymentKind && | |
| result.reason instanceof ResourceError && | |
| result.reason.code === ResourceErrorCode.NotFound | |
| ) | |
| }) | |
| if (failure?.status === 'rejected') { | |
| reject(failure.reason) | |
| return | |
| } | |
| resolve(promisesSettledResult) | |
| }) | |
| }), |
🤖 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 `@frontend/src/lib/delete-cluster.ts` around lines 148 - 169, Update
resolveDeleteResult to inspect every entry in promisesSettledResult, rejecting
on any rejected deletion result. Preserve the special case that ignores only
ResourceError NotFound failures when ignoreClusterDeploymentNotFound applies to
the ClusterDeployment, and ensure non-ResourceError rejections—including at
index 0—are propagated.
Comment on lines
+415
to
+416
| enablePreserveOnDelete: cluster.isHive && !cluster.isHypershift && !cluster.isHostedCluster, | ||
| actionWhenPreserve: t('Destroy and preserve infrastructure'), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Gate preservation on ClusterDeployment patch permission.
destroyRbac checks delete permission only, but selecting this option performs a PATCH. A delete-only user can therefore select preservation and encounter an authorization failure that aborts destruction.
Check patch access separately and disable or hide only the preservation control; do not require patch permission for ordinary destruction.
As per coding guidelines, use createSubjectAccessReview with ResourceAttributes for direct permission checks.
🤖 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
`@frontend/src/routes/Infrastructure/Clusters/ManagedClusters/components/ClusterActionDropdown.tsx`
around lines 415 - 416, Update the preservation-control configuration near
enablePreserveOnDelete and actionWhenPreserve to perform a separate
ClusterDeployment PATCH permission check using createSubjectAccessReview with
ResourceAttributes. Disable or hide only preservation when PATCH access is
unavailable, while keeping ordinary destruction gated solely by the existing
delete permission.
Source: Coding guidelines
Author
|
/retest |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Summary
Ticket Summary (Title):
Add UI option to preserve managed cluster infrastructure during cluster deletion from ACM
Ticket Link:
Type of Change:
What this PR does
When destroying a Hive-managed cluster from the ACM/MCE UI, there was previously no way to preserve the underlying cloud infrastructure — the only workaround was manually patching the
ClusterDeploymentCR via CLI (spec.preserveOnDelete: true) before initiating deletion. This PR surfaces that option directly in the destroy modal.Changes
Destroy modal (single and bulk):
Detach modal (single and bulk):
Backend logic (
deleteCluster):preserveOnDeleteis true, issues a merge-patch to setspec.preserveOnDelete: trueon theClusterDeploymentbefore proceeding with deletionClusterDeploymentwith this field)Files changed
frontend/src/resources/cluster-deployment.tspreserveOnDelete?: booleanto spec interfacefrontend/src/lib/delete-cluster.tspreserveOnDeleteparam, PATCH-before-delete logicfrontend/src/components/BulkActionModal.tsxfrontend/src/routes/.../ClusterActionDropdown.tsxfrontend/src/components/Clusters/ClustersTableHelper.tsxfrontend/public/locales/en/translation.jsonfrontend/src/lib/delete-cluster.test.ts✅ Checklist
General
ACM-12340 Fix bug with...)If Feature
If Bugfix
🗒️ Notes for Reviewers
deleteClusteronly patches non-Hypershift Hive clusters.preserveOnDeletefield is an existing GA field on the HiveClusterDeploymentCRD — no new CRDs or RBAC changes are needed.bulk.message.detachi18n key was replaced bybulk.message.detach.singleandbulk.message.detach.bulkto differentiate the copy. The oldbulk.message.destroykey is still used by the DestroyHosted action (Hypershift) which retains its original copy.Summary by CodeRabbit