Health and Details card for ceph dashboard #1803

cloudbehl · 2019-06-24T12:29:03Z

Dependent: #1742

jelkosz · 2019-06-24T12:35:30Z

can you please add also a screenshot?

cloudbehl · 2019-06-24T12:39:59Z

@jelkosz Ack will do that. Fixing lint errors will update them in a while.

rawagner · 2019-06-24T12:51:40Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

no need for { and <>

rawagner · 2019-06-24T12:53:55Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

Suggested change

title="version"

title="Version"

rawagner · 2019-06-25T18:51:40Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

Suggested change

}, [watchK8sResource, stopWatchK8sResource, watchURL, stopWatchURL]);

}, [watchK8sResource, stopWatchK8sResource]);

rawagner · 2019-06-25T18:54:44Z

frontend/packages/ceph-storage-plugin/src/constants/index.ts

Suggested change

export const CEPH_ERROR = 'Ceph health is in error state';

export const CEPH_ERROR = 'Ceph health is in an error state';

@rawagner Messages are going to change a bit more. I am a discussion with the UX team and will send a separate PR fixing the message.

rawagner · 2019-06-25T19:49:20Z

...packages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/health-card.tsx

no need to export

rawagner · 2019-06-25T19:55:17Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

watchURL and stopWatchURL is never used

rawagner · 2019-06-25T19:56:57Z

...packages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/health-card.tsx

this does not seem correct, ocsResponse is not SystemHealth[]

rawagner · 2019-06-25T19:57:31Z

frontend/packages/ceph-storage-plugin/src/plugin.ts

Suggested change

'./components/dashboard-page/storage-dashboard/health-card' /* webpackChunkName: "health-card" */

'./components/dashboard-page/storage-dashboard/health-card' /* webpackChunkName: "storage-health-card" */

rawagner · 2019-06-25T19:57:44Z

frontend/packages/ceph-storage-plugin/src/plugin.ts

Suggested change

'./components/dashboard-page/storage-dashboard/details-card' /* webpackChunkName: "details-card" */

'./components/dashboard-page/storage-dashboard/details-card' /* webpackChunkName: "storage-details-card" */

rawagner · 2019-06-25T19:58:23Z

frontend/packages/console-app/package.json

I wouldn't enable storage plugin yet

vojtechszocs · 2019-06-26T15:40:35Z

Is this ready for review?

rawagner · 2019-06-26T16:43:11Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

Please remove the style, its not supposed to be here. I also need to remove it from Overview's Health card

Removed in #1833

@rawagner done

spadgett · 2019-06-26T19:08:43Z

/approve
/assign @vojtechszocs

rawagner · 2019-07-01T13:15:03Z

/lgtm
@vojtechszocs

openshift-bot · 2019-07-01T17:06:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-07-01T17:45:09Z

/retest

Please review the full test history for this PR and help us cut down flakes.

vojtechszocs · 2019-07-02T16:23:54Z

...packages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/health-card.tsx

Nit: no need for trailing /index, this can be

import { CEPH_HEALTHY, CEPH_DEGRADED, CEPH_ERROR, CEPH_UNKNOWN } from '../../../constants';

vojtechszocs · 2019-07-02T16:32:23Z

...packages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/health-card.tsx

As discussed in #1873 (comment), code under packages should converge towards "single default component export per file" convention.

If this was a default export, you wouldn't have to give it an explicit name, i.e.

export default withDashboardResources(HealthCard);

It's up to you.

You can always add packages/ceph-storage-plugin/.eslintrc that extends packages/.eslintrc.js and tweak rules as necessary, although in the long term, all code under packages should converge to the same conventions and rule config.

vojtechszocs · 2019-07-02T16:39:09Z

frontend/packages/ceph-storage-plugin/src/constants/storage.ts

Why not src/components/dashboard-page/storage-dashboard/queries.ts just like in #1732?

export const StorageHealthQueries = { CEPH_STATUS_QUERY: 'ceph_health_status', };

This way, you can co-locate query definitions with the corresponding Dashboard tab (in this case, storage-dashboard).

vojtechszocs · 2019-07-02T16:40:18Z

frontend/packages/ceph-storage-plugin/src/plugin.ts

I'd suggest adding plugin ID to webpackChunkName, e.g. ceph-storage-health-card or similar.

vojtechszocs · 2019-07-02T16:40:39Z

frontend/packages/ceph-storage-plugin/src/plugin.ts

Same as above 😃

cloudbehl · 2019-07-03T11:30:31Z

@vojtechszocs updated the PR please have a look.

rawagner · 2019-07-08T10:12:30Z

/lgtm

openshift-bot · 2019-07-08T10:17:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-07-08T11:45:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2019-07-08T12:44:52Z

...ackages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/details-card.tsx

nit: we try to keep the imports alphabetized in new code

spadgett · 2019-07-08T14:09:11Z

...packages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/health-card.tsx

Why is this an object instead of an array?

Agree with Sam, this should be an array, with individual elements of type CephHealth.

const cephHealthValues: CephHealth[] = [ { state: HealthState.OK, message: CEPH_HEALTHY, }, // the rest of values ];

openshift-bot · 2019-07-08T15:50:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

vojtechszocs · 2019-07-08T16:42:53Z

...packages/ceph-storage-plugin/src/components/dashboard-page/storage-dashboard/health-card.tsx

Agree with Sam, this should be an array, with individual elements of type CephHealth.

const cephHealthValues: CephHealth[] = [ { state: HealthState.OK, message: CEPH_HEALTHY, }, // the rest of values ];

cloudbehl · 2019-07-08T18:23:26Z

@spadgett @vojtechszocs Updated the code, please look and thanks for pointing. #1803 (comment))

Signed-off-by: Ankush Behl <cloudbehl@gmail.com>

rawagner · 2019-07-09T14:53:01Z

/lgtm

openshift-ci-robot · 2019-07-09T14:53:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cloudbehl, rawagner, spadgett

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/packages/ceph-storage-plugin/OWNERS~~ [cloudbehl,rawagner,spadgett]

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

openshift-bot · 2019-07-09T15:13:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-07-09T16:20:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2019-07-09T21:00:57Z

@cloudbehl: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`218e777`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot requested review from spadgett and zherman0 June 24, 2019 12:29

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 24, 2019

cloudbehl mentioned this pull request Jun 24, 2019

Add plugin infra for adding new dashboard tabs/cards #1742

Merged

cloudbehl changed the title ~~Health and details card for ceph dashboard~~ [WIP] Health and details card for ceph dashboard Jun 24, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2019

cloudbehl force-pushed the health-details-card branch 2 times, most recently from aad5ebd to 60ab862 Compare June 24, 2019 12:56

spadgett added this to the v4.2 milestone Jun 24, 2019

cloudbehl force-pushed the health-details-card branch 3 times, most recently from 7c51f1f to c5bde2b Compare June 25, 2019 19:36

cloudbehl changed the title ~~[WIP] Health and details card for ceph dashboard~~ Health and Details card for ceph dashboard Jun 25, 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 Jun 25, 2019

rawagner suggested changes Jun 25, 2019

View reviewed changes

cloudbehl force-pushed the health-details-card branch from c5bde2b to f4c26ce Compare June 26, 2019 06:54

rawagner suggested changes Jun 26, 2019

View reviewed changes

openshift-ci-robot assigned vojtechszocs Jun 26, 2019

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

cloudbehl force-pushed the health-details-card branch 2 times, most recently from 7528d5c to bde6e5d Compare July 1, 2019 13:13

openshift-ci-robot assigned rawagner Jul 1, 2019

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

vojtechszocs suggested changes Jul 2, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2019

cloudbehl force-pushed the health-details-card branch from b6eb552 to 990d5ec Compare July 3, 2019 03:07

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 3, 2019

cloudbehl force-pushed the health-details-card branch 2 times, most recently from e36ca40 to bb20d3d Compare July 3, 2019 11:27

cloudbehl force-pushed the health-details-card branch from bb20d3d to b4d270b Compare July 8, 2019 09:07

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

cloudbehl mentioned this pull request Jul 8, 2019

Added alerts for ceph-storage-tab #1950

Merged

spadgett reviewed Jul 8, 2019

View reviewed changes

vojtechszocs suggested changes Jul 8, 2019

View reviewed changes

cloudbehl force-pushed the health-details-card branch from b4d270b to a75aca3 Compare July 8, 2019 18:20

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019

cloudbehl force-pushed the health-details-card branch from a75aca3 to ed67bd6 Compare July 8, 2019 18:26

Details and health card for ceph dashboard plugin

218e777

Signed-off-by: Ankush Behl <cloudbehl@gmail.com>

cloudbehl force-pushed the health-details-card branch from ed67bd6 to 218e777 Compare July 9, 2019 13:34

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

openshift-merge-robot merged commit fc53e3c into openshift:master Jul 9, 2019

	}, [watchK8sResource, stopWatchK8sResource, watchURL, stopWatchURL]);
	}, [watchK8sResource, stopWatchK8sResource]);

	export const CEPH_ERROR = 'Ceph health is in error state';
	export const CEPH_ERROR = 'Ceph health is in an error state';

	'./components/dashboard-page/storage-dashboard/health-card' /* webpackChunkName: "health-card" */
	'./components/dashboard-page/storage-dashboard/health-card' /* webpackChunkName: "storage-health-card" */

Health and Details card for ceph dashboard #1803

Health and Details card for ceph dashboard #1803

Uh oh!

Conversation

cloudbehl commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelkosz commented Jun 24, 2019

Uh oh!

cloudbehl commented Jun 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtechszocs commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Jun 26, 2019

Uh oh!

rawagner commented Jul 1, 2019

Uh oh!

openshift-bot commented Jul 1, 2019

Uh oh!

openshift-bot commented Jul 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudbehl commented Jul 3, 2019

Uh oh!

rawagner commented Jul 8, 2019

Uh oh!

openshift-bot commented Jul 8, 2019

Uh oh!

openshift-bot commented Jul 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Jul 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudbehl commented Jul 8, 2019

Uh oh!

rawagner commented Jul 9, 2019

cloudbehl commented Jun 24, 2019 •

edited

Loading