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

VM snapshots -WIP #183

Merged
merged 26 commits into from Jul 11, 2019
Merged

VM snapshots -WIP #183

merged 26 commits into from Jul 11, 2019

Conversation

Ranelim
Copy link
Contributor

@Ranelim Ranelim commented May 22, 2019

Snapshots allow users to create a copy of the VM disks and memory, mainly for restore and backup purposes. The snapshots are taken live or offline depending on if the VM is running or not.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 22, 2019
Copy link
Contributor

@lizsurette lizsurette left a comment

Choose a reason for hiding this comment

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

@Ranelim Excellent job on a big topic! Added mostly nitpicky comments :)

web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
Copy link
Contributor

@serenamarie125 serenamarie125 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 nit, and unsure if it needs to be changed, but the spacing between buttons the in the modals should be much less.

Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@beanh66 should we be using Sentence style for new actions? I think we may have talked about this, but it makes the kebab menus look pretty inconsistent

@beanh66
Copy link
Member

beanh66 commented May 28, 2019

Good question @serenamarie125, moving to sentence style will be a tricky change but that is the PF4 direction so need to figure out how to roll that out. All of our new designs should follow the PF4 topography standards. What do you think?

Copy link
Contributor

@matthewcarleton matthewcarleton left a comment

Choose a reason for hiding this comment

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

@Ranelim this looks great! I just left a couple small updates.

Copy link
Member

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

@matthewcarleton @Ranelim I left some comments inline

web-console/knikubevirt/snapshots/snapshots.md Outdated Show resolved Hide resolved
Copy link
Contributor

@andybraren andybraren left a comment

Choose a reason for hiding this comment

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

Sorry for reviewing so late - thank you for adding this Ran. Some of these suggestions are related to the original design being a bit old, so maybe they can be fixed in a future update. FYI @matthewcarleton.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2019
@Ranelim Ranelim changed the title VM snapshots [WIP] VM snapshots Jul 10, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2019
Copy link
Contributor

@lizsurette lizsurette left a comment

Choose a reason for hiding this comment

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

Awesome work, @Ranelim !

@matthewcarleton matthewcarleton changed the title [WIP] VM snapshots VM snapshots -WIP Jul 11, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019
@matthewcarleton matthewcarleton merged commit 30aa96f into openshift:master Jul 11, 2019
@Ranelim Ranelim deleted the snapshots branch April 6, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants