Details, Inventory card and Health card for Storage Dashboard #311

cloudbehl · 2019-04-04T13:55:38Z

coveralls · 2019-04-04T13:59:57Z

Pull Request Test Coverage Report for Build 1271

53 of 56 (94.64%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 87.686%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/StorageOverview/OCSHealth/Health.js	5	6	83.33%
src/components/StorageOverview/StorageOverview.js	6	7	85.71%
src/components/StorageOverview/StorageOverviewContext/StorageOverviewContext.js	3	4	75.0%

Totals
Change from base Build 1264:	0.08%
Covered Lines:	3154
Relevant Lines:	3451

💛 - Coveralls

cloudbehl · 2019-04-04T14:04:51Z

Sorry for the XXL PR, Once this is merged will make sure all that for each component we send it separately.

cloudbehl · 2019-04-05T06:32:15Z

@mareklibra @rawagner please review.

rawagner · 2019-04-05T07:19:58Z

src/components/StorageOverview/Details/Details.js

+  </DashboardCard>
+);
+
+const getClusterName = cephCluster => {


no need to have this as separate function - you can directly use getName(cephCluster) and the result will be the same

rawagner · 2019-04-05T07:20:34Z

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

+  },
+  {
+    component: StorageDetails,
+    name: 'Loading cluster details',


should be Loading storage details

rawagner · 2019-04-05T07:23:05Z

src/components/StorageOverview/Inventory/Inventory.js

+
+const InventoryBody = ({ nodes, pods, vms, vmis, pvs, pvcs, migrations }) => (
+  <React.Fragment>
+    <InventoryRow title="Nodes" {...mapNodesToProps(nodes)} />


Looking at the design - you want Nodes, Disks, Pods, PVs and PVCs. You are missing Disks but I guess you will add them later, however VMs here are not needed so you can get rid of vm, vmi and migrations

rawagner · 2019-04-05T07:25:57Z

src/components/StorageOverview/Inventory/utils.js

@@ -0,0 +1,74 @@
+import { getVmStatus, VM_STATUS_ALL_ERROR, VM_STATUS_ALL_PROGRESS } from '../../../utils/status/vm';


this file is the same as ClusterOverview/Inventory/utils so you could move it to Dashboard/Inventory/utils and reuse it in ClusterOverview/Inventory and StorageOverview/Inventory

maybe mapStatuses and resolveStatusResult could be moved. But I am not sure about the other mapXToProps. Shouldn't these be different for each dashboard?

as written in #311 (comment) I think that its fine to have overlapping information so IMHO it makes sense to move these utils as other dashboards will use it.

if that is the case then sure, it makes sense to move them

@suomiy @rawagner Ack! Done

rawagner · 2019-04-05T07:38:38Z

src/tests/mocks/persistentVolume/index.js

+    metadata: {
+      // should be the same namespaces as the vm
+      name: 'disk-one_pv',
+      namespace: 'default',


PVs are not namespaced (applies for all other occurrences)

rawagner · 2019-04-05T07:49:24Z

src/utils/status/pv/pvStatus.js

+import { NOT_HANDLED } from '../common';
+
+const statusMapper = {
+  Available: PV_STATUS_AVAILABLE,


you could convert all values to form such as Available: { status: PV_STATUS_AVAILABLE } and then in getPvStatus you could just do pv => statusMapper[getStatusPhase(pv)] || {status: PV_STATUS_DEFAULT};. getMappedStatus could be deleted then

the statusMapper is only meant to be the mapping of constant -> constant, getMappedStatus can include other functionality in the future.

@rawagner @suomiy Is there any action required from my side on this? I am not sure if I understood it well.

@suomiy ok, i think there is not action needed @cloudbehl :)

rawagner · 2019-04-05T07:51:13Z

src/components/StorageOverview/OCSHealth/HealthBody.js

@@ -0,0 +1,49 @@
+import React from 'react';


We should extract some parts of Health to Dashboard but I think this is fine for now, we can do it as a follow up

atiratree · 2019-04-05T12:07:00Z

src/utils/status/pv/constants.js

+export const PV_STATUS_DEFAULT = 'PV_STATUS_DEFAULT';
+
+export const PV_STATUS_ALL_ERROR = [PV_STATUS_FAILED];
+export const PV_STATUS_ALL_PROGRESS = [PV_STATUS_AVAILABLE];


does it make sense to have Available PV shown as inProgress?

@suomiy The PV that are not bound and are available to be claimed I put them under this category. I was also bit confused about it, if you can provide some feedback on what should be shown in this category.

atiratree · 2019-04-05T12:09:27Z

src/components/StorageOverview/Inventory/utils.js

@@ -0,0 +1,74 @@
+import { getVmStatus, VM_STATUS_ALL_ERROR, VM_STATUS_ALL_PROGRESS } from '../../../utils/status/vm';


maybe mapStatuses and resolveStatusResult could be moved. But I am not sure about the other mapXToProps. Shouldn't these be different for each dashboard?

atiratree · 2019-04-05T12:10:10Z

src/components/StorageOverview/Inventory/Inventory.js

+    <InventoryRow title="Nodes" {...mapNodesToProps(nodes)} />
+    <InventoryRow title="Pods" {...mapPodsToProps(pods)} />
+    <InventoryRow title="PVs" {...mapPvsToProps(pvs)} />
+    <InventoryRow title="PVCs" {...mapPvcsToProps(pvcs)} />


what is the point of showing the same information in both dashboards?

I guess there will be overlapping information since Cluster Overview is very general and other dashboards will want to focus more on some specifics but still show important parts like PVs and PVCs in case of Storage.

atiratree · 2019-04-05T12:11:41Z

src/components/StorageOverview/OCSHealth/HealthBody.js

+  </div>
+);
+
+const OCSHealthMap = {


maybe it would make sense to make a status component out of this

+1 lets do this when we refactor Health Card

- Moved Inventory utils to common place

cloudbehl · 2019-04-08T06:03:29Z

@suomiy @rawagner updated the PR.

kubevirt-bot added the size/XXL label Apr 4, 2019

cloudbehl mentioned this pull request Apr 4, 2019

Details, Inventory card and Health card for Storage Dashboard kubevirt/web-ui#260

Merged

cloudbehl force-pushed the storage-dashboard-changes branch 2 times, most recently from b8f25e5 to a4082b5 Compare April 5, 2019 06:31

cloudbehl force-pushed the storage-dashboard-changes branch from a4082b5 to d700c65 Compare April 5, 2019 06:43

rawagner suggested changes Apr 5, 2019

View reviewed changes

rawagner mentioned this pull request Apr 5, 2019

Created details and inventory cards for storage dashboard view #288

Closed

atiratree suggested changes Apr 5, 2019

View reviewed changes

cloudbehl force-pushed the storage-dashboard-changes branch 3 times, most recently from ad167e3 to 0c35f96 Compare April 7, 2019 07:59

Added inventory, health and details card.

d9ad2ee

- Moved Inventory utils to common place

cloudbehl force-pushed the storage-dashboard-changes branch from 0c35f96 to d9ad2ee Compare April 7, 2019 08:26

rawagner approved these changes Apr 8, 2019

View reviewed changes

rawagner merged commit 1084c0a into kubevirt:master Apr 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Details, Inventory card and Health card for Storage Dashboard #311

Details, Inventory card and Health card for Storage Dashboard #311

cloudbehl commented Apr 4, 2019 •

edited

Loading

coveralls commented Apr 4, 2019 •

edited

Loading

cloudbehl commented Apr 4, 2019

cloudbehl commented Apr 5, 2019

rawagner Apr 5, 2019

rawagner Apr 5, 2019

rawagner Apr 5, 2019

rawagner Apr 5, 2019

atiratree Apr 5, 2019

rawagner Apr 5, 2019

atiratree Apr 5, 2019

cloudbehl Apr 6, 2019 •

edited

Loading

rawagner Apr 5, 2019

rawagner Apr 5, 2019

atiratree Apr 5, 2019

cloudbehl Apr 6, 2019

rawagner Apr 8, 2019

rawagner Apr 5, 2019

atiratree Apr 5, 2019

cloudbehl Apr 6, 2019

atiratree Apr 5, 2019

atiratree Apr 5, 2019

rawagner Apr 5, 2019

atiratree Apr 5, 2019

rawagner Apr 8, 2019

cloudbehl commented Apr 8, 2019

		@@ -0,0 +1,74 @@
		import { getVmStatus, VM_STATUS_ALL_ERROR, VM_STATUS_ALL_PROGRESS } from '../../../utils/status/vm';

Details, Inventory card and Health card for Storage Dashboard #311

Details, Inventory card and Health card for Storage Dashboard #311

Conversation

cloudbehl commented Apr 4, 2019 • edited Loading

coveralls commented Apr 4, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1271

💛 - Coveralls

cloudbehl commented Apr 4, 2019

cloudbehl commented Apr 5, 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

Choose a reason for hiding this comment

cloudbehl Apr 6, 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

cloudbehl commented Apr 8, 2019

cloudbehl commented Apr 4, 2019 •

edited

Loading

coveralls commented Apr 4, 2019 •

edited

Loading

cloudbehl Apr 6, 2019 •

edited

Loading