Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Added disks to the Inventory card #323

Merged
merged 1 commit into from
Apr 9, 2019
Merged

Conversation

cloudbehl
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Apr 8, 2019

Pull Request Test Coverage Report for Build 1086

  • 14 of 14 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 87.296%

Totals Coverage Status
Change from base Build 1297: -0.007%
Covered Lines: 3200
Relevant Lines: 3514

💛 - Coveralls

count: null,
};

const mapDiskToProps = disks => {
Copy link
Contributor

Choose a reason for hiding this comment

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

mapDisks or diskStats?

const STATUS_RESULT_ERROR = 'error';

const result = {
[STATUS_RESULT_OK]: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

you can import these constants from dashboard

if (cephOsdUpCount || cephOsdDownCount) {
result[STATUS_RESULT_OK] = cephOsdUpCount;
result[STATUS_RESULT_ERROR] = cephOsdDownCount;
result.count = result[STATUS_RESULT_OK] + result[STATUS_RESULT_ERROR];
Copy link
Contributor

Choose a reason for hiding this comment

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

rather result.count = cephOsdUpCount + cephOsdDownCount ?


export default [
{
component: DiskInventoryUtils,
Copy link
Contributor

Choose a reason for hiding this comment

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

DiskInventoryUtils is not a component. please delete this file

@@ -0,0 +1,26 @@
import { get } from 'lodash';

Copy link
Contributor

Choose a reason for hiding this comment

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

please rename this file to diskInventoryUtils. Only component files start with an uppercase.

import { Inventory } from '../Inventory';
import { default as InventoryFixtures } from '../fixtures/Inventory.fixture';

const testInventoryOverview = () => <Inventory {...InventoryFixtures[0].props} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a test for DiskInventoryUtils but for an Inventory component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, sorry! Stupid mistake :/

@cloudbehl
Copy link
Contributor Author

@suomiy Updated the PR please have a look

import { getCapacityStats } from '../../../selectors';
import { STATUS_RESULT_OK, STATUS_RESULT_ERROR } from '../../Dashboard/Inventory/utils';

const result = {
Copy link
Contributor

Choose a reason for hiding this comment

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

please put this inside the function. There is no reason to have it outside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack!

<React.Fragment>
<InventoryRow title="Nodes" {...mapNodesToProps(nodes)} />
<InventoryRow title="Disks" {...diskStatsToProps(disks)} />
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure here, but please be consistent. Can we rename the prop to diskStats when we have a function diskStatsToProps? Or the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack!

@rawagner rawagner merged commit 5015def into kubevirt:master Apr 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants