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

Add Status Card to Persistent Dashboard #2897

Merged

Conversation

afreen23
Copy link
Contributor

@afreen23 afreen23 commented Oct 3, 2019

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard labels Oct 3, 2019
@afreen23 afreen23 force-pushed the status-card-persistent branch 3 times, most recently from 50847aa to c8120d6 Compare October 3, 2019 10:59
const resiliencyProgressError = prometheusResults.getIn([
DATA_RESILIENCY_QUERIES[StorageDashboardQuery.RESILIENCY_PROGRESS],
'loadError',
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add data type here as well similar to other prometheus response.

Copy link
Contributor Author

@afreen23 afreen23 Oct 4, 2019

Choose a reason for hiding this comment

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

It comes as an empty string and a TypeError object.
Is it necessary to define a type for this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC @rawagner told me that Prometheus error can be arbitrary (any type), so I'd just use as PrometheusResponse for the query results and leave query error objects untyped.

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

@@ -48,3 +51,12 @@ export const getOCSVersion = (items: FirehoseResult): string => {
);
return _.get(operator, 'status.currentCSV');
};

export const getResiliencyProgress = (results: PrometheusResponse[]): number => {
const progress = getGaugeValue(results[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO its better to reuse existing getInstantVectorStats. You wont have to create your own selector

Suggested change
const progress = getGaugeValue(results[0]);
const progress = getInstantVectorStats(result[0])[0];

Copy link
Contributor Author

@afreen23 afreen23 Oct 4, 2019

Choose a reason for hiding this comment

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

getInstantVectorStats is for graphs. Then we need to get the value as getInstantVectorStats(result[0])[0].y .This looks a bit hackish but imposes re usability.
Either change it to _.get(results[0], 'data.result[0].value[1]') only ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@andybraren
Copy link
Contributor

andybraren commented Oct 4, 2019

Looking great @afreen23! A few minor things I notice:

  1. "View more" in the alerts should be "View details" instead since they take the user to the alert's Details page. (Sorry @rawagner, I somehow missed this in your original Status card PR)
  2. In the Not available state, the question mark icon in the center should be the same unknown icon used in the status bar.
  3. The center icon's color (both okay and unknown) should be pf-black-400 to be a little less visually distracting.

FYI @yuvalgalanti

@afreen23
Copy link
Contributor Author

afreen23 commented Oct 4, 2019

@gnehapk @rawagner @andybraren made the suggested changes

Copy link
Contributor

@andybraren andybraren left a comment

Choose a reason for hiding this comment

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

One last tiny thing and then I'm happy 😄 LGTM! Thank you!

@@ -48,7 +48,7 @@ export const StatusItem: React.FC<StatusItemProps> = ({

export const AlertItem: React.FC<AlertItemProps> = ({ alert }) => {
const LinkComponent = React.useCallback(
() => <Link to={alertURL(alert, alert.rule.id)}>View more</Link>,
() => <Link to={alertURL(alert, alert.rule.id)}>View Details</Link>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
() => <Link to={alertURL(alert, alert.rule.id)}>View Details</Link>,
() => <Link to={alertURL(alert, alert.rule.id)}>View details</Link>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the reviews @andybraren I have updated the PR. :)

const resiliencyProgressError = prometheusResults.getIn([
DATA_RESILIENCY_QUERIES[StorageDashboardQuery.RESILIENCY_PROGRESS],
'loadError',
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC @rawagner told me that Prometheus error can be arbitrary (any type), so I'd just use as PrometheusResponse for the query results and leave query error objects untyped.

emptyMessage="No persistent storage alerts"
>
{alerts.length
? alerts.map((alert) => <AlertItem key={alertURL(alert, alert.rule.id)} alert={alert} />)
Copy link
Contributor

Choose a reason for hiding this comment

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

{alerts.length > 0 && <AlertItem key={alertURL(alert, alert.rule.id)} alert={alert} />}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally favor alerts.length > 0 && xxx instead of alerts.length && xxx but I'm OK with the latter form as well.

@gnehapk
Copy link
Contributor

gnehapk commented Oct 9, 2019

@afreen23 Please resolve the conflicts.

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. component/shared Related to console-shared labels Oct 9, 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 Oct 9, 2019
@gnehapk
Copy link
Contributor

gnehapk commented Oct 10, 2019

/approve

import { PrometheusResponse } from '@console/internal/components/graphs';

export const getResiliencyProgress = (results: PrometheusResponse): number => {
const progress = _.get(results, 'data.result[0].value[1]');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you make this as a selector? This is being used at multiple [places in both dashboards.

Copy link
Contributor

Choose a reason for hiding this comment

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

can be done in separate PR, which will change all occurrences of a single stat values.

error={alertsResponseError}
emptyMessage="No persistent storage alerts"
>
{alerts.length &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to check for alerts array emptiness?

We could simply do:

{alerts.map((alert) => <AlertItem key={alertURL(alert, alert.rule.id)} alert={alert} />)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ActivityBody component shows "no persistent storage alerts" only when it gets a falsy value .
The map function will return [], which will be truthy.
That's why adding a condition on length.
Reference

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, it's clear to me now.

DashboardItemProps,
} from '@console/internal/components/dashboard/with-dashboard-resources';
import {
DATA_RESILIENCY_QUERY,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be DATA_RESILIENCY_QUERIES (plural) ?

Copy link
Contributor Author

@afreen23 afreen23 Oct 10, 2019

Choose a reason for hiding this comment

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

It will be singular actually, changed below as well.
The recently merged patch makes it a single query now.
Reference

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks for clarification.

My understanding was that <NAME>_QUERIES constants in queries.ts files were, by convention, maps (query name to its value) which is why I suggested the plural.

emptyMessage="No persistent storage alerts"
>
{alerts.length
? alerts.map((alert) => <AlertItem key={alertURL(alert, alert.rule.id)} alert={alert} />)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally favor alerts.length > 0 && xxx instead of alerts.length && xxx but I'm OK with the latter form as well.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2019
@vojtechszocs
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, gnehapk, vojtechszocs

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-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit f91b378 into openshift:master Oct 10, 2019
@spadgett spadgett added this to the v4.3 milestone Oct 15, 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. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/shared Related to console-shared 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

10 participants