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: Add create instance from snapshot feature [WD-14411] #858

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

mas-who
Copy link
Collaborator

@mas-who mas-who commented Aug 26, 2024

Done

  • Add duplicate snapshot feature

Notes

  • Currently using the copy vanilla icon for the duplicate snapshot button
  • Removed the allowInconsistent and instanceOnly options. These are not relevant for snapshot duplication.
  • Stateful copy is only relevant for stateful snapshots
  • For a stateful copy of a snapshot, the instance name must remain the same. This means the instance cannot be created in the same project. Tested this in the CLI, it's the same behaviour (see code ref). Therefore, I added some additional form validation logic. If the Copy stateful option is checked, then validation is performed on instance name (must be the same as the original instance name) and target project (must be a different project to the source instance project).
    Screenshot from 2024-08-26 13-28-43

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Create an instance
    • Create a stateless snapshot and a stateful snapshot for the instance (migration.stateful must be true and the instance must be running)
    • Try duplicate both snapshots. Pay close attention to the stateful snapshot form validation.

@webteam-app
Copy link

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.

Oh nice, a new feature :)
Some suggestions on naming and copy. I think generally we should call it "Create instance from snapshot". And also update notifications to "Instance created" or "Instance creation failed". The context of coming from a snapshot is probably not important for the notifications.

@mas-who mas-who force-pushed the duplicate-snapshot branch from db2b937 to 88470b6 Compare August 26, 2024 13:35
@mas-who mas-who requested a review from edlerd August 26, 2024 13:38
@mas-who mas-who force-pushed the duplicate-snapshot branch from 88470b6 to cd0b55f Compare August 26, 2024 13:38
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 looks mostly good. I found one issue where the instance name is wrongly reported as the instance snapshot name. I think this comes from reading the values of the instance creation operation, that contains both the snapshot and the instance as a resource. See the attached screen for more details on this. This is also visible on the instance list, when the instance is being created. I think both paths (operation list and instance list) use the same function getInstanceName to get the instance name from the operation.

Code also mostly good, some small comments below.

Screenshot from 2024-08-26 16-12-19

@mas-who mas-who force-pushed the duplicate-snapshot branch from cd0b55f to 96ea8b8 Compare August 26, 2024 18:10
@mas-who
Copy link
Collaborator Author

mas-who commented Aug 26, 2024

QA looks mostly good. I found one issue where the instance name is wrongly reported as the instance snapshot name. I think this comes from reading the values of the instance creation operation, that contains both the snapshot and the instance as a resource. See the attached screen for more details on this. This is also visible on the instance list, when the instance is being created. I think both paths (operation list and instance list) use the same function getInstanceName to get the instance name from the operation.

Code also mostly good, some small comments below.

Screenshot from 2024-08-26 16-12-19

Thanks for the hint here @edlerd , I made some adjustments to the getInstanceName function to handle the url encoded resource path returned by the backend. It will resolve the issue you observed above.

@mas-who mas-who force-pushed the duplicate-snapshot branch from 96ea8b8 to b32099f Compare August 26, 2024 18:19
src/pages/instances/ClusterMemberSelector.tsx Outdated Show resolved Hide resolved
src/util/operations.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the duplicate-snapshot branch from b32099f to 57a4e65 Compare August 27, 2024 08:46
edlerd
edlerd previously approved these changes Aug 28, 2024
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, some nitpicks below. Maybe change the commit message also to say "Create instance from snapshot" the current "duplicate snapshot" is misleading.

@mas-who mas-who force-pushed the duplicate-snapshot branch from 2e9bbfe to 8d36467 Compare August 28, 2024 09:28
@mas-who mas-who changed the title feat: Add duplicate snapshot feature [WD-14411] feat: Add create instance from snapshot feature [WD-14411] Aug 28, 2024
@mas-who mas-who merged commit ac058d1 into canonical:main Aug 28, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 28, 2024
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.

3 participants