MGMT-23549: Unable to recover from duplicate/ invalid manifest#3482
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgyselov: This pull request references MGMT-23549 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 bug to target the "4.22.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. |
openshift-ci
bot
added
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@jgyselov: This pull request references MGMT-23549 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 bug to target the "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/components/CustomManifestsArray.tsx (1)
73-80:⚠️ Potential issue | 🟠 MajorSplit delete failure handling from UI-settings update failure.
At Line 73,
updateUISettingsis inside the sametryas the backend delete. If delete succeeds but UI-settings update fails, the alert at Line 77 says deletion failed (incorrect), andonConfirmstill removes the form row, whilecustomManifestsUpdatedmay stay unset. That can leave install-state gating stale.💡 Proposed fix
- const removeManifest = React.useCallback( - async (clusterId: string, manifestIdx: number) => { + const removeManifest = React.useCallback( + async (clusterId: string, manifestIdx: number): Promise<boolean> => { const manifestToRemove = field.value[manifestIdx]; if ((manifestToRemove['folder'] as string) !== '' && manifestToRemove['filename'] !== '') { try { await ClustersAPI.removeCustomManifest( clusterId, manifestToRemove['folder'] as string, manifestToRemove['filename'], ); - await updateUISettings({ customManifestsUpdated: true }); } catch (e) { handleApiError(e, () => addAlert({ title: 'Manifest could not be deleted', message: getApiErrorMessage(e), }), ); + return false; + } + + try { + await updateUISettings({ customManifestsUpdated: true }); + } catch (e) { + handleApiError(e, () => + addAlert({ + title: 'Manifest deleted, but failed to refresh UI state', + message: getApiErrorMessage(e), + }), + ); } } + return true; }, [addAlert, field.value, updateUISettings], );const onConfirm = React.useCallback(async (): Promise<void> => { if (manifestIdxToRemove !== null) { - await removeManifest(clusterId, manifestIdxToRemove); - remove(manifestIdxToRemove); + const shouldRemoveFromForm = await removeManifest(clusterId, manifestIdxToRemove); + if (shouldRemoveFromForm) { + remove(manifestIdxToRemove); + } setManifestIdxToRemove(null); } }, [removeManifest, clusterId, remove, manifestIdxToRemove, setManifestIdxToRemove]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/components/CustomManifestsArray.tsx` around lines 73 - 80, The backend delete and the UI-settings update are in the same try/catch causing delete-success + update-failure to be reported as a deletion failure; separate them: keep the backend delete call in its own try/catch and on success proceed to a second try/catch that calls updateUISettings({ customManifestsUpdated: true }); if the delete try fails, use handleApiError(...) with addAlert titled "Manifest could not be deleted" and message from getApiErrorMessage; if the UI-settings update try fails, call handleApiError(...) with a distinct alert (e.g., "Manifest deleted but UI settings update failed") and the API error message, and ensure removal of the form row or local state update is performed only based on the actual delete success logic (not silently rolled back by the UI-update error).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/components/CustomManifestsArray.tsx`:
- Around line 73-80: The backend delete and the UI-settings update are in the
same try/catch causing delete-success + update-failure to be reported as a
deletion failure; separate them: keep the backend delete call in its own
try/catch and on success proceed to a second try/catch that calls
updateUISettings({ customManifestsUpdated: true }); if the delete try fails, use
handleApiError(...) with addAlert titled "Manifest could not be deleted" and
message from getApiErrorMessage; if the UI-settings update try fails, call
handleApiError(...) with a distinct alert (e.g., "Manifest deleted but UI
settings update failed") and the API error message, and ensure removal of the
form row or local state update is performed only based on the actual delete
success logic (not silently rolled back by the UI-update error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ffb7dc4-05f8-4e8c-b343-7bab16dad1aa
📒 Files selected for processing (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/manifestsConfiguration/components/CustomManifestsArray.tsx
jgyselov
force-pushed
the
ui_settings_for_manifest_remove
branch
from
24abdf8 to
a81ad95
Compare
|
@jgyselov: This pull request references MGMT-23549 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 bug to target the "4.22.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. |
|
/cherry-pick releases/v2.51 |
|
@jgyselov: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, LiorSoffer 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 |
openshift-merge-bot
bot
merged commit c02338a
into
openshift-assisted:master
|
@jgyselov: new pull request created: #3485 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 kubernetes-sigs/prow repository. |
4 participants
https://redhat.atlassian.net/browse/MGMT-23549
Summary by CodeRabbit