-
Notifications
You must be signed in to change notification settings - Fork 44
Bug 1520857- add support in tc-web for displaying information from the lastFire table #413
base: master
Are you sure you want to change the base?
Changes from 8 commits
c57174f
7a33df8
5b5c97d
a154a50
f55678d
c5f5a16
87993e3
f98d385
4674382
0156365
e238f6f
8bb249f
2b28062
aa51ef9
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 |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import React, { Component, Fragment } from 'react'; | ||
import { Link } from 'react-router-dom'; | ||
import { string, bool, func, oneOfType, object } from 'prop-types'; | ||
import { string, bool, func, oneOfType, object, array } from 'prop-types'; | ||
import classNames from 'classnames'; | ||
import { equals, assocPath } from 'ramda'; | ||
import cloneDeep from 'lodash.clonedeep'; | ||
|
@@ -11,6 +11,8 @@ import List from '@material-ui/core/List'; | |
import ListItem from '@material-ui/core/ListItem'; | ||
import ListItemText from '@material-ui/core/ListItemText'; | ||
import ListSubheader from '@material-ui/core/ListSubheader'; | ||
import TableRow from '@material-ui/core/TableRow'; | ||
import TableCell from '@material-ui/core/TableCell'; | ||
import TextField from '@material-ui/core/TextField'; | ||
import Switch from '@material-ui/core/Switch'; | ||
import FormGroup from '@material-ui/core/FormGroup'; | ||
|
@@ -21,17 +23,19 @@ import Typography from '@material-ui/core/Typography'; | |
import FlashIcon from 'mdi-react/FlashIcon'; | ||
import PlusIcon from 'mdi-react/PlusIcon'; | ||
import DeleteIcon from 'mdi-react/DeleteIcon'; | ||
import LinkIcon from 'mdi-react/LinkIcon'; | ||
import ContentSaveIcon from 'mdi-react/ContentSaveIcon'; | ||
import { docs } from 'taskcluster-lib-urls'; | ||
import Label from '@mozilla-frontend-infra/components/Label'; | ||
import ErrorPanel from '../ErrorPanel'; | ||
import DataTable from '../DataTable'; | ||
import Button from '../Button'; | ||
import SpeedDial from '../SpeedDial'; | ||
import SpeedDialAction from '../SpeedDialAction'; | ||
import DialogAction from '../DialogAction'; | ||
import DateDistance from '../DateDistance'; | ||
import { HOOKS_LAST_FIRE_TYPE } from '../../utils/constants'; | ||
import { hook } from '../../utils/prop-types'; | ||
import removeKeys from '../../utils/removeKeys'; | ||
import ToggleView from '../ToggleView'; | ||
|
||
const initialHook = { | ||
metadata: { | ||
|
@@ -114,12 +118,24 @@ const initialHook = { | |
subheader: { | ||
fontSize: theme.typography.pxToRem(16), | ||
}, | ||
displayBlock: { | ||
display: 'block', | ||
}, | ||
errorTableCell: { | ||
whiteSpace: 'normal', | ||
}, | ||
errorPanel: { | ||
maxHeight: '300px', | ||
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. No need to put quotations when the units are
|
||
maxWidth: '75ch', | ||
overflowY: 'scroll', | ||
}, | ||
})) | ||
/** A form to view/edit/create a hook */ | ||
export default class HookForm extends Component { | ||
static defaultProps = { | ||
isNewHook: false, | ||
hook: initialHook, | ||
hookLastFires: null, | ||
onTriggerHook: null, | ||
onCreateHook: null, | ||
onUpdateHook: null, | ||
|
@@ -129,8 +145,12 @@ export default class HookForm extends Component { | |
}; | ||
|
||
static propTypes = { | ||
/** A GraphQL hook response. Not needed when creating a new hook */ | ||
/** Part of a GraphQL hook response containing info about that hook. | ||
Not needed when creating a new hook */ | ||
hook: hook.isRequired, | ||
/** Part of the same Grahql hook response as above containing info | ||
about some last hook fired attempts */ | ||
hookLastFires: array, | ||
/** Set to `true` when creating a new hook. */ | ||
isNewHook: bool, | ||
/** Callback function fired when a hook is triggered. */ | ||
|
@@ -153,6 +173,7 @@ export default class HookForm extends Component { | |
|
||
state = { | ||
hook: null, | ||
hookLastFires: null, | ||
// eslint-disable-next-line react/no-unused-state | ||
previousHook: null, | ||
taskInput: '', | ||
|
@@ -165,14 +186,18 @@ export default class HookForm extends Component { | |
}; | ||
|
||
static getDerivedStateFromProps(props, state) { | ||
if (equals(props.hook, state.previousHook)) { | ||
if ( | ||
equals(props.hook, state.previousHook) && | ||
equals(props.hookLastFires, state.hookLastFires) | ||
) { | ||
return null; | ||
} | ||
|
||
const hook = props.isNewHook ? initialHook : props.hook; | ||
|
||
return { | ||
hook: props.hook, | ||
hookLastFires: props.hookLastFires, | ||
previousHook: props.hook, | ||
taskInput: JSON.stringify( | ||
removeKeys(cloneDeep(hook.task), ['__typename']), | ||
|
@@ -412,10 +437,9 @@ export default class HookForm extends Component { | |
triggerSchemaInput, | ||
triggerContextInput, | ||
hook, | ||
hookLastFires, | ||
validation, | ||
} = this.state; | ||
/* eslint-disable-next-line no-underscore-dangle */ | ||
const lastFireTypeName = !isNewHook && hook.status.lastFire.__typename; | ||
|
||
return ( | ||
<Fragment> | ||
|
@@ -508,61 +532,70 @@ export default class HookForm extends Component { | |
/> | ||
</FormGroup> | ||
</ListItem> | ||
<ListItem className={classes.displayBlock}> | ||
<ListItemText | ||
primary={ | ||
<Typography variant="subtitle1"> | ||
Last Hook Fire Attempts | ||
</Typography> | ||
} | ||
/> | ||
{hookLastFires && ( | ||
<DataTable | ||
items={hookLastFires} | ||
headers={['Task ID', 'FiredBy', 'Result', 'Attempted', 'Error']} | ||
isPaginate | ||
renderRow={hookFire => ( | ||
<TableRow key={hookFire.taskId}> | ||
<TableCell> | ||
{(hookFire.result === 'SUCCESS' && ( | ||
<Link to={`/tasks/${hookFire.taskId}`}> | ||
<Typography>{hookFire.taskId}</Typography> | ||
</Link> | ||
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. Right now it looks like: It should look like this: This will make it look like any other link in a table to the rest of the app. Here's an example: The Worker ID column in the Workers view. 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. If the lastfire result is 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. From https://github.com/taskcluster/taskcluster-web/pull/413/files#r254810845, we could remove the link icon when the task has an error. |
||
)) || <Typography>{hookFire.taskId}</Typography>} | ||
</TableCell> | ||
<TableCell> | ||
<Typography>{hookFire.firedBy}</Typography> | ||
</TableCell> | ||
<TableCell> | ||
<Label mini status={hookFire.result.toLowerCase()}> | ||
{hookFire.result} | ||
</Label> | ||
</TableCell> | ||
<TableCell> | ||
<DateDistance from={hookFire.taskCreateTime} /> | ||
</TableCell> | ||
<TableCell className={classes.errorTableCell}> | ||
{(hookFire.result === 'ERROR' && ( | ||
<ToggleView | ||
panel={ | ||
<ErrorPanel | ||
className={classes.errorPanel} | ||
error={hookFire.error} | ||
onClose={null} | ||
/> | ||
} | ||
/> | ||
)) || <Typography>n/a</Typography>} | ||
</TableCell> | ||
</TableRow> | ||
)} | ||
/> | ||
)} | ||
</ListItem> | ||
{!isNewHook && ( | ||
<Fragment> | ||
<ListItem> | ||
<ListItemText | ||
primary="Last Fired" | ||
secondary={ | ||
lastFireTypeName === HOOKS_LAST_FIRE_TYPE.NO_FIRE ? ( | ||
'n/a' | ||
) : ( | ||
<DateDistance from={hook.status.lastFire.time} /> | ||
) | ||
} | ||
/> | ||
</ListItem> | ||
{lastFireTypeName === HOOKS_LAST_FIRE_TYPE.SUCCESSFUL_FIRE ? ( | ||
<ListItem | ||
button | ||
component={Link} | ||
className={classes.listItemButton} | ||
to={`/tasks/${hook.status.lastFire.taskId}`}> | ||
<ListItemText | ||
primary="Last Fired Result" | ||
secondary={hook.status.lastFire.taskId} | ||
/> | ||
<LinkIcon /> | ||
</ListItem> | ||
) : ( | ||
<ListItem> | ||
<ListItemText | ||
primary="Last Fired Result" | ||
secondary={ | ||
lastFireTypeName === HOOKS_LAST_FIRE_TYPE.NO_FIRE ? ( | ||
'n/a' | ||
) : ( | ||
<pre> | ||
{JSON.stringify(hook.status.lastFire.error, null, 2)} | ||
</pre> | ||
) | ||
} | ||
/> | ||
</ListItem> | ||
)} | ||
<ListItem> | ||
<ListItemText | ||
primary="Next Scheduled Fire" | ||
secondary={ | ||
hook.status.nextScheduledDate ? ( | ||
<DateDistance from={hook.status.nextScheduledDate} /> | ||
) : ( | ||
'n/a' | ||
) | ||
} | ||
/> | ||
</ListItem> | ||
</Fragment> | ||
<ListItem> | ||
<ListItemText | ||
primary="Next Scheduled Fire" | ||
secondary={ | ||
hook.status.nextScheduledDate ? ( | ||
<DateDistance from={hook.status.nextScheduledDate} /> | ||
) : ( | ||
'n/a' | ||
) | ||
} | ||
/> | ||
</ListItem> | ||
)} | ||
<List> | ||
<ListItem> | ||
|
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.
Why are we adding pagination? Is the last fire endpoint paginated (i.e., uses continuation token)?
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.
As per discussion with Dustin in irc we are keeping as many as 100 lastfire info rows for each hookId (expiration logic). Hence I think showing all rows at once won't be good.
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.
We also only purge those once per day, so a particularly active hook might have many more rows than that at some time during the day!