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.

Sign up for GitHub

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

Jump to bottom

Updated Alertmanager Critical, Default, and Watchdog Receiver InfoTips #4795

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Updated Alertmanager Critical, Default, and Watchdog Receiver InfoTips #4795

merged 1 commit into from
Mar 27, 2020

Conversation

dtaylor113
Copy link
Contributor

Updated InfoTips for Alertmanager Receiver configuration.
Design Doc

Receivers Table InfoTip

Configure Multiple Receivers
image

Configure Single Receiver
image
All Required Receivers Configured (no InfoTip)
image

Default Receiver InfoTip

image
InfoTip remains after Default Receiver configured
image

Critical Receiver InfoTip

image
InfoTip not shown after receiver configured
image

Watchdog Receiver InfoTip

image
InfoTip not shown after receiver configured
image

JIRA: https://issues.redhat.com/browse/CONSOLE-2055

@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/monitoring Related to monitoring labels Mar 23, 2020
@cshinn
Copy link

cshinn commented Mar 23, 2020 •
edited
Loading

I just realized that the copy for the default receiver doesn't really make sense now that we provide multiple receivers at the same time; there isn't really a "first" one anymore. I'll work on some updated text. Looks good other than that though

@dtaylor113
Copy link
Contributor Author

dtaylor113 commented Mar 23, 2020 •
edited
Loading

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2020
@dtaylor113
Copy link
Contributor Author

...I'll work on some updated text.

Thanks @cshinn

@dtaylor113
Copy link
Contributor Author

Hi @cshinn, wondering if Watchdog should end with "...this receiver should remain in its current state with no Receiver Type selected." -or something to that extent -thanks

@dtaylor113 dtaylor113 force-pushed the update_initial_three_alert_receivers branch 2 times, most recently from 10b97a8 to 432fd50 Compare March 23, 2020 19:21
@cshinn
Copy link

cshinn commented Mar 24, 2020

@dtaylor113
Default Receiver:
Your default receiver will automatically receive all alerts from this cluster that are not caught by other receivers first.

Watchdog:
The Watchdog alert fires constantly to confirm that your alerting stack is functioning correctly. This receiver is configured to prevent it from creating unnecessary notifications. You can edit this receiver if you plan to use the information that Watchdog provides, otherwise this receiver should remain in its current state with no set receiver type.

@dtaylor113 dtaylor113 force-pushed the update_initial_three_alert_receivers branch from 432fd50 to 289d8f3 Compare March 24, 2020 13:14
@dtaylor113
Copy link
Contributor Author

@cshinn, thanks for the updates. Latest screenshots:
image
image

@dtaylor113
Copy link
Contributor Author

dtaylor113 commented Mar 24, 2020 •
edited
Loading

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2020
benjaminapetersen
benjaminapetersen reviewed Mar 25, 2020
View reviewed changes
frontend/public/components/monitoring/alert-manager-config.tsx Outdated
@@ -279,7 +286,7 @@ const ReceiverTableRow: React.FC<ReceiverTableRowProps> = ({
<TableRow id={index} index={index} trKey={key} style={style}>
<TableData className={tableColumnClasses[0]}>{receiver.name}</TableData>
<TableData className={tableColumnClasses[1]}>
{!integrationTypesLabel ? (
{(receiver.name === 'Critical' || receiver.name === 'Default') && !integrationTypesLabel ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

I see .name === 'Critical' several times across these files. Do you want to extract into a isCritical(thingWithName) func, and then make the Critical and Default an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added InitialReceivers enum. Not sure isCritical() is needed as the conditional seems easier to read to me, is only used in two places, and matches how we test for Default initial receiver.

benjaminapetersen
benjaminapetersen reviewed Mar 25, 2020
View reviewed changes
frontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx Outdated
@@ -173,6 +173,37 @@ const getRouteLabelsForEditor = (
: routeLabels;
};

const ReceiverInfoTip: React.FC<ReceiverInfoTipProps> = ({ type }) => {
let msg: string = '';
Copy link
Contributor

@benjaminapetersen benjaminapetersen Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How about dropping the let and the break statements:

case 'Default':
   return 'Your default receiver will automatically receive al.....'
case 'Critical':
   return 'The routing labels for this receiv....'
case 'Watchdog':
  return 'The Watchdog alert fires consta....'

via an alertMsg() func?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, much cleaner -thanks

@dtaylor113 dtaylor113 force-pushed the update_initial_three_alert_receivers branch from 289d8f3 to bbcbb2d Compare March 26, 2020 13:33
@dtaylor113
Copy link
Contributor Author

/retest

2 similar comments
@dtaylor113
Copy link
Contributor Author

/retest

@dtaylor113
Copy link
Contributor Author

/retest

TheRealJon
TheRealJon reviewed Mar 27, 2020
View reviewed changes
frontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx Outdated
@@ -580,3 +600,7 @@ type SaveAsDefaultCheckboxProps = {
dispatchFormChange: Function;
tooltip: string;
};

type ReceiverInfoTipProps = {
type: InitialReceivers.Watchdog | InitialReceivers.Critical | InitialReceivers.Default;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this just be:

Suggested change
type: InitialReceivers.Watchdog | InitialReceivers.Critical | InitialReceivers.Default;
type: InitialReceivers;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, thanks, fixed :-)

TheRealJon
TheRealJon approved these changes Mar 27, 2020
View reviewed changes
Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

Just one small nit.

@dtaylor113 dtaylor113 force-pushed the update_initial_three_alert_receivers branch from bbcbb2d to 65decc4 Compare March 27, 2020 18:21
benjaminapetersen
benjaminapetersen approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@benjaminapetersen benjaminapetersen left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, dtaylor113, TheRealJon

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2020
@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 c30741e into openshift:master Mar 27, 2020
@spadgett spadgett added this to the v4.5 milestone Mar 31, 2020
@dtaylor113 dtaylor113 deleted the update_initial_three_alert_receivers branch April 27, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminapetersen benjaminapetersen benjaminapetersen approved these changes

@TheRealJon TheRealJon TheRealJon approved these changes

@spadgett spadgett Awaiting requested review from spadgett

@cshinn cshinn Awaiting requested review from cshinn

@jcaianirh jcaianirh Awaiting requested review from jcaianirh

Assignees

@benjaminapetersen benjaminapetersen

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/monitoring Related to monitoring lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.5
Development

Successfully merging this pull request may close these issues.
8 participants
@dtaylor113 @cshinn @openshift-ci-robot @openshift-bot @benjaminapetersen @TheRealJon @spadgett @openshift-merge-robot