Create VmDetails component #135

pcbailey · 2018-12-05T12:58:11Z

This PR creates a VmDetails component for the VM details overview page.

src/components/Details/VmDetails/VmDetails.js

rawagner · 2018-12-05T14:31:00Z

src/components/Details/VmDetails/VmDetails.js

+                    </dd>
+
+                    <dt>Operating System</dt>
+                    <dd>{get(vm, 'metadata.annotations["os.template.cnv.io"]', DASHES)}</dd>


can you extract this to https://github.com/kubevirt/web-ui-components/blob/master/src/utils/selectors.js ? Same for Workload Profile and Templates.

I also moved the OS/Flavor/Workload from annotations to labels just today. So its under vm/metadata.labels not vm/metadata.annotations

Can you please also show the OS icon? In common templates it is defined as iconClass which points to standard openshift icons defined here.
Pleas note this icon might not be defined in the template so you can not rely on it 100%, but in all standard templates it should be there.

src/components/Details/VmDetails/VmDetails.js

src/components/Details/VmDetails/fixtures/VmDetails.fixture.js

src/components/Details/VmDetails/VmDetails.js

mareklibra · 2018-12-07T09:33:38Z

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

@@ -0,0 +1,5 @@
+// TODO Add valid tests


coveralls · 2018-12-10T02:42:16Z

Pull Request Test Coverage Report for Build 387

18 of 49 (36.73%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-2.7%) to 83.492%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/k8s/mock_pod/cloudInitTestPod.pod.js	0	1	0.0%
src/k8s/mock_vm/cloudInitTestVm.vm.js	0	1	0.0%
src/k8s/mock_vmi/cloudInitTestVmi.vmi.js	0	1	0.0%
src/components/Details/VmDetails/VmDetails.js	12	40	30.0%

Totals
Change from base Build 386:	-2.7%
Covered Lines:	1240
Relevant Lines:	1402

💛 - Coveralls

coveralls · 2018-12-10T02:42:16Z

Pull Request Test Coverage Report for Build 534

50 of 52 (96.15%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 85.956%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/k8s/mock_pod/cloudInitTestPod.pod.js	0	1	0.0%
src/k8s/mock_vm/cloudInitTestVm.vm.js	0	1	0.0%

Totals
Change from base Build 523:	0.2%
Covered Lines:	1332
Relevant Lines:	1477

💛 - Coveralls

pcbailey · 2018-12-10T02:46:40Z

I haven't been able to fully test all the changes due to my environment malfunctioning, but I wanted to push it to give you guys a chance to run it for yourself if you'd like. I still have a few more changes coming in addition to the tests. It builds fine for me, but there is a test failure that seems to be unrelated as it also occurs on master.

src/models/index.js

src/k8s/request.js

src/components/Details/VmDetails/VmDetails.js

src/k8s/request.js

This commit makes changes based on review feedback, including the following items: - Use VmStatus instead of StateColumn - Extract selectors - Use cloudInitData function - Remove dashes when no description is provided - Add CPU and Memory to Flavor field - Organize mocks for the VmDetails fixture

src/components/Details/VmDetails/VmDetails.js

atiratree · 2018-12-21T15:44:15Z

src/utils/selectors.js

+export const getLabelValue = (vm, label) => {
+  let value;
+  if (has(vm, 'metadata.labels')) {
+    forEach(Object.keys(vm.metadata.labels), key => {


maybe using an array find function would be more readable here

atiratree · 2018-12-21T15:55:07Z

src/components/Details/VmDetails/VmDetails.js

+  const ipAddresses = getVmIpAddresses(vmi);
+
+  return (
+    <Fragment>


I don't really see a reason for using Fragment here

atiratree · 2018-12-21T16:03:30Z

src/components/Details/VmDetails/VmDetails.js

+
+const DASHES = '---';
+
+export const getVmIpAddresses = vmi => {


maybe it would be better to move this function into selectors? + also the tests

atiratree · 2018-12-21T16:07:12Z

src/components/Details/VmDetails/VmDetails.js

+export const getVmIpAddresses = vmi => {
+  let ipAddresses = [];
+  if (has(vmi, 'status.interfaces')) {
+    ipAddresses = vmi.status.interfaces.map(iface => iface.ipAddress);


I would prefer this as a ternary condition, but there is no real need for this change.

atiratree · 2018-12-21T16:09:32Z

src/components/Details/VmDetails/VmDetails.js

+    const memory = getMemory(vm);
+    let resourceStr = '';
+    resourceStr += cpu ? `${cpu} CPU` : '';
+    resourceStr += memory ? `, ${memory} Memory` : '';


this will build a string starting with , if there is no cpu by any chance

atiratree · 2018-12-21T16:27:17Z

src/components/Details/VmDetails/VmDetails.js

+      <div className="co-m-pane__body">
+        <h1 className="co-m-pane__heading">Virtual Machine Overview</h1>
+        <div>
+          <div className="row">


We could use pf-react (bootstrap) Row and Col components here. I personally do not mind this approach but we are already using Col in this project, so I guess it would be better to be consistent.

atiratree · 2018-12-21T16:30:22Z

src/components/Details/VmDetails/VmDetails.js

+              </dl>
+            </div>
+
+            {/* Details columns */}


I guess it would be more useful to use ids like this for the columns and the descriptiveness would be kept.

atiratree · 2018-12-21T16:35:31Z

src/components/Details/VmDetails/VmDetails.js

+                  <dd>{launcherPod ? launcherPod.spec.hostname : DASHES}</dd>
+
+                  <dt>Namespace</dt>
+                  <dd>{NamespaceResourceLink}</dd>


this component sharing is really ugly, but I guess we don't have a choice here.

atiratree · 2018-12-21T16:38:57Z

src/components/Details/VmDetails/fixtures/VmDetails.fixture.js

+  },
+};
+
+export default {


We could expose more fixtures (with more colorful set of data) to the cosmos. cloudInitTestVm would be nice

rawagner · 2019-01-02T13:43:52Z

Since @pcbailey is on PTO until the end of the week, I will merge this and create a follow-up PR which addresses @suomiy's review comments

kubevirt-bot added size/XL needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 5, 2018

jelkosz requested review from mareklibra and rawagner and removed request for mareklibra December 5, 2018 13:09

rawagner suggested changes Dec 5, 2018

View reviewed changes

mareklibra suggested changes Dec 7, 2018

View reviewed changes

pcbailey changed the title ~~Create VmDetails component~~ WIP: Create VmDetails component Dec 10, 2018

pcbailey force-pushed the create-vm-details-component branch from 0a5caf1 to 3d3d990 Compare December 10, 2018 02:39

kubevirt-bot added size/L and removed size/XL needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 10, 2018

pcbailey mentioned this pull request Dec 10, 2018

Move VM details to web-ui-components component kubevirt/web-ui#132

Merged

pcbailey force-pushed the create-vm-details-component branch from 3d3d990 to 97312bb Compare December 17, 2018 06:21

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2018

mareklibra reviewed Dec 18, 2018

View reviewed changes

src/models/index.js Outdated Show resolved Hide resolved

src/k8s/request.js Outdated Show resolved Hide resolved

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

pcbailey force-pushed the create-vm-details-component branch from 97312bb to e235245 Compare December 18, 2018 13:07

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2018

pcbailey force-pushed the create-vm-details-component branch from e235245 to 18f01bc Compare December 18, 2018 13:54

rawagner reviewed Dec 18, 2018

View reviewed changes

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

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

src/k8s/request.js Outdated Show resolved Hide resolved

pcbailey added 5 commits December 18, 2018 21:49

Create VmDetails component

1401337

Pass resource link components directly

84b8b88

Display IP addresses

363ef85

Add tests

d961666

pcbailey force-pushed the create-vm-details-component branch from 18f01bc to 657b8e3 Compare December 19, 2018 04:08

mareklibra suggested changes Dec 19, 2018

View reviewed changes

pcbailey force-pushed the create-vm-details-component branch from 25e3491 to 87c23c4 Compare December 19, 2018 11:14

kubevirt-bot added size/XXL and removed size/L labels Dec 19, 2018

pcbailey added 10 commits December 20, 2018 11:15

Fix rendering of flavor, OS, workload profile, and template

83af8f7

Remove left margin from VM details body

ab38137

Remove Status tooltip

c96abec

Add right margin to description

559fc79

Replace on/off status with VM name

10eb3f5

Fix rendering of description element

9bf8fc4

Change VM details title to 'Virtual Machine Overview'

0ad32c5

Remove cloudinit status from VM details overview

ee5f0f3

Fix rendering of node element in VM details overview

0f929f2

Fix PropTypes for VmDetails

1f749da

pcbailey force-pushed the create-vm-details-component branch from 1fa2e21 to 9f50f67 Compare December 20, 2018 16:25

pcbailey added 2 commits December 20, 2018 11:53

Add snapshot tests for VmDetails

ce31e99

Add LoadingComponent to CreateDiskRow fixture

c748bd8

pcbailey force-pushed the create-vm-details-component branch from 9f50f67 to c748bd8 Compare December 20, 2018 16:55

pcbailey changed the title ~~WIP: Create VmDetails component~~ Create VmDetails component Dec 20, 2018

atiratree suggested changes Dec 21, 2018

View reviewed changes

rawagner merged commit 2ec13fe into kubevirt:master Jan 2, 2019

pcbailey deleted the create-vm-details-component branch April 22, 2019 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create VmDetails component #135

Create VmDetails component #135

pcbailey commented Dec 5, 2018

rawagner Dec 5, 2018

jelkosz Dec 7, 2018

mareklibra Dec 7, 2018

coveralls commented Dec 10, 2018

coveralls commented Dec 10, 2018 •

edited

Loading

pcbailey commented Dec 10, 2018

atiratree Dec 21, 2018 •

edited

Loading

atiratree Dec 21, 2018

atiratree Dec 21, 2018

atiratree Dec 21, 2018

atiratree Dec 21, 2018

atiratree Dec 21, 2018

atiratree Dec 21, 2018

atiratree Dec 21, 2018

atiratree Dec 21, 2018

rawagner commented Jan 2, 2019


		const DASHES = '---';

		export const getVmIpAddresses = vmi => {

Create VmDetails component #135

Create VmDetails component #135

Conversation

pcbailey commented Dec 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 10, 2018

Pull Request Test Coverage Report for Build 387

💛 - Coveralls

coveralls commented Dec 10, 2018 • edited Loading

Pull Request Test Coverage Report for Build 534

💛 - Coveralls

pcbailey commented Dec 10, 2018

atiratree Dec 21, 2018 • 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

rawagner commented Jan 2, 2019

coveralls commented Dec 10, 2018 •

edited

Loading

atiratree Dec 21, 2018 •

edited

Loading