kubevirt: Edit VM flavor #2392

mareklibra · 2019-08-19T09:50:14Z

The VM Detail is enriched for the Edit VM Flavor feature, implemented as a modal dialog.

TODO:

edit flavor on Template Detail page

Note:
Integration tests will be added by a follow-up as they will need to be stacked on other open PRs recently.

mareklibra · 2019-08-19T09:54:22Z

mareklibra · 2019-08-19T09:58:15Z

...packages/kubevirt-plugin/src/components/modals/vm-description-modal/vm-description-modal.tsx

@@ -13,6 +13,7 @@ import { VMLikeEntityKind } from '../../../types';
 import { getDescription, getVMLikeModel } from '../../../selectors/selectors';
 import { getUpdateDescriptionPatches } from '../../../k8s/patches/vm/vm-patches';

+// TODO: should be moved under kubevirt-plugin/src/style.scss


Looking for opinions: will we encapsulate styles with components or will we maintain single SASS hierarchy starting with frontend/public/style.scss (or kubevirt-plugin/src/style.scss).

The encapsulation has clearly its benefits, but we do not develop a library here. And the rest of the code follows the later approach.

mareklibra · 2019-08-20T11:17:42Z

Cc @matthewcarleton

matthewcarleton · 2019-08-20T12:03:59Z

@mareklibra thanks this looks great. A few questions: can we only adjust the height of the modal if they choose custom? That white space is a bit odd if they choose any other preset flavor.
Also some text here could be useful for screen reader users. Maybe "Edit the combination of processing power and memory for the virtual machine."

mareklibra · 2019-08-26T12:58:31Z

@matthewcarleton , I tried to resize the modal for non-custom entries but the dropdown shows badly as it hides its rows behind modal box content.
This is what I got so far:

Recently, there can be up to 5 options to select from.
Any idea how to deal with it, please?

matthewcarleton · 2019-08-26T21:10:31Z

@mareklibra this would be an overflow issue on the content area of .modal-body that shouldn't ship with PF3 from what I've seen. Has it been applied via the console?

mareklibra · 2019-08-27T06:53:00Z

Thank you, @matthewcarleton , that's the issue. As we are still on PF3, what about following fix:

I think it behaves better than "jumping on the fly", where the modal box would increase height when the dropdown is opened and resize back when an option is selected. If the dropdown is opened, there is no other action than select an option or dismiss. Wdyt?

frontend/packages/kubevirt-plugin/src/components/modals/vm-flavor-modal/_vm-flavor-modal.scss

matthewcarleton · 2019-08-27T14:11:27Z

@mareklibra yes I agree this is much better IMO.
I've also opened an issue in PatternFly b/c I see the same issue with the latest version.

mareklibra · 2019-08-28T07:36:13Z

/retest

mareklibra · 2019-08-28T09:44:12Z

New addition: the VM Details page shows memory and CPU wihtin the Flavor field:

rawagner · 2019-08-28T09:50:09Z

frontend/packages/kubevirt-plugin/src/components/modals/vm-flavor-modal/vm-flavor-modal.tsx

+  return convertToBaseValue(memory) / MB;
+};
+
+const getFlavors = (vm: VMLikeEntityKind, templates: TemplateKind[]) => {


why dont we move this to selectors ? I think it has a potential to be used in other places too

rawagner · 2019-08-28T09:50:30Z

frontend/packages/kubevirt-plugin/src/components/modals/vm-flavor-modal/vm-flavor-modal.tsx

+const VMFlavorModal = withHandlePromise((props: VMFlavornModalProps) => {
+  const { vmLike, templates, inProgress, errorMessage, handlePromise, close, cancel } = props;
+
+  const flattenTemplates = _.get(templates, 'data', []) as TemplateKind[];


loading/error state should be handled

rawagner · 2019-08-28T09:50:48Z

frontend/packages/kubevirt-plugin/src/components/modals/vm-flavor-modal/vm-flavor-modal.tsx

+  };
+
+  let topClass = 'modal-content ';
+  topClass +=


i'd rather use classNames for this

rawagner · 2019-08-28T09:56:04Z

frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template-resource.tsx

+      <dt>
+        Flavor
+        {canUpdateTemplate && (
+          <button


can we create component out of this ? Something like EditButton ?

rawagner · 2019-08-28T09:58:02Z

frontend/packages/kubevirt-plugin/src/k8s/patches/vm/vm-patches.ts

+import { getVMLikePatches } from '../vm-template';
+import { isCPUEqual } from '../../../utils';
+
+const getLabelsPatch = (vmLike: VMLikeEntityKind): Patch | null => {


Suggested change

const getLabelsPatch = (vmLike: VMLikeEntityKind): Patch | null => {

const getLabelsPatch = (vmLike: VMLikeEntityKind): Patch => {

we dont have strict null check enabled, so no need to add | null. Applies for others too

leftover, removed

rawagner · 2019-08-28T10:01:46Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/selectors.ts

+  getTemplatesLabelValues(vmTemplates, TEMPLATE_WORKLOAD_LABEL);
+
+export const getTemplates = (
+  templates: TemplateKind[],


i'd prefer to specify default type in function declaration

Suggested change

templates: TemplateKind[],

templates: TemplateKind[] = [],

rawagner · 2019-08-28T10:02:00Z

frontend/packages/kubevirt-plugin/src/selectors/vm-template/selectors.ts

+  workload: string,
+  flavor: string,
+) =>
+  (templates || []).filter((t) => {


Suggested change

(templates || []).filter((t) => {

templates.filter((t) => {

rawagner · 2019-08-28T10:07:00Z

frontend/packages/kubevirt-plugin/src/selectors/vm/vmlike.ts

+import { VirtualMachineModel } from '../../models';
+import { selectVM } from '../vm-template/selectors';
+
+export const isVM = (vmLikeEntity: VMLikeEntityKind): boolean =>


this could be a type guard

Suggested change

export const isVM = (vmLikeEntity: VMLikeEntityKind): boolean =>

export const isVM = (vmLikeEntity: VMLikeEntityKind): vmLikeEntity is VMKind =>

good idea, done

rawagner · 2019-08-28T10:09:10Z

frontend/packages/kubevirt-plugin/src/selectors/vm/vmlike.ts

+    return null;
+  }
+
+  if (isVM(vmLikeEntity)) {


when the above is converted to type guard you can just

Suggested change

if (isVM(vmLikeEntity)) {

return isVM(vmLikeEntity) ? vmLikeEntity : selectVM(vmLikeEntity);

rawagner · 2019-08-28T10:10:10Z

frontend/packages/kubevirt-plugin/src/selectors/vm/cpu.ts

+  );
+};
+
+type CPUType = {


Suggested change

type CPUType = {

type CPURaw = {

?

why not ... done

matthewcarleton · 2019-08-28T12:29:22Z

@mareklibra three little nitpicks:

Can we capitalize all the flavor types
For memory I believe we are following the GiB format rather than just G
Can we have the format be : Large : 2 vCPUs, 6 GiB memory

mareklibra · 2019-08-28T14:07:54Z

@matthewcarleton ,

Can we capitalize all the flavor types

sure, done

For memory I believe we are following the GiB format rather than just G

Our (KubeVirt) templates use values like 6G which converted to GiB leads to an ugly number.
For that reason, I would not go with the binary units unless changed within wider context (out of scope for this PR).

I have newly added conversion to best-possible human readable decimal byte-unit, like 6 GB instead of input 6000 M. Wdyt?

Can we have the format be : Large : 2 vCPUs, 6 GiB memory

done with respect to the previous point

mareklibra · 2019-08-28T14:09:15Z

Latest screenshots:

matthewcarleton · 2019-08-28T14:35:52Z

@mareklibra that sounds reasonable to me. Thanks!

mareklibra · 2019-08-29T07:42:24Z

The PR is retargeted to release-4.3 branch.

mareklibra · 2019-08-29T07:59:05Z

/retest

mareklibra · 2019-08-29T08:29:44Z

Switching back to master and waiting for next master-next. Please excuse the confusion.

mareklibra · 2019-09-20T07:53:21Z

/retest

rawagner · 2019-09-21T11:54:04Z

/lgtm

openshift-ci-robot · 2019-09-21T11:54:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mareklibra, rawagner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/kubevirt-plugin/OWNERS~~ [mareklibra,rawagner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rawagner · 2019-09-21T12:21:36Z

/test e2e-aws-console-olm

rhamilto · 2019-11-04T18:50:44Z

This PR appears to be the cause of https://bugzilla.redhat.com/show_bug.cgi?id=1767853

attn: @mareklibra, @rawagner

rhamilto · 2019-11-04T22:08:00Z

Specifically, git bisect pointed at ef3a10c

openshift-ci-robot requested review from jelkosz and vojtechszocs August 19, 2019 09:51

mareklibra commented Aug 19, 2019

View reviewed changes

mareklibra changed the title ~~WIP Edit VM flavor~~ kubevirt: Edit VM flavor Aug 19, 2019

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 Aug 19, 2019

mareklibra commented Aug 27, 2019

View reviewed changes

frontend/packages/kubevirt-plugin/src/components/modals/vm-flavor-modal/_vm-flavor-modal.scss Outdated Show resolved Hide resolved

rawagner suggested changes Aug 28, 2019

View reviewed changes

mareklibra mentioned this pull request Aug 28, 2019

kubevirt: Add edit VM flavor integration test #2517

Merged

1 task

mareklibra changed the base branch from master to release-4.3 August 29, 2019 07:35

mareklibra changed the base branch from release-4.3 to master August 29, 2019 08:28

mareklibra added 5 commits September 19, 2019 12:50

remove typescript's null

40be319

Add more types

816feb6

kubevirt: Capitalize flavors when rendering

4b08bd5

VM flavor: humanize memory

0c28e9d

Move edit button to value in the VM Detail

f0e35fe

mareklibra changed the base branch from master to master-4.3 September 19, 2019 10:55

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 19, 2019

openshift-ci-robot assigned rawagner Sep 21, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2019

openshift-merge-robot merged commit 963353d into openshift:master-4.3 Sep 21, 2019

mareklibra mentioned this pull request Sep 23, 2019

Expose cdrom in UI - Story #1, #2, #3, #4 #2477

Merged

sg00dwin mentioned this pull request Sep 23, 2019

Follow on to update inline edit links in kubevirt-plugin #2719

Closed

spadgett added this to the v4.3 milestone Sep 23, 2019

	const getLabelsPatch = (vmLike: VMLikeEntityKind): Patch \| null => {
	const getLabelsPatch = (vmLike: VMLikeEntityKind): Patch => {

	export const isVM = (vmLikeEntity: VMLikeEntityKind): boolean =>
	export const isVM = (vmLikeEntity: VMLikeEntityKind): vmLikeEntity is VMKind =>

	if (isVM(vmLikeEntity)) {
	return isVM(vmLikeEntity) ? vmLikeEntity : selectVM(vmLikeEntity);

kubevirt: Edit VM flavor #2392

kubevirt: Edit VM flavor #2392

Conversation

mareklibra commented Aug 19, 2019 • edited Loading

mareklibra commented Aug 19, 2019

Choose a reason for hiding this comment

mareklibra commented Aug 20, 2019

matthewcarleton commented Aug 20, 2019

mareklibra commented Aug 26, 2019

matthewcarleton commented Aug 26, 2019

mareklibra commented Aug 27, 2019

matthewcarleton commented Aug 27, 2019

mareklibra commented Aug 28, 2019

mareklibra commented Aug 28, 2019

rawagner Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewcarleton commented Aug 28, 2019

mareklibra commented Aug 28, 2019

mareklibra commented Aug 28, 2019

matthewcarleton commented Aug 28, 2019

mareklibra commented Aug 29, 2019

mareklibra commented Aug 29, 2019

mareklibra commented Aug 29, 2019

mareklibra commented Sep 20, 2019

rawagner commented Sep 21, 2019

openshift-ci-robot commented Sep 21, 2019

rawagner commented Sep 21, 2019

rhamilto commented Nov 4, 2019

rhamilto commented Nov 4, 2019 • edited Loading

mareklibra commented Aug 19, 2019 •

edited

Loading

rawagner Aug 28, 2019 •

edited

Loading

rhamilto commented Nov 4, 2019 •

edited

Loading