Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added top consumer to ceph storage plugin #2040

Merged
merged 1 commit into from Jul 21, 2019

Conversation

cloudbehl
Copy link
Contributor

@cloudbehl cloudbehl commented Jul 16, 2019

Screenshot from 2019-07-16 18-52-46

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 16, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019
@cloudbehl cloudbehl force-pushed the top-consumer branch 3 times, most recently from b6594c0 to 39d66d8 Compare July 16, 2019 13:34
@cloudbehl cloudbehl changed the title [WIP] Added top consumer to ceph storage plugin Added top consumer to ceph storage plugin Jul 16, 2019
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 16, 2019
display: flex;
margin: 0.6em 0em;
}
.ceph-top-consumer-card__dropdown-item {
Copy link
Contributor

Choose a reason for hiding this comment

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

add empty line between classes

import * as _ from 'lodash';
import { ChartLineIcon } from '@patternfly/react-icons';
import {
ChartGroup,
Copy link
Contributor

Choose a reason for hiding this comment

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

alpha (+ all other imports)

return dt.toString().substring(16, 21);
};

export const getGraphVectorStats: GetStats = (response, metricType, unit) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use getInstantVectorStats from graph/utils. You also dont need to specify GetStats as it is already in graph/utils too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rawagner can't use the getInstantVectorStats as it is for a single value doesn't work when multiple values are being returned.

Also the same for GetStats as in my case multiple PrometheusResponse are being returned and that is returning only one.


export const getMetricType = (resource, metricType) => _.get(resource, ['metric', metricType], '');

export const formatToShortTime = (timestamp: Date): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use twentyFourHourTime from utils/datetime to be consistent with rest of the console

<DashboardCard>
<DashboardCardHeader>
<DashboardCardTitle>Top Consumers</DashboardCardTitle>
<div className="ceph-top-consumer-card__dropdown">
Copy link
Contributor

Choose a reason for hiding this comment

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

height of the card header is inconsistent with others. Take a look at these styles https://github.com/openshift/console/pull/1722/files#diff-163991d4c119db922a729100ce2bea7fR91

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, those dropdowns are actually not in header as yours are.
@andybraren @matthewcarleton any help on how to include these dropdowns into PF4 Card Header and keep header height consistent ?

dropdowns-header

See the difference between Capacity and Top Consumers header

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @rawagner I believe you can use the actions area for the card header provided. That should fix the spacing isssues. If not we may need to investigate further if there is unnecessary spacing added to other elements in there.

@cloudbehl cloudbehl force-pushed the top-consumer branch 3 times, most recently from 87d0251 to 4ee5fc5 Compare July 17, 2019 13:11
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2019
@cloudbehl cloudbehl force-pushed the top-consumer branch 2 times, most recently from b05d010 to 76ac8ee Compare July 17, 2019 13:48
@cloudbehl
Copy link
Contributor Author

/test e2e-aws-console
/test e2e-aws-console-olm

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2019
@cloudbehl
Copy link
Contributor Author

/test e2e-aws-console
/test e2e-aws-console-olm

@jelkosz
Copy link

jelkosz commented Jul 18, 2019

/test e2e-aws

@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2019
@spadgett spadgett added this to the v4.2 milestone Jul 18, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2019
@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2019
Signed-off-by: Ankush Behl <cloudbehl@gmail.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2019
@rawagner
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-merge-robot openshift-merge-robot merged commit 0caff9e into openshift:master Jul 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants