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

Fix tooltips messages in the graphs at DR Policy dashboard #1061

Conversation

@TimothyAsirJeyasing
Copy link
Contributor Author

@GowthamShanmugam Please review

@openshift-merge-robot
Copy link
Contributor

@TimothyAsirJeyasing: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/odf-console-e2e-aws 3dc217c link true /test odf-console-e2e-aws

Full PR test history. Your PR dashboard.

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.

@SanjalKatiyar
Copy link
Collaborator

plz add screenshots to the PR (all pages where changes have been made)...

<p className="odf-dashboard-body">
Note: Only inbound values are displayed exclusively for block
volumes (ceph rbd) and include snapshots from all applications
excluding apps using file voluems (cephfs).
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
excluding apps using file voluems (cephfs).
excluding apps using file volumes (cephfs).

Copy link
Contributor

Choose a reason for hiding this comment

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

Here all text under Note should be muted

@@ -16,6 +19,12 @@ export const AlertsCard: React.FC = () => {
alerts={alerts}
AlertItemComponent={AlertItem}
alertsFilter={filterDRAlerts}
titleToolTip={
<Trans t={t}>
<b>Note:</b> Alerts are deployed for both <b>ApplicationsSet</b>{' '}
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
<b>Note:</b> Alerts are deployed for both <b>ApplicationsSet</b>{' '}
<b>Note:</b> Alerts are deployed for both <b>ApplicationSet</b>{' '}

<p className="odf-dashboard-body">
Note: Only inbound values are displayed exclusively for block
volumes (ceph rbd) and include snapshots from all applications
excluding apps using file voluems (cephfs).
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
excluding apps using file voluems (cephfs).
excluding apps using file volumes (cephfs).

This graph provides an overview of the total number of snapshots
synced across your clusters.
</p>
<p className="odf-dashboard-body">
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
<p className="odf-dashboard-body">
<p className="text-muted mco-cluster-app__text--margin-top">

incase mco-cluster-app__text--margin-top is giving too much space try to create new class

for the cluster i.e, rate at which data is replicated within the
cluster.
</p>
<p className="odf-dashboard-body">
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
<p className="odf-dashboard-body">

refer: https://github.com/red-hat-storage/odf-console/pull/1061/files#r1343941666

<FieldLevelHelp>
<Trans t={t}>
<b>Note:</b> The applications count displayed herein pertain
exclusively to <b>ApplicationsSet</b> type applications.
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
exclusively to <b>ApplicationsSet</b> type applications.
exclusively to <b>ApplicationSet</b> type applications.

Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is still active

@GowthamShanmugam
Copy link
Contributor

GowthamShanmugam commented Oct 3, 2023

You missed two changes from UX:

  1. Change Application: All ApplicationSet (On application name dropdown)
  2. The Total PVC count card has been changed to Protected PVCs and Protected PVCs with issues. (Remove PVCsSection completely and re-use ProtectedPVCsSection component), Also move ProtectedPVCsSection to common.tsx

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch 2 times, most recently from 39006b2 to 6280876 Compare October 3, 2023 15:27
@TimothyAsirJeyasing
Copy link
Contributor Author

Screenshot from 2023-10-03 20-54-31
Screenshot from 2023-10-03 20-55-04
Screenshot from 2023-10-03 20-55-22
Screenshot from 2023-10-03 20-45-29

@@ -16,6 +19,12 @@ export const AlertsCard: React.FC = () => {
alerts={alerts}
AlertItemComponent={AlertItem}
alertsFilter={filterDRAlerts}
titleToolTip={
<Trans t={t}>
<b>Note:</b> Alerts are deployed for both <b>ApplicationSet</b>{' '}
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
<b>Note:</b> Alerts are deployed for both <b>ApplicationSet</b>{' '}
<b>Note:</b> Alerts are displayed for both <b>ApplicationSet</b>{' '}

protectedPVCData={protectedPVCData}
clusterName={clusterName}
selectedAppSet={null}
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
selectedAppSet={null}

Copy link
Contributor

Choose a reason for hiding this comment

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

check this comment

Comment on lines 353 to 371
const updateProtectedPVC = React.useCallback(() => {
const placementInfo = selectedAppSet?.placementInfo?.[0];

const issueCount =
protectedPVCData?.reduce((acc, protectedPVCItem) => {
const drpcChecks =
selectedAppSet &&
protectedPVCItem?.drpcName === placementInfo?.drpcName &&
protectedPVCItem?.drpcNamespace === placementInfo?.drpcNamespace;

const replicationHealth = getVolumeReplicationHealth(
getTimeDifferenceInSeconds(protectedPVCItem?.lastSyncTime),
protectedPVCItem?.schedulingInterval
)[0];

if (
(!selectedAppSet &&
replicationHealth !== VOLUME_REPLICATION_HEALTH.HEALTHY) ||
(selectedAppSet &&
drpcChecks &&
replicationHealth !== VOLUME_REPLICATION_HEALTH.HEALTHY)
) {
return acc + 1;
} else {
return acc;
}
}, 0) || 0;

setProtectedPVC([protectedPVCData?.length || 0, issueCount]);
}, [selectedAppSet, protectedPVCData, setProtectedPVC]);
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
const updateProtectedPVC = React.useCallback(() => {
const placementInfo = selectedAppSet?.placementInfo?.[0];
const issueCount =
protectedPVCData?.reduce((acc, protectedPVCItem) => {
const drpcChecks =
selectedAppSet &&
protectedPVCItem?.drpcName === placementInfo?.drpcName &&
protectedPVCItem?.drpcNamespace === placementInfo?.drpcNamespace;
const replicationHealth = getVolumeReplicationHealth(
getTimeDifferenceInSeconds(protectedPVCItem?.lastSyncTime),
protectedPVCItem?.schedulingInterval
)[0];
if (
(!selectedAppSet &&
replicationHealth !== VOLUME_REPLICATION_HEALTH.HEALTHY) ||
(selectedAppSet &&
drpcChecks &&
replicationHealth !== VOLUME_REPLICATION_HEALTH.HEALTHY)
) {
return acc + 1;
} else {
return acc;
}
}, 0) || 0;
setProtectedPVC([protectedPVCData?.length || 0, issueCount]);
}, [selectedAppSet, protectedPVCData, setProtectedPVC]);
const updateProtectedPVC = React.useCallback(() => {
const placementInfo = selectedAppSet?.placementInfo?.[0];
const issueCount =
protectedPVCData?.reduce((acc, protectedPVCItem) => {
const replicationHealth = getVolumeReplicationHealth(
getTimeDifferenceInSeconds(protectedPVCItem?.lastSyncTime),
protectedPVCItem?.schedulingInterval
)[0];
!!selectedAppSet :
protectedPVCItem?.drpcName === placementInfo?.drpcName &&
protectedPVCItem?.drpcNamespace === placementInfo?.drpcNamespace &&
replicationHealth !== VOLUME_REPLICATION_HEALTH.HEALTHY && acc ++
? replicationHealth !== VOLUME_REPLICATION_HEALTH.HEALTHY && acc ++
return acc;
}, 0) || 0;
setProtectedPVC([protectedPVCData?.length || 0, issueCount]);
}, [selectedAppSet, protectedPVCData, setProtectedPVC]);

<FieldLevelHelp>
<Trans t={t}>
<b>Note:</b> The applications count displayed herein pertain
exclusively to <b>ApplicationsSet</b> type applications.
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is still active

@@ -1,4 +1,4 @@
export const ALL_APPS = 'All applications';
export const ALL_APPS = 'All applicationSet';
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
export const ALL_APPS = 'All applicationSet';
export const ALL_APPS = 'All ApplicationSet';

@GowthamShanmugam
Copy link
Contributor

Screenshot from 2023-10-03 20-54-31 Screenshot from 2023-10-03 20-55-04 Screenshot from 2023-10-03 20-55-22 Screenshot from 2023-10-03 20-45-29

Test this with some affected PVC, I want to see the PVC count (Total, as well as affected)here for the cluster and application view.

Create at least two apps under the same cluster and protect PVCs. one app with PVC replication affected and the other one with all healthy.

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch from 6280876 to 863b650 Compare October 4, 2023 11:00
@TimothyAsirJeyasing
Copy link
Contributor Author

image

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch from 863b650 to 4de03e6 Compare October 4, 2023 12:12
@TimothyAsirJeyasing
Copy link
Contributor Author

/retest

@GowthamShanmugam
Copy link
Contributor

/cherry-pick release-4.14

@openshift-cherrypick-robot

@GowthamShanmugam: once the present PR merges, I will cherry-pick it on top of release-4.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.14

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.

@GowthamShanmugam
Copy link
Contributor

/cherry-pick release-4.14-compatibility

@openshift-cherrypick-robot

@GowthamShanmugam: once the present PR merges, I will cherry-pick it on top of release-4.14-compatibility in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.14-compatibility

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.

@GowthamShanmugam
Copy link
Contributor

/approve

@GowthamShanmugam
Copy link
Contributor

/retest

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch from 4de03e6 to d7b9561 Compare October 5, 2023 11:25
@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch from d7b9561 to b7e5c7e Compare October 6, 2023 12:01
@openshift-ci openshift-ci bot removed the lgtm label Oct 6, 2023
@TimothyAsirJeyasing
Copy link
Contributor Author

Screenshot from 2023-10-06 17-26-12
Screenshot from 2023-10-06 17-26-33
Screenshot from 2023-10-06 17-27-46
Screenshot from 2023-10-06 17-28-03

titleToolTip={
<Trans t={t}>
The graph displays the total number of block volumes inbound
snapshots, by cluster, from all AppicationSet and Subscription
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
snapshots, by cluster, from all AppicationSet and Subscription
snapshots, by cluster, from all ApplicationSet and Subscription

Comment on lines 327 to 332
<Trans t={t}>
The graph displays the total number of block volumes inbound
snapshots, by cluster, from all AppicationSet and Subscription
type applications. Applications that use file volumes are excluded
in the total snapshot count.
</Trans>
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
<Trans t={t}>
The graph displays the total number of block volumes inbound
snapshots, by cluster, from all AppicationSet and Subscription
type applications. Applications that use file volumes are excluded
in the total snapshot count.
</Trans>
t("
The graph displays the total number of block volumes inbound
snapshots, by cluster, from all AppicationSet and Subscription
type applications. Applications that use file volumes are excluded
in the total snapshot count.")

try t instead of , we dont have any tags inside the string. And test once how it looks like in tooltip,

Comment on lines 344 to 349
<Trans t={t}>
The graph displays the average replication throughput inbound, by
cluster, from all ApplicationSet and Subscription type
applications. Applications that use file volumes are excluded in
the replication throughput.
</Trans>
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, use t instead of

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch from b7e5c7e to d25df0f Compare October 6, 2023 13:12
@TimothyAsirJeyasing
Copy link
Contributor Author

/retest

import {
HealthSection,
PeerConnectionSection,
ApplicationsSection,
PVCsSection,
// PVCsSection,
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
// PVCsSection,

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch from d25df0f to ad9e5b7 Compare October 9, 2023 09:11
@openshift-ci openshift-ci bot added the lgtm label Oct 9, 2023
@GowthamShanmugam
Copy link
Contributor

/hold cancel

@GowthamShanmugam
Copy link
Contributor

/hold

@@ -228,33 +221,6 @@ export const ApplicationsSection: React.FC<ApplicationsSectionProps> = ({
);
};

export const PVCsSection: React.FC<PVCsSectionProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove dead code for PVCsSection component(All count_persistentvolumeclaim_total metrics-related queries have to be removed).

@TimothyAsirJeyasing TimothyAsirJeyasing force-pushed the bz-2239101-2239093-2239096 branch from ad9e5b7 to 7a65f75 Compare October 9, 2023 09:39
@openshift-ci openshift-ci bot removed the lgtm label Oct 9, 2023
@openshift-ci openshift-ci bot added the lgtm label Oct 9, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GowthamShanmugam, SanjalKatiyar, TimothyAsirJeyasing

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

@GowthamShanmugam
Copy link
Contributor

/ hold cancel

@GowthamShanmugam
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot merged commit 1ac3c70 into red-hat-storage:master Oct 9, 2023
3 checks passed
@openshift-cherrypick-robot

@GowthamShanmugam: new pull request created: #1070

In response to this:

/cherry-pick release-4.14

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.

@openshift-cherrypick-robot

@GowthamShanmugam: new pull request created: #1071

In response to this:

/cherry-pick release-4.14-compatibility

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants