Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: [WD-14769] Move custom storage volume to another storage pool. #916

Merged
merged 1 commit into from
Sep 26, 2024
Merged

feat: [WD-14769] Move custom storage volume to another storage pool. #916

merged 1 commit into from
Sep 26, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Sep 19, 2024 •
edited
Loading

Done

  • Implemented a button that opens a modal to migrate Custom storage volumes between different storage pools.
  • Used Segmented-Controls css/ui for the Custom storage volume detail page.
  • Created api endpoint 'migrateStorageVolume'.
  • Added test for storage volume migration in storage.spec.ts.

Fixes [list issues/bugs if needed]

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Navigate to a custom storage volume and view the Migrate button at the top-right of the page.
    • Attempt to change the storage pool of the volume.

Screenshots

image
image
image

@Kxiru Kxiru requested review from edlerd and mas-who September 19, 2024 22:41
@Kxiru Kxiru self-assigned this Sep 19, 2024
@webteam-app
Copy link

Demo

Jenkins

demos.haus

@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch 2 times, most recently from ae56576 to 5e2168f Compare September 19, 2024 23:48
@Kxiru Kxiru changed the title feat: [WD-14769] Move custom storage volume to aother storage pool. feat: [WD-14769] Move custom storage volume to another storage pool. Sep 19, 2024
edlerd
edlerd reviewed Sep 20, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA LGTM, good work :)
Small improvement ideas for code, copy or suggestions to show more information below.

src/api/storage-pools.tsx Show resolved Hide resolved
src/pages/storage/MigrateVolumeBtn.tsx Outdated Show resolved Hide resolved
src/pages/storage/MigrateVolumeBtn.tsx Outdated Show resolved Hide resolved
src/pages/storage/MigrateVolumeBtn.tsx Outdated Show resolved Hide resolved
src/pages/storage/MigrateVolumeModal.tsx Show resolved Hide resolved
src/pages/storage/MigrateVolumeModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/MigrateVolumeModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/MigrateVolumeModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/forms/StorageVolumeFormMain.tsx Show resolved Hide resolved
src/pages/storage/forms/StorageVolumeFormMain.tsx Outdated Show resolved Hide resolved
@mas-who mas-who mentioned this pull request Sep 23, 2024
Merged
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch from 5e2168f to 24a1da1 Compare September 23, 2024 12:15
@Kxiru Kxiru requested a review from edlerd September 23, 2024 12:16
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch 3 times, most recently from 5d82183 to c6c719f Compare September 23, 2024 15:05
edlerd
edlerd reviewed Sep 23, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue with the redirect and one question on changing the form field ordering.

Also I think we should be more consistent in calling it "pool" vs "storage pool" and "volume" vs "storage volume".

src/pages/storage/forms/StorageVolumeFormMain.tsx Show resolved Hide resolved
src/pages/storage/MigrateVolumeBtn.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru requested a review from edlerd September 23, 2024 17:08
edlerd
edlerd reviewed Sep 23, 2024
View reviewed changes
src/pages/storage/MigrateVolumeBtn.tsx Outdated Show resolved Hide resolved
src/pages/storage/MigrateVolumeModal.tsx Outdated Show resolved Hide resolved
tests/helpers/storageVolume.ts Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch 3 times, most recently from 18d9657 to c86a1ad Compare September 24, 2024 11:54
@Kxiru Kxiru requested a review from edlerd September 24, 2024 13:06
edlerd
edlerd reviewed Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor issues, then this should be good to go.

src/pages/storage/MigrateVolumeBtn.tsx Outdated Show resolved Hide resolved
src/pages/storage/MigrateVolumeBtn.tsx Outdated Show resolved Hide resolved
@mas-who mas-who mentioned this pull request Sep 24, 2024
Merged
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch from c86a1ad to 46132dd Compare September 24, 2024 17:42
@Kxiru Kxiru requested a review from edlerd September 24, 2024 17:43
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch from 46132dd to c78a6ab Compare September 24, 2024 17:46
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch 3 times, most recently from 20dbc30 to 2cf1e79 Compare September 24, 2024 18:02
@piperdeck
Copy link

Please see my comment on PR 917

edlerd
edlerd reviewed Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA and code LGTM mostly, just one thing left with giving a better experience in the error case.

src/pages/storage/MigrateVolumeModal.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch from 2cf1e79 to 8c7b681 Compare September 25, 2024 12:01
@Kxiru Kxiru requested a review from edlerd September 25, 2024 12:01
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch 2 times, most recently from b19f296 to 0444d13 Compare September 26, 2024 15:21
@Kxiru Kxiru requested a review from edlerd September 26, 2024 15:22
@Kxiru Kxiru force-pushed the move-custom-storage-volume-to-another-storage-pool branch from 0444d13 to 7ee9c21 Compare September 26, 2024 15:22
edlerd
edlerd approved these changes Sep 26, 2024
View reviewed changes
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Kxiru Kxiru merged commit e06ffbe into canonical:main Sep 26, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 26, 2024
workflow: remove all reports for PR #916
a867850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edlerd edlerd edlerd approved these changes

@mas-who mas-who Awaiting requested review from mas-who

Assignees

@Kxiru Kxiru

Labels
Review: Code +1 Review: Design needed Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Kxiru @webteam-app @piperdeck @edlerd