feat: add update backup target action #830
Conversation
WalkthroughThe pull request introduces comprehensive enhancements to backup and volume management functionality across multiple files. The changes focus on improving backup target updates, introducing new modal components for managing backup targets, and refactoring existing code to support more flexible keyword-based filtering. The modifications span several components and utility functions, enabling more granular control over backup volume operations and providing a more intuitive user interface for managing backup targets. Changes
Sequence DiagramsequenceDiagram
participant User
participant VolumeList
participant UpdateBackupTargetModal
participant VolumeModel
User->>VolumeList: Select Update Backup Target
VolumeList->>UpdateBackupTargetModal: Show Modal
User->>UpdateBackupTargetModal: Select New Backup Target
UpdateBackupTargetModal->>VolumeModel: Dispatch Update Action
VolumeModel-->>UpdateBackupTargetModal: Confirm Update
UpdateBackupTargetModal-->>VolumeList: Close Modal
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
a110605
force-pushed
the
backup_target_2
branch
from
eff3590 to
48c0f2c
Compare
a110605
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| const v1DataEngineEnabledSetting = settings.find(s => s.id === 'v1-data-engine') | ||
| const v2DataEngineEnabledSetting = settings.find(s => s.id === 'v2-data-engine') | ||
| const v1DataEngineEnabled = v1DataEngineEnabledSetting?.value === 'true' | ||
| const v2DataEngineEnabled = v2DataEngineEnabledSetting?.value === 'true' | ||
|
|
||
| const backups = volumeName ? backupData.filter(bk => bk.volumeName === volumeName) : backupData | ||
| sortBackups(backupData) | ||
| const backups = currentBackUp ? backupData.filter(bk => bk.volumeName === currentBackUp.volumeName) : backupData |
There was a problem hiding this comment.
We might need to add the backuptarget filter to the URL as well?
The scenarios are as follows:
- Create a volume with the backup target
defaultand create a backup. (e.g. 1217-test) - Change the backup target to
cifsand create another backup. - On the backup page, both backups appear with their respective targets (expected).
- Clicking the volume with the
defaulttarget showscifsin the details page.
There was a problem hiding this comment.
Another issue found: When clicking Restore Latest Backup and Create Disaster Recovery Volume for 1217-test (backup target: default), the modal doesn’t appear.
There was a problem hiding this comment.
Like the backend bug that the /v1/backupvolumes/bug-test-a98258ed?action=backupList (default) returns cifs backup data. @mantissahz will fix from backend first.
There was a problem hiding this comment.
@houhoucoop , @mantissahz , let's review and merge this PR to add update backup target feature first. We can fix this bug in next PR working with backend change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/dataDependency.js (1)
188-188: Consider performance implications of additional dependencyAdding the 'backup' dependency means that backup-related data will be queried whenever the backing image route is accessed. This could impact performance if the backup data is large and not immediately needed.
Consider:
- Lazy loading the backup data only when needed
- Adding comments explaining why this dependency is necessary
- Documenting any performance impact in the PR description
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/dataDependency.js(1 hunks)
🔇 Additional comments (1)
src/utils/dataDependency.js (1)
188-188: Verify the backup dependency usage for backing images
The addition of 'backup' dependency for '/backingImage' path looks correct, but let's verify that this dependency is actually needed and used in the backing image components.
Let's check the usage:
✅ Verification successful
The backup dependency for backing images is correctly required
The verification shows that the backup dependency is actively used in the backing image components:
- BackupBackingImage.js handles backup-related operations and UI for backing images
- CreateBackupBackingImageModal.js manages backup target selection and backup creation
- Multiple components interact with backup-related data, including backup targets, backup operations, and backup states
The dependency addition is necessary to ensure proper data availability for these backup-related features in the backing image section.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if backing image components use backup-related data
# Search for backup-related data usage in backing image components
echo "Checking backup data usage in backing image components..."
rg -l 'backup' --type js src/routes/backingImage/
# Check if the backing image components dispatch any backup-related actions
echo "Checking backup action dispatches..."
ast-grep --pattern 'dispatch({
type: $_,
payload: {
backup: $_
}
})' src/routes/backingImage/
Length of output: 939
Script:
#!/bin/bash
# Let's check the actual usage of backup data in these files
echo "Checking BackupBackingImage.js for backup data usage..."
rg -A 5 'backup' src/routes/backingImage/BackupBackingImage.js
echo "\nChecking CreateBackupBackingImageModal.js..."
rg -A 5 'backup' src/routes/backingImage/CreateBackupBackingImageModal.js
echo "\nChecking RestoreBackupBackingImageModal.js..."
rg -A 5 'backup' src/routes/backingImage/RestoreBackupBackingImageModal.js
# Check if there are any direct references to backup store/state
echo "\nChecking for backup store/state usage..."
ast-grep --pattern 'backup: state.backup' src/routes/backingImage/
Length of output: 4095
Signed-off-by: andy.lee <[email protected]>
Signed-off-by: andy.lee <[email protected]>
Signed-off-by: andy.lee <[email protected]>
Signed-off-by: andy.lee <[email protected]>
Signed-off-by: andy.lee <[email protected]>
a110605
force-pushed
the
backup_target_2
branch
from
d68cc66 to
aa9a441
Compare
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@mergify backport v1.8.x |
✅ Backports have been created
|
What this PR does / why we need it
Delete All backupsaction in backup detail pagevolumeNamefeild in backup detail page urlIssue
longhorn/longhorn#8647
Test Result
Update volume backup target
volume_update.mov
Bulk update backup target
bulk_update.mov
Fix action in backup detail page
Additional documentation or context
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores