-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(ui): added notification rule & checks last run status & error #16338
Changes from all commits
4bfbf1e
dd6c809
c96a64f
d2d06f5
0432784
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,7 +147,9 @@ func Test_newNotificationRuleResponses(t *testing.T) { | |
], | ||
"type": "slack", | ||
"updatedAt": "0001-01-01T00:00:00Z", | ||
"status": "active" | ||
"status": "active", | ||
"latestCompleted": "0001-01-01T00:00:00Z", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spacing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hoorayimhelping go won't allow for weird formatting. This may look strange but it's kosher in the actual files There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i know gfmt controls spacing, but those things can get weird sometimes, it's always good to confirm and not just trust the machines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for sure. It's all good |
||
"latestScheduled": "0001-01-01T00:00:00Z" | ||
}, | ||
{ | ||
"createdAt": "0001-01-01T00:00:00Z", | ||
|
@@ -170,7 +172,9 @@ func Test_newNotificationRuleResponses(t *testing.T) { | |
"runbookLink": "", | ||
"type": "pagerduty", | ||
"updatedAt": "0001-01-01T00:00:00Z", | ||
"status": "active" | ||
"status": "active", | ||
"latestCompleted": "0001-01-01T00:00:00Z", | ||
"latestScheduled": "0001-01-01T00:00:00Z" | ||
} | ||
] | ||
}`, | ||
|
@@ -287,7 +291,9 @@ func Test_newNotificationRuleResponse(t *testing.T) { | |
} | ||
], | ||
"type": "slack", | ||
"updatedAt": "0001-01-01T00:00:00Z" | ||
"updatedAt": "0001-01-01T00:00:00Z", | ||
"latestCompleted": "0001-01-01T00:00:00Z", | ||
"latestScheduled": "0001-01-01T00:00:00Z" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spacing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
}`, | ||
}, | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,6 +182,18 @@ export const saveCheckFromTimeMachine = () => async ( | |
} | ||
} | ||
|
||
export const updateCheck = (check: Partial<Check>) => async ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was previously removed and caused a breaking change. It has been added back here with the intention of refactoring this implementation with a different ticket There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this still cause a breaking change? can you link the relevant tickets please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, when I had originally submitted the comment I hadn't had the chance to write up the issue. Here's the breaking change PR: #16333 and here's the issue that is intended on resolving it : |
||
dispatch: Dispatch<Action | NotificationAction> | ||
) => { | ||
const resp = await api.putCheck({checkID: check.id, data: check as Check}) | ||
if (resp.status === 200) { | ||
dispatch(setCheck(resp.data)) | ||
} else { | ||
throw new Error(resp.data.message) | ||
} | ||
dispatch(setCheck(resp.data)) | ||
} | ||
|
||
const updateCheckFromTimeMachine = async (check: Check) => { | ||
// todo: refactor after https://github.com/influxdata/influxdb/issues/16317 | ||
const getCheckResponse = await api.getCheck({checkID: check.id}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import {withRouter, WithRouterProps} from 'react-router' | |
import {SlideToggle, ComponentSize, ResourceCard} from '@influxdata/clockface' | ||
import CheckCardContext from 'src/alerting/components/CheckCardContext' | ||
import InlineLabels from 'src/shared/components/inlineLabels/InlineLabels' | ||
import LastRunTaskStatus from 'src/shared/components/lastRunTaskStatus/LastRunTaskStatus' | ||
|
||
// Constants | ||
import {DEFAULT_CHECK_NAME} from 'src/alerting/constants' | ||
|
@@ -170,7 +171,13 @@ const CheckCard: FunctionComponent<Props> = ({ | |
/> | ||
} | ||
metaData={[ | ||
<>Last completed at {check.latestCompleted}</>, | ||
<>{relativeTimestampFormatter(check.updatedAt, 'Last updated ')}</>, | ||
<LastRunTaskStatus | ||
key={2} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this a react warning or a lint error? if react is generating a warning then that's the issue we should address. if lint is mistakenly saying we need a key, we should comment an override here, because it's adding meaning to the code to get around a tool issue. do you understand the difference? a real react warning about a list item not having a key is something we should fix, a mistaken lint error trying to head off that warning is something we should comment about, but not change our code to make go away. if it's an actual react warning, we should give this a more unique name, if possible. react says to only use the item's list index as a last resort
https://reactjs.org/docs/lists-and-keys.html#keys same comment and any changes that need to happen apply to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was a lint error - i imagine stemming from the way |
||
lastRunError={check.lastRunError} | ||
lastRunStatus={check.lastRunStatus} | ||
/>, | ||
]} | ||
/> | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import {withRouter, WithRouterProps} from 'react-router' | |
import {SlideToggle, ComponentSize, ResourceCard} from '@influxdata/clockface' | ||
import NotificationRuleCardContext from 'src/alerting/components/notifications/RuleCardContext' | ||
import InlineLabels from 'src/shared/components/inlineLabels/InlineLabels' | ||
import LastRunTaskStatus from 'src/shared/components/lastRunTaskStatus/LastRunTaskStatus' | ||
|
||
// Constants | ||
import {DEFAULT_NOTIFICATION_RULE_NAME} from 'src/alerting/constants' | ||
|
@@ -165,7 +166,13 @@ const RuleCard: FC<Props> = ({ | |
/> | ||
} | ||
metaData={[ | ||
<>Last completed at {rule.latestCompleted}</>, | ||
<>{relativeTimestampFormatter(rule.updatedAt, 'Last updated ')}</>, | ||
<LastRunTaskStatus | ||
key={2} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
lastRunError={rule.lastRunError} | ||
lastRunStatus={rule.lastRunStatus} | ||
/>, | ||
]} | ||
/> | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there's some spacing issue here?