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.

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

feat(storage) crud for custom volume snapshots [WD-7576] #563

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

mas-who
Copy link
Collaborator

@mas-who mas-who commented Nov 29, 2023

Done

  • removed snapshot volume type from volumes list filter.
  • clicking on volume name will redirect to instance detail page if it's a container or vm volume, redirect to image list page if it's a image volume and redirect to volume details if it's a custom volume.
  • added the snapshots column to the volumes list table. Number of snapshots per volume is calculated based on number of snapshot volumes that matches each volume name.
  • CTA for storage volume list table. For custom storage volumes, user can add snapshots or delete volume. For instance volumes, link to instance detail and for image volumes link to images list. Show CTA only on hover volume row.
  • Generalised snapshot form modal
  • Snapshot form modal will now be rendered using portal
  • Added storage volume snapshot api
  • Create instance and custom volume snapshots using the generalised snapshot form modal
  • Fixed issue with snapshot form modal retaining stale states after snapshot created, this was an issue with instance snapshot as well
  • Implemented snapshots tab for storage volume detail page (heavy reference to the snapshots tab for instnace detail page)
  • Code cleanup
  • Fixed issue with not able to sort by the snapshots column on the storage volume list table
  • Fixed issue with disabling snapshot creation if project is restricted

Review Changes

  • Fixes based on David's comments:
    • Error handling for instance and volume snapshot creation when project is restricted
    • Moved VolumeSnapshotsForm out of the generic component folder
  • Added tests for custom volume snapshot crud
  • More changes for David's review:
    • unique naming for instance and volume snapshot api methods
    • removed PortalModalBtn and created addition button components for readability
    • each file should contain one component
    • moved request to get volume snapshots from StorageVolumeDetail to StorageVolumeSnapshots component
    • removed loadCustomVolumeAndSnapshots.tsx as it is not needed
    • removed unecessary refetchOnMount
    • removed description input for custom storage volume snapshots
    • improved logic for determining if snapshot is disabled for the project
    • split snapshots utils into generic, instance and volume specific files
    • updated LxdSyncResponse interface with generic type
    • handle undefined case for isSnapshotDisabled in utils/snapshots.tsx
    • consistent props destructuring for components throughout
    • removed useInstanceSnapshot and useVolumeSnapshot hooks
    • moved NotificationRow below TabLinks in StorageVolumeDetail.tsx
    • improve small screen viewing for storage volumes table with collapsed columns
    • added disable snapshot creation tooltips for instance and volume snapshots
    • minor styling fixes
    • spelling fix
    • moved generateLinkForVolumeDetail from utils file to StorageVolumeNameLink component

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • Navigate to Storage -> Volumes tab: check that clicking on the name of a Container or VM type volume will redirect you to the relevant instance detail page. For a Image type volume, clicking on the name of the volume will redirect you to the images overview page.
    • On volumes tab, CTAs will now be visible on hover. Only custom storage volumes will have localised CTAs (add snapshot and delete volume). Try add a snapshot for a specific volume by clicking on the add snapshot CTA.
    • On storage volumes tab, create a custom volume then navigate to its details page by clicking on the volume's name. Go to the Snapshots tab and check:
      • create, edit and delete snapshots.
      • configure snapshot settings for the volume by clicking on the See configuration button.

@webteam-app
Copy link

Demo starting at https://lxd-ui-563.demos.haus

src/util/storageVolumeTable.tsx Outdated Show resolved Hide resolved
src/pages/storage/StorageVolumes.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the custom-volume-snapshot-crud branch 13 times, most recently from 53b12b0 to afe4c3f Compare December 6, 2023 10:22
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.

This already works nicely on all the happy paths. I did a bit of QA and found some rough edges, see below comments for details.

src/components/forms/VolumeSnapshotsForm.tsx Outdated Show resolved Hide resolved
src/components/forms/VolumeSnapshotsForm.tsx Outdated Show resolved Hide resolved
src/api/volume-snapshots.tsx Show resolved Hide resolved
src/api/volume-snapshots.tsx Outdated Show resolved Hide resolved
src/components/PortalModalBtn.tsx Outdated Show resolved Hide resolved
src/components/forms/SnapshotForm.tsx Outdated Show resolved Hide resolved
src/pages/storage/StorageVolumeDetail.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceDetail.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceSnapshots.tsx Outdated Show resolved Hide resolved
src/pages/instances/InstanceSnapshots.tsx Outdated Show resolved Hide resolved
src/util/snapshots.tsx Show resolved Hide resolved
src/types/apiResponse.d.ts Outdated Show resolved Hide resolved
src/components/forms/SnapshotForm.tsx Outdated Show resolved Hide resolved
src/util/snapshots.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/CreateInstanceSnapshotForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/EditInstanceSnapshotForm.tsx Outdated Show resolved Hide resolved
src/pages/storage/StorageVolumeDetail.tsx Outdated Show resolved Hide resolved
src/pages/storage/StorageVolumeNameLink.tsx Outdated Show resolved Hide resolved
@piperdeck
Copy link

Everything looks good to me, great work!

@mas-who mas-who force-pushed the custom-volume-snapshot-crud branch 3 times, most recently from be67cc5 to 660ded8 Compare December 8, 2023 09:53
@mas-who mas-who marked this pull request as ready for review December 8, 2023 09:58
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.

Thanks for all the changes, this is looking better and better.
Now, I finished the review with some new discoveries, when those are settled this should be ready to merge.

src/pages/storage/StorageVolumes.tsx Show resolved Hide resolved
src/pages/instances/InstanceSnapshots.tsx Show resolved Hide resolved
src/pages/storage/StorageVolumeSnapshots.tsx Outdated Show resolved Hide resolved
src/sass/_storage.scss Outdated Show resolved Hide resolved
src/sass/_storage.scss Outdated Show resolved Hide resolved
src/sass/styles.scss Outdated Show resolved Hide resolved
src/util/instanceSnapshots.tsx Outdated Show resolved Hide resolved
src/util/storageVolume.tsx Outdated Show resolved Hide resolved
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.

Thanks for the changes.

Some suggestions for the captions, trying to tailor the copy closer to the setting the user has to flip to enable it.

- removed snapshot volume type from volumes list filter.
- clicking on volume name will redirect to instance detail page if it's a container or vm volume, redirect to image list page if it's a image volume and redirect to volume details if it's a custom volume.
- added the snapshots column to the volumes list table. Number of snapshots per volume is calculated based on number of snapshot volumes that matches each volume name.
- CTA for storage volume list table. For custom storage volumes, user can add snapshots or delete volume. For instance volumes, link to instance detail and for image volumes link to images list. Show CTA only on hover volume row.
- Generalised snapshot form modal
- Snapshot form modal will now be rendered using portal
- Added storage volume snapshot api
- Create instance and custom volume snapshots using the generalised snapshot form modal
- Fixed issue with snapshot form modal retaining stale states after snapshot created, this was an issue with instance snapshot as well
- Implemented snapshots tab for storage volume detail page (heavy reference to the snapshots tab for instnace detail page)
- Code cleanup
- Fixed issue with not able to sort by the snapshots column on the storage volume list table
- Fixed issue with disabling snapshot creation if project is restricted
- Fixed tooltip wording for add snapshot CTA button on volume list table
- Fixes based on David's comments:
    - Error handling for instance and volume snapshot creation when project is restricted
    - Moved VolumeSnapshotsForm out of the generic component folder
- Added tests for custom volume snapshot crud
- More changes for David's review:
    - unique naming for instance and volume snapshot api methods
    - removed PortalModalBtn and created addition button components for readability
    - each file should contain one component
    - moved request to get volume snapshots from StorageVolumeDetail to StorageVolumeSnapshots component
    - removed loadCustomVolumeAndSnapshots.tsx as it is not needed
    - removed unecessary refetchOnMount
    - removed description input for custom storage volume snapshots
    - improved logic for determining if snapshot is disabled for the project
    - split snapshots utils into generic, instance and volume specific files
    - updated LxdSyncResponse interface with generic type
    - handle undefined case for isSnapshotDisabled in utils/snapshots.tsx
    - consistent props destructuring for components throughout
    - removed useInstanceSnapshot and useVolumeSnapshot hooks
    - moved NotificationRow below TabLinks in StorageVolumeDetail.tsx
    - improve small screen viewing for storage volumes table with collapsed columns
    - added disable snapshot creation tooltips for instance and volume snapshots
    - minor styling fixes
    - spelling fix
    - moved generateLinkForVolumeDetail from utils file to StorageVolumeNameLink component

Signed-off-by: Mason Hu <[email protected]>
@mas-who mas-who merged commit 225aa1e into main Dec 8, 2023
6 checks passed
@edlerd edlerd deleted the custom-volume-snapshot-crud branch December 8, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants