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

Add volume list and iso imports #318

Merged
merged 16 commits into from
Aug 30, 2023
Merged

Conversation

edlerd
Copy link
Collaborator

@edlerd edlerd commented Apr 26, 2023

Done

  • Add storage volume list
  • Add import local file to custom volume
  • Add list of images (cached and ISO)

QA Steps

  • launch a new instance and upload a prepared ISO (this needs LXD 5.16 or later, upgrade with snap refresh lxd --channel=latest/candidate)
  • use the iso image for a new instance
  • ensure the instance is launched with a special message (linking to the console tab)
  • ensure instance can't be started on creation directly
  • check the storage detail page
  • check the storage detail page volume tab
  • ensure ISO can be deleted on the volume tab if it is not in use.
  • check list of images
  • try deleting / using a cached image and an iso image

@webteam-app
Copy link

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

@edlerd edlerd force-pushed the add-iso-imports branch 2 times, most recently from ba2729d to 13ea259 Compare April 27, 2023 10:15
@edlerd edlerd force-pushed the add-iso-imports branch 2 times, most recently from 51377f5 to 566c607 Compare May 23, 2023 20:22
@edlerd edlerd force-pushed the add-iso-imports branch 2 times, most recently from bb76cc2 to 96596db Compare July 11, 2023 07:19
@edlerd edlerd force-pushed the add-iso-imports branch 3 times, most recently from 890377d to 6cf462d Compare August 3, 2023 12:51
@lorumic
Copy link
Contributor

lorumic commented Aug 4, 2023

Can you please add the expected QA steps?

@edlerd
Copy link
Collaborator Author

edlerd commented Aug 4, 2023

Added QA steps.

Copy link
Contributor

@lorumic lorumic left a comment

Choose a reason for hiding this comment

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

This is a good start. I'm adding a few suggestions and ideas for improvements.

src/pages/storage/UploadIsoImage.tsx Outdated Show resolved Hide resolved
src/pages/storage/UploadIsoImage.tsx Outdated Show resolved Hide resolved
src/pages/storage/UploadIsoImage.tsx Outdated Show resolved Hide resolved
src/pages/storage/StorageVolumes.tsx Outdated Show resolved Hide resolved
src/pages/storage/StorageVolumes.tsx Outdated Show resolved Hide resolved
src/pages/images/RemoteImageSelector.tsx Outdated Show resolved Hide resolved
src/util/images.tsx Outdated Show resolved Hide resolved
src/pages/storage/StorageDetail.tsx Show resolved Hide resolved
src/pages/instances/CreateInstanceForm.tsx Show resolved Hide resolved
Copy link
Contributor

@lorumic lorumic 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 applying the changes. I've left a few minor comments for further improvements, but it's already looking much better.

src/pages/storage/UploadIsoImageHint.tsx Outdated Show resolved Hide resolved
src/pages/images/ImageSelectorModal.tsx Show resolved Hide resolved
src/pages/storage/UploadIsoImage.tsx Outdated Show resolved Hide resolved
src/types/storage.d.ts Outdated Show resolved Hide resolved
src/pages/images/RemoteImageSelector.tsx Outdated Show resolved Hide resolved
src/types/image.d.ts Outdated Show resolved Hide resolved
@piperdeck
Copy link

Some issues I found:

  1. As we discussed in our last sync, "ISOs" should be referred to as "images" in most parts of the interface, since LXD supports more types of images than just ISOs.

  2. I think the instuctions for pre-processing Windows images should be more contextual. It doesn't make sense to show the message "Image must be prepared with distrobuilder" every time someone goes to upload an image. The user may well be uploading, for example, a linux image in qcow2 format.

Therefore, let's try only showing the message about converting an ISO once the user has chosen a Windows ISO using the file picker. Then, we show the information about converting Windows images, and we tailor the distrobuilder command to reference the specific file path of the image that the user chose with the file picker.

Pasted image 20230814104401

Note:

  • distrobuilder does not, in fact, come with lxd. So the user should be directed to install it.
  • There are a few other dependencies besides distrobuilder that need to be install also. These are outlined in Miona's tutorial.
  • This is a complex design problem. I'll work on this and come up with a more robust design for how this should work.
  1. The Images > Custom ISOs tab could probably use a rename (as noted above), since not all uploaded images will be ISOs. Perhaps:
    • Images
      • Cached
      • Custom

Note that eventually, images created from instance snapshots would live under "Custom", which is why I wouldn't name it "uploaded", or something like that

  1. The Images page should also probably be a place from which to upload a new custom image.

Pasted image 20230814110631

  1. When a windows instance is created, it's easy to miss the pop-up message explaining what to do. Maybe include a CTA button in the pop-up?

Frame 286

@edlerd
Copy link
Collaborator Author

edlerd commented Aug 25, 2023

Thank you for the design review @piperdeck
I realized all the suggestions you mentioned above, and also the additional ones we discussed today, including

  • redesign the "select base image" button / "change image" buttons on instance launch
  • after custom image upload the users goes back to the image list with the new images select button positive

Please have another look and if it is good to go please remove the "design needed" label and add the "design +1" one. Or maybe there are more suggestions? As for the "upload custom image" modal, I'd iterate on it maybe in a separate PR.

Copy link

@piperdeck piperdeck left a comment

Choose a reason for hiding this comment

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

Everything seems good to me. The instance creation flow feels great!

@edlerd edlerd merged commit da698ae into canonical:main Aug 30, 2023
4 checks passed
@edlerd edlerd deleted the add-iso-imports branch August 30, 2023 13:54
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