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: [WD-14036] Duplicate instance (lxc copy) #849

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Aug 15, 2024

Done

  • Added DuplicateInstance Button and Form
  • Added duplicate instance API call.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Navigate to an instance
    • Select "Duplicate instance" in header.
    • Fill out form and select "Duplicate & Create"
    • Verify that another instance has been created with the same details.

Screenshots

image

@webteam-app
Copy link

@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch 4 times, most recently from ac36cc3 to e21e0f2 Compare August 15, 2024 19:45
@piperdeck
Copy link

So I'm getting an error where it says it can't find the instance I'm duplicating in the project I've selected. Is it possible that the project dropdown is being used to look for the source instance, rather than specifying the destination?

CleanShot.2024-08-15.at.20.58.31.mp4

@edlerd
Copy link
Collaborator

edlerd commented Aug 16, 2024

Some observations

  • the "Duplicate & start" copy on the new modal deviates from the "Create and start" copy on creation. We should better align those
  • the copy "Created and started cloned instance abcd-1." of the success notification reads very odd to me.
  • the copy "Creation for instance abcd-1 started." does not transport that this is a clone operation. I am not sure if this is the right thing to say in this event.
  • I think we can simplify by duplicating and not starting the cloned instance? Currently, we have a complex series of multiple API calls going on, adding complexity on error handling.

@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from e21e0f2 to bf45e63 Compare August 16, 2024 12:20
@Kxiru Kxiru requested review from mas-who and edlerd August 16, 2024 12:21
edlerd
edlerd previously requested changes Aug 16, 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.

Thanks for the adjustments, this looks much cleaner already. Two further suggestions below.

@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from bf45e63 to 0aba060 Compare August 19, 2024 13:07
@Kxiru
Copy link
Contributor Author

Kxiru commented Aug 19, 2024

Alrighty, that's all been covered, @edlerd , please let me know what you think :)

@Kxiru Kxiru requested a review from edlerd August 19, 2024 13:09
@Kxiru
Copy link
Contributor Author

Kxiru commented Aug 21, 2024

Got this strange cryptic error:
...

Hey, Piper,
I have not been able to recreate your issue. I am doing some investigation as I have not seen this error before, but the initial problem that you mentioned has been addressed, and yes, now duplicated instances are set in the correct project.

  • Are you running/testing the branch locally in a clustered environment or within the demo server?
  • Currently, I am having issues with the demo server. Could you tell me more about the instance you are trying to duplicate? Such as it's base image and instance type?
  • When duplicating other randomly selected instances locally and in the demo server, I do not come across this error. Are you having this issue across multiple different instances?

@Kxiru Kxiru requested a review from edlerd August 21, 2024 13:05
edlerd
edlerd previously approved these changes Aug 21, 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.

LGTM

@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from 383a48c to 667b622 Compare August 21, 2024 13:45
@Kxiru
Copy link
Contributor Author

Kxiru commented Aug 21, 2024

@Kxiru ,

  • Validate instance name(s) and increment tail if name already in use.
  • Center modal when screen is minimised.

@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from 667b622 to f3c6e0f Compare August 21, 2024 16:46
@Kxiru Kxiru requested a review from edlerd August 21, 2024 16:46
@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from f3c6e0f to 73040f8 Compare August 21, 2024 16:48
@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from 73040f8 to 8ffba4a Compare August 21, 2024 18:10
@Kxiru Kxiru requested a review from edlerd August 21, 2024 18:13
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 last comment on the hook for the deduplication, and there is the open discussion on the CSS change, then this should be good from my side.

@mas-who
Copy link
Collaborator

mas-who commented Aug 22, 2024

@Kxiru just a few minor comments from my end and also good to go, great work! :)

@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from 8ffba4a to 567376c Compare August 22, 2024 12:23
@Kxiru Kxiru requested review from edlerd and mas-who August 22, 2024 12:24
@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch 2 times, most recently from 168aaed to 1b5f68b Compare August 22, 2024 12:54
@Kxiru Kxiru force-pushed the duplicate-instance-from-inst/snapshot branch from 1b5f68b to 064f454 Compare August 22, 2024 15:08
@Kxiru Kxiru requested a review from mas-who August 22, 2024 15:08
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, thanks for adding the duplication!

@Kxiru Kxiru merged commit dd59a2a into canonical:main Aug 22, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 22, 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.

5 participants