Render VmDetails with deleted template curreclty. #287

yaacov · 2019-03-24T17:59:21Z

Description
When we want to render VmDetails with missing VmTemplate we get a warning that we can not find the template. But It is OK to delete a template of an existing vm, the view should silently fall back to vm's current resources.

Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1685393

Screenshots

When template is missing, show warning triangle instead of an error:

Replace drop-down with a label when only one flavor option is available:

Add an optional template resource link method to vmDetails:

Show labels for CPU and Memory in flavor form:

coveralls · 2019-03-24T18:08:46Z

Pull Request Test Coverage Report for Build 1088

11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
15 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.2%) to 86.957%

Files with Coverage Reduction	New Missed Lines	%
src/components/Details/VmDetails/VmDetails.js	4	84.5%
src/components/Details/VmTemplateDetails/VmTemplateDetails.js	4	75.9%
src/components/Details/Flavor/Flavor.js	7	76.0%

Totals
Change from base Build 875:	-0.2%
Covered Lines:	2829
Relevant Lines:	3119

💛 - Coveralls

coveralls · 2019-03-24T18:08:46Z

Pull Request Test Coverage Report for Build 1193

38 of 38 (100.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 87.855%

Totals
Change from base Build 968:	0.1%
Covered Lines:	3010
Relevant Lines:	3285

💛 - Coveralls

yaacov · 2019-03-25T07:09:40Z

wip until fixing coverage ...

atiratree · 2019-03-25T09:58:53Z

src/components/Details/VmDetails/VmDetails.js

    if (this.state.editing && !this.isVmOff(vm, launcherPod, importerPods, migration)) {
      this.setEditing(false);
    }
+
+    // Check template, if template is missing, update view.
+    if (getVmTemplate(vm) != null && this.state.templateError === PENDING_STR) {


This component will most likely be rerendered multiple times, but strictly speaking componentDidUpdate will not be called for initial render.

Thanks ! will fix that.

atiratree · 2019-03-25T10:07:11Z

src/components/Details/VmDetails/VmDetails.js

+
+    // Check template, if template is missing, update view.
+    if (getVmTemplate(vm) != null && this.state.templateError === PENDING_STR) {
+      retrieveVmTemplate(k8sGet, vm)


retrieveVmTemplate is already called for the flavor. Please call this method only once.

rawagner · 2019-03-25T10:24:18Z

src/components/Details/VmDetails/tests/VmDetails.test.js

@@ -15,6 +15,8 @@ import { getId } from '../../../../selectors';

 const testVmDetails = (vm, otherProps) => <VmDetails {...VmDetailsFixture[0].props} vm={vm} {...otherProps} />;

+const flushAllPromises = () => new Promise(resolve => setImmediate(resolve));


you can reuse https://github.com/kubevirt/web-ui-components/blob/master/src/tests/enzyme.js#L3

Nice, thanks ❗ 👍

yaacov · 2019-03-25T12:19:19Z

@rawagner @suomiy please re-review.

I'v removed the part that indicate missing template in the vm details view, because it is not part of the BZ, and may be implemented better in a separate PR.

rawagner · 2019-03-25T12:36:43Z

@yaacov I wonder if its okay to not show anything in UI if we cannot load the template ? We use the loaded template to get possible flavors when editing. Lets consider this flow:

I will create VM from template with Small flavor
I will delete the template (or somebody else)
I will go to VM details - I can see that flavor is Small
I will go to Edit mode - flavor dropdown now shows only Custom Flavor and I might get confused why is that so.

@matthewcarleton WDYT ?

src/components/Details/Flavor/Flavor.js

atiratree · 2019-04-01T09:58:44Z

src/components/Details/VmTemplateDetails/VmTemplateDetails.js

@@ -145,7 +145,10 @@ export class VmTemplateDetails extends React.Component {
    const { vmTemplate, dataVolumes, NamespaceResourceLink, LoadingComponent } = this.props;
    const id = getId(vmTemplate);
    const vm = selectVm(vmTemplate.objects);
-    const baseTemplateLabels = getVmTemplate(vmTemplate);
+    const getTemplateDisplayName = () => {


I meant, can we replace the getVmTemplate method? So it can be also reused in vmDetails.

I think it's ok now 💮

yaacov · 2019-04-01T10:34:23Z

can we add some margin to the custom label so it appears similar to the dropdown ?

.kubevirt-flavor__label {
   margin-bottom ...
}

@suomiy WDYT ?

yaacov · 2019-04-01T10:41:42Z

@suomiy @matthewcarleton do we need some labels to the inputs in edit mode ?

we curently have:

[ flavor drop down ]
[ number input     ]
[ number input     ]

should it have labels in edit mode, e.g.

[ flavor drop down        ]
Memory [ number input     ]
CPU    [ number input     ]

WDYT ?

b - Does <InlineEdit formFields has some props for that ?

atiratree · 2019-04-01T11:00:27Z

.kubevirt-flavor__label {
  margin-bottom ...
}

@suomiy WDYT ?

yup, I would make the margin similar to the dropdown

[ flavor drop down ]
Memory [ number input ]
CPU [ number input ]

WDYT ?

b - Does <InlineEdit formFields has some props for that ?

+1, that is a good remark. I guess it has to be some regression with InlineFormFactory, because It already has a title. Can you look at it?

yaacov · 2019-04-01T11:02:20Z

I guess it has to be some regression with InlineFormFactory, because It already has a title. Can you look at it?

Sure 👍

atiratree · 2019-04-01T11:07:14Z

src/components/Details/VmTemplateDetails/VmTemplateDetails.js

    const id = getId(vmTemplate);
    const vm = selectVm(vmTemplate.objects);
-    const baseTemplate = getVmTemplate(vmTemplate);
+    const baseTemplateLabels = getTemplateDisplayName(getVmTemplate(vmTemplate));


nit: variable name

sorry ... baseTemplateName ?

np, yup could be or baseTemplateDisplayName

yaacov · 2019-04-01T11:47:50Z

show labels in inline form:

yaacov · 2019-04-01T11:49:17Z

@suomiy this is nearing the end 🎉 🎈 , should I squash, or leave separate commits for easier review ?

matthewcarleton · 2019-04-01T12:13:41Z

@yaacov this looks great, thanks! One question, what happens when the available CPU/Memory is exceeded?

yaacov · 2019-04-01T12:27:45Z

what happens when the available CPU/Memory is exceeded?

@matthewcarleton

TL;DR - I do not know.

IFAIK we do not have the quota/total values to validate the form ... @suomiy do you know something about this ?

p.s.
What I think happen when values are bad is that openshift will try to execute and fail after some tries ... @suomiy - do you know something about that ?

…ng icon on template label instead. Ref: BZ1685393 1- Shaw warning triangle when template is missing. 2- Replace dropdown with div when only one option avaliable. 3- Add link to template resource. 4- Add deleted template test to vmTemplateDetails. 5- Show labels in the inline flavors edit form. 6- Add css to flavor label.

yaacov · 2019-04-01T15:47:13Z

@suomiy @rawagner please review ... I think it's ready 🍻

atiratree · 2019-04-01T17:05:20Z

what happens when the available CPU/Memory is exceeded?

@matthewcarleton

TL;DR - I do not know.

IFAIK we do not have the quota/total values to validate the form ... @suomiy do you know > something about this ?

p.s.
What I think happen when values are bad is that openshift will try to execute and fail after >some tries ... @suomiy - do you know something about that ?

Currently, we do not have a way to know the quota AFAIK. The request will go ok, just the VM will fail with an error the next time you try to start it.

atiratree · 2019-04-01T17:06:03Z

@suomiy this is nearing the end tada balloon , should I squash, or leave separate commits for easier review ?

don't worry about that. It will be squashed on merge

atiratree · 2019-04-01T17:24:50Z

.kubevirt-flavor__label {
margin-bottom ...
}

is this present in this PR?

yaacov · 2019-04-01T17:28:26Z

.kubevirt-flavor__label {
margin-bottom ...
}

is this present in this PR?

Aaa... forgot to do git add 💯

matthewcarleton · 2019-04-01T19:29:10Z

@suomiy ah ok. Ya it would be a nicer experience for sure to know what the limit is at the time. Would it be possible to explore adding that feature (in another PR :) )

…gth !==/=== 1

atiratree · 2019-04-02T08:08:48Z

Would it be possible to explore adding that feature (in another PR :) )

I think this should be explored on the backend. Doesn't seem like we can do much about it here in the UI.

@jelkosz Do you have some additional information on this issue?

atiratree · 2019-04-02T08:17:59Z

looks good, merging

kubevirt-bot added the size/L label Mar 24, 2019

yaacov force-pushed the render-vm-detail-with-deleted-template branch from 1c0bf32 to f243916 Compare March 24, 2019 18:04

yaacov changed the title ~~Render VmDetails with deleted template curreclty.~~ [WIP]Render VmDetails with deleted template curreclty. Mar 25, 2019

kubevirt-bot added the do-not-merge/work-in-progress label Mar 25, 2019

atiratree suggested changes Mar 25, 2019

View reviewed changes

rawagner reviewed Mar 25, 2019

View reviewed changes

kubevirt-bot added size/XXL and removed size/L labels Mar 25, 2019

yaacov force-pushed the render-vm-detail-with-deleted-template branch from 88af55e to a9e0a26 Compare March 25, 2019 10:50

kubevirt-bot added size/L and removed size/XXL labels Mar 25, 2019

yaacov force-pushed the render-vm-detail-with-deleted-template branch 2 times, most recently from 71bde61 to 3748b64 Compare March 25, 2019 11:28

kubevirt-bot added size/XL and removed size/L labels Mar 25, 2019

yaacov force-pushed the render-vm-detail-with-deleted-template branch 2 times, most recently from 7de0d06 to 96e3563 Compare March 25, 2019 11:58

kubevirt-bot added size/L and removed size/XL labels Mar 25, 2019

yaacov force-pushed the render-vm-detail-with-deleted-template branch 3 times, most recently from b0942e7 to 93dd926 Compare March 25, 2019 12:13

yaacov changed the title ~~[WIP]Render VmDetails with deleted template curreclty.~~ Render VmDetails with deleted template curreclty. Mar 25, 2019

kubevirt-bot removed the do-not-merge/work-in-progress label Mar 25, 2019

atiratree reviewed Mar 25, 2019

View reviewed changes

src/components/Details/Flavor/Flavor.js Outdated Show resolved Hide resolved

kubevirt-bot added size/XXL and removed size/XL labels Apr 1, 2019

atiratree reviewed Apr 1, 2019

View reviewed changes

yaacov force-pushed the render-vm-detail-with-deleted-template branch from 7c51b51 to 43c9da8 Compare April 1, 2019 10:56

atiratree reviewed Apr 1, 2019

View reviewed changes

yaacov changed the title ~~[WIP] Render VmDetails with deleted template curreclty.~~ Render VmDetails with deleted template curreclty. Apr 1, 2019

kubevirt-bot removed the do-not-merge/work-in-progress label Apr 1, 2019

yaacov force-pushed the render-vm-detail-with-deleted-template branch from 1dfe303 to f252b77 Compare April 1, 2019 15:21

yaacov force-pushed the render-vm-detail-with-deleted-template branch from f252b77 to 5d3a01c Compare April 1, 2019 15:39

add missing scss file

a76a6d4

yaacov added 2 commits April 2, 2019 10:13

Remove unneeded check for flavor !==/=== custom, it is implied by len…

8d3816b

…gth !==/=== 1

Move error logic into titleWithWarning instead of inline

c60efe4

atiratree merged commit c694293 into kubevirt:master Apr 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render VmDetails with deleted template curreclty. #287

Render VmDetails with deleted template curreclty. #287

yaacov commented Mar 24, 2019 •

edited

Loading

coveralls commented Mar 24, 2019

coveralls commented Mar 24, 2019 •

edited

Loading

yaacov commented Mar 25, 2019

atiratree Mar 25, 2019

yaacov Mar 25, 2019

atiratree Mar 25, 2019

yaacov Mar 25, 2019

rawagner Mar 25, 2019

yaacov Mar 25, 2019

yaacov commented Mar 25, 2019

rawagner commented Mar 25, 2019

atiratree Apr 1, 2019

yaacov Apr 1, 2019

yaacov Apr 1, 2019

yaacov commented Apr 1, 2019

yaacov commented Apr 1, 2019

atiratree commented Apr 1, 2019

yaacov commented Apr 1, 2019

atiratree Apr 1, 2019

yaacov Apr 1, 2019

atiratree Apr 1, 2019

yaacov commented Apr 1, 2019

yaacov commented Apr 1, 2019

matthewcarleton commented Apr 1, 2019

yaacov commented Apr 1, 2019

yaacov commented Apr 1, 2019

atiratree commented Apr 1, 2019

atiratree commented Apr 1, 2019

atiratree commented Apr 1, 2019

yaacov commented Apr 1, 2019

matthewcarleton commented Apr 1, 2019

atiratree commented Apr 2, 2019

atiratree commented Apr 2, 2019

		@@ -15,6 +15,8 @@ import { getId } from '../../../../selectors';

		const testVmDetails = (vm, otherProps) => <VmDetails {...VmDetailsFixture[0].props} vm={vm} {...otherProps} />;

		const flushAllPromises = () => new Promise(resolve => setImmediate(resolve));

Render VmDetails with deleted template curreclty. #287

Render VmDetails with deleted template curreclty. #287

Conversation

yaacov commented Mar 24, 2019 • edited Loading

coveralls commented Mar 24, 2019

Pull Request Test Coverage Report for Build 1088

💛 - Coveralls

coveralls commented Mar 24, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1193

💛 - Coveralls

yaacov commented Mar 25, 2019

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

yaacov commented Mar 25, 2019

rawagner commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Apr 1, 2019

yaacov commented Apr 1, 2019

atiratree commented Apr 1, 2019

yaacov commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Apr 1, 2019

yaacov commented Apr 1, 2019

matthewcarleton commented Apr 1, 2019

yaacov commented Apr 1, 2019

yaacov commented Apr 1, 2019

atiratree commented Apr 1, 2019

atiratree commented Apr 1, 2019

atiratree commented Apr 1, 2019

yaacov commented Apr 1, 2019

matthewcarleton commented Apr 1, 2019

atiratree commented Apr 2, 2019

atiratree commented Apr 2, 2019

yaacov commented Mar 24, 2019 •

edited

Loading

coveralls commented Mar 24, 2019 •

edited

Loading