From 790ada913e382c4dd0ce04df8e9ef3302d0e7779 Mon Sep 17 00:00:00 2001 From: tidy-dev Date: Wed, 10 Nov 2021 12:30:08 -0500 Subject: [PATCH 1/4] Flatten check run names to use Workflow app names --- app/src/lib/api.ts | 1 - app/src/lib/ci-checks/ci-checks.ts | 60 ++------- .../check-runs/ci-check-run-actions-logs.tsx | 28 +---- .../ui/check-runs/ci-check-run-list-item.tsx | 11 +- app/src/ui/check-runs/ci-check-run-list.tsx | 117 ++++-------------- app/src/ui/check-runs/ci-check-run-logs.tsx | 112 ++--------------- .../ui/check-runs/ci-check-run-popover.tsx | 18 +-- 7 files changed, 63 insertions(+), 284 deletions(-) diff --git a/app/src/lib/api.ts b/app/src/lib/api.ts index 726877ffa69..1598398a064 100644 --- a/app/src/lib/api.ts +++ b/app/src/lib/api.ts @@ -336,7 +336,6 @@ export interface IAPIRefCheckRun { readonly status: APICheckStatus readonly conclusion: APICheckConclusion | null readonly name: string - readonly output: IAPIRefCheckRunOutput readonly check_suite: IAPIRefCheckRunCheckSuite readonly app: IAPIRefCheckRunApp readonly completed_at: string diff --git a/app/src/lib/ci-checks/ci-checks.ts b/app/src/lib/ci-checks/ci-checks.ts index e68e7117d43..33f20aa4eca 100644 --- a/app/src/lib/ci-checks/ci-checks.ts +++ b/app/src/lib/ci-checks/ci-checks.ts @@ -29,42 +29,16 @@ export interface IRefCheck { readonly status: APICheckStatus readonly conclusion: APICheckConclusion | null readonly appName: string - readonly checkSuiteId: number | null // API status don't have check suite id's - readonly output: IRefCheckOutput readonly htmlUrl: string | null + + // Following are action check specific + readonly checkSuiteId: number | null + readonly actionJobSteps?: ReadonlyArray readonly actionsWorkflowRunId?: number + readonly actionsWorkflowName?: string | null readonly logs_url?: string } -/** - * There are two types of check run outputs. - * - * 1. From GitHub Actions, which comes in steps with individual log texts, - * statuses, and duration info. - * 2. From any other check run app, which comes with a generic string of - * whatever the check run app provides. - */ -export type IRefCheckOutput = - | { - readonly title: string | null - readonly summary?: string | null - readonly type: RefCheckOutputType.Actions - readonly steps: ReadonlyArray - } - | { - readonly title: string | null - readonly summary?: string | null - readonly type: RefCheckOutputType.Default - // This text is whatever a check run app decides to place in it. - // It may include html. - readonly text: string | null - } - -export enum RefCheckOutputType { - Actions = 'Actions', - Default = 'Default', -} - /** * A combined view of all legacy commit statuses as well as * check runs for a particular Git reference. @@ -135,11 +109,6 @@ export function apiStatusToRefCheck(apiStatus: IAPIRefStatusItem): IRefCheck { conclusion, appName: '', checkSuiteId: null, - output: { - type: RefCheckOutputType.Default, - title: null, - text: null, - }, htmlUrl: apiStatus.target_url, } } @@ -261,10 +230,6 @@ export function apiCheckRunToRefCheck(checkRun: IAPIRefCheckRun): IRefCheck { conclusion: checkRun.conclusion, appName: checkRun.app.name, checkSuiteId: checkRun.check_suite.id, - output: { - ...checkRun.output, - type: RefCheckOutputType.Default, - }, htmlUrl: checkRun.html_url, } } @@ -427,11 +392,7 @@ export async function getLatestPRWorkflowRunsLogsForCheckRun( mappedCheckRuns.push({ ...cr, htmlUrl: matchingJob.html_url, - output: { - ...cr.output, - type: RefCheckOutputType.Actions, - steps: matchingJob.steps, - }, + actionJobSteps: matchingJob.steps, }) continue @@ -452,11 +413,7 @@ export async function getLatestPRWorkflowRunsLogsForCheckRun( mappedCheckRuns.push({ ...cr, htmlUrl: matchingJob.html_url, - output: { - ...cr.output, - type: RefCheckOutputType.Actions, - steps: await parseJobStepLogs(logZip, matchingJob), - }, + actionJobSteps: await parseJobStepLogs(logZip, matchingJob), }) } @@ -550,10 +507,11 @@ function getCheckRunWithActionsJobAndLogURLs( continue } - const { id, logs_url } = matchingWR + const { id, name, logs_url } = matchingWR mappedCheckRuns.push({ ...cr, actionsWorkflowRunId: id, + actionsWorkflowName: name, logs_url, }) } diff --git a/app/src/ui/check-runs/ci-check-run-actions-logs.tsx b/app/src/ui/check-runs/ci-check-run-actions-logs.tsx index 67b2145ed85..c522bdef343 100644 --- a/app/src/ui/check-runs/ci-check-run-actions-logs.tsx +++ b/app/src/ui/check-runs/ci-check-run-actions-logs.tsx @@ -15,9 +15,7 @@ import { import { IAPIWorkflowJobStep } from '../../lib/api' import { getFormattedCheckRunDuration, - IRefCheckOutput, isFailure, - RefCheckOutputType, } from '../../lib/ci-checks/ci-checks' import { enableCICheckRunsLogs } from '../../lib/feature-flag' @@ -26,8 +24,7 @@ const MAX_LOG_LINE_NUMBER_WIDTH = 100 // arbitrarily chosen const INDIVIDUAL_LOG_LINE_NUMBER_WIDTH_ALLOWED = 10 interface ICICheckRunActionLogsProps { - /** The check run to display **/ - readonly output: IRefCheckOutput + readonly actionSteps: ReadonlyArray } interface ICICheckRunActionLogsState { @@ -45,10 +42,7 @@ export class CICheckRunActionLogs extends React.PureComponent< super(props) const openSections = new Set() - const firstFailedSectionIndex = - props.output.type === RefCheckOutputType.Actions - ? props.output.steps.findIndex(isFailure) - : -1 + const firstFailedSectionIndex = this.props.actionSteps.findIndex(isFailure) openSections.add(firstFailedSectionIndex) @@ -59,15 +53,8 @@ export class CICheckRunActionLogs extends React.PureComponent< } public componentDidUpdate(prevProps: ICICheckRunActionLogsProps) { - if ( - prevProps.output.type === this.props.output.type || - this.props.output.type !== RefCheckOutputType.Actions - ) { - return - } - const openSections = new Set() - const firstFailedSectionIndex = this.props.output.steps.findIndex(isFailure) + const firstFailedSectionIndex = this.props.actionSteps.findIndex(isFailure) openSections.add(firstFailedSectionIndex) @@ -333,14 +320,9 @@ export class CICheckRunActionLogs extends React.PureComponent< } public render() { - const { output } = this.props - - if (output.type !== RefCheckOutputType.Actions) { - // This shouldn't happen, should only be provided actions type - return <>Unable to load logs. - } + const { actionSteps } = this.props - return output.steps.map((step, i) => { + return actionSteps.map((step, i) => { const isSkipped = step.conclusion === 'skipped' const showLogs = this.state.openSections.has(i) && !isSkipped diff --git a/app/src/ui/check-runs/ci-check-run-list-item.tsx b/app/src/ui/check-runs/ci-check-run-list-item.tsx index bc6ad3e71e8..78a9dc58b6e 100644 --- a/app/src/ui/check-runs/ci-check-run-list-item.tsx +++ b/app/src/ui/check-runs/ci-check-run-list-item.tsx @@ -124,6 +124,9 @@ export class CICheckRunListItem extends React.PureComponent< public render() { const { checkRun, showLogs, baseHref } = this.props + const name = checkRun.actionsWorkflowName + ? `${checkRun.actionsWorkflowName} / ${checkRun.name}` + : checkRun.name return ( <>
- {checkRun.name} + {name}
{checkRun.description}
@@ -155,7 +158,9 @@ export class CICheckRunListItem extends React.PureComponent< />
- {showLogs && !this.isLoading() ? ( + {showLogs && + !this.isLoading() && + checkRun.actionJobSteps !== undefined ? ( - readonly appNames: ReadonlyArray } /** The CI Check list. */ @@ -46,12 +40,6 @@ export class CICheckRunList extends React.PureComponent< } public componentDidUpdate(prevProps: ICICheckRunListProps) { - // Currently, this updates if we retreive from api, thus memory ref check is - // appropriate. - if (prevProps.checkRuns === this.props.checkRuns) { - return - } - this.setState( this.setupStateAfterCheckRunPropChange(this.props, this.state) ) @@ -61,77 +49,42 @@ export class CICheckRunList extends React.PureComponent< props: ICICheckRunListProps, currentState: ICICheckRunListState | null ): ICICheckRunListState { - const checksByApp = _.groupBy(props.checkRuns, 'appName') - if (checksByApp[''] !== undefined) { - checksByApp['Other'] = checksByApp[''] - delete checksByApp[''] - } - const appNames = this.getSortedAppNames(checksByApp) - - let checkRunLogsShown = - currentState !== null ? currentState.checkRunLogsShown : null - let checkRunsShown = - currentState !== null ? currentState.checkRunsShown : null + let checkRunExpanded = + currentState !== null ? currentState.checkRunExpanded : null if (currentState === null || !currentState.hasUserToggledCheckRun) { - // If there is a failure, we want the first app and first check run with a - // failure, to be opened so the user doesn't have to click through to find - // it. - const firstFailureAppName = appNames.find(an => { - checkRunLogsShown = - checksByApp[an].find(isFailure)?.id.toString() ?? null - return checkRunLogsShown !== null - }) - - // If there are no failures, just show the first app open. Let user pick - // which logs they would like look it. (checkRunLogsShown = null) - checkRunsShown = - firstFailureAppName !== undefined ? firstFailureAppName : appNames[0] + // If there is a failure, we want the first check run with a failure, to + // be opened so the user doesn't have to click through to find it. + // Otherwise, just open the first one. (Only actions type can be expanded.) + const firstFailure = props.checkRuns.find( + cr => isFailure(cr) && cr.actionJobSteps !== undefined + ) + checkRunExpanded = + firstFailure !== undefined + ? firstFailure.id.toString() + : props.checkRuns[0].id.toString() } return { - checkRunsShown, - checkRunLogsShown, - checksByApp, - appNames, + checkRunExpanded, hasUserToggledCheckRun: currentState ? currentState.hasUserToggledCheckRun : false, } } - private getSortedAppNames(checksByApp: _.Dictionary): string[] { - return Object.keys(checksByApp) - .sort((a, b) => b.length - a.length) - .map(name => (name !== '' ? name : 'Other')) - } - private onCheckRunClick = (checkRun: IRefCheck): void => { this.setState({ - checkRunLogsShown: - this.state.checkRunLogsShown === checkRun.id.toString() + checkRunExpanded: + this.state.checkRunExpanded === checkRun.id.toString() ? null : checkRun.id.toString(), hasUserToggledCheckRun: true, }) } - private onAppHeaderClick = (appName: string) => { - return () => { - this.setState({ - checkRunsShown: this.state.checkRunsShown === appName ? '' : appName, - hasUserToggledCheckRun: true, - }) - } - } - - private renderList = (appName: string): JSX.Element | null => { - const { checkRunsShown, checksByApp } = this.state - if (checkRunsShown !== appName) { - return null - } - - const list = checksByApp[appName].map((c, i) => { + private renderList = (): JSX.Element | null => { + const list = [...this.props.checkRuns].sort().map((c, i) => { return ( {list} } - private renderCheckAppHeader = (appName: string): JSX.Element => { - const { checkRunsShown } = this.state - + public render() { return ( -
- -
{appName}
+
+
{this.renderList()}
) } - - public render() { - const { appNames } = this.state - - const checkLists = appNames.map(appName => ( -
- {this.renderCheckAppHeader(appName)} - {this.renderList(appName)} -
- )) - - return
{checkLists}
- } } diff --git a/app/src/ui/check-runs/ci-check-run-logs.tsx b/app/src/ui/check-runs/ci-check-run-logs.tsx index 4e24d3c863d..2c145ea3422 100644 --- a/app/src/ui/check-runs/ci-check-run-logs.tsx +++ b/app/src/ui/check-runs/ci-check-run-logs.tsx @@ -1,17 +1,7 @@ import * as React from 'react' -import { - IRefCheck, - IRefCheckOutput, - RefCheckOutputType, -} from '../../lib/ci-checks/ci-checks' +import { IRefCheck } from '../../lib/ci-checks/ci-checks' import classNames from 'classnames' import { CICheckRunActionLogs } from './ci-check-run-actions-logs' -import { SandboxedMarkdown } from '../lib/sandboxed-markdown' -import { enableCICheckRunsLogs } from '../../lib/feature-flag' -import { LinkButton } from '../lib/link-button' -import { encodePathAsUrl } from '../../lib/path' - -const PaperStackImage = encodePathAsUrl(__dirname, 'static/paper-stack.svg') interface ICICheckRunLogsProps { /** The check run to display **/ @@ -36,102 +26,16 @@ interface ICICheckRunLogsProps { /** The CI check list item. */ export class CICheckRunLogs extends React.PureComponent { - private isOutputToDisplay(output: IRefCheckOutput): boolean { - return ( - output.type === RefCheckOutputType.Default && - (output.text === null || output.text.trim() === '') && - (output.summary === undefined || - output.summary === null || - output.summary.trim() === '') - ) - } - - private getNonActionsOutputMD( - output: IRefCheckOutput, - checkRunName: string - ): string | null { - const { title, summary } = output - - const titleOutput = - title !== null && - title.trim() !== '' && - title.trim().toLocaleLowerCase() !== - checkRunName.trim().toLocaleLowerCase() - ? `### ${title.trim()}\n` - : '' - - const summaryOutput = - summary !== null && summary !== undefined && summary.trim() !== '' - ? summary - : '' - - const mainOutput = - output.type !== RefCheckOutputType.Actions && output.text !== null - ? output.text.trim() - : '' - - const combinedOutput = titleOutput + summaryOutput + mainOutput - return combinedOutput === '' ? null : combinedOutput - } - - private renderNonActionsLogOutput = ( - output: IRefCheckOutput, - checkRunName: string - ) => { - if (this.isOutputToDisplay(output) || !enableCICheckRunsLogs()) { - return this.renderEmptyLogOutput() - } - - const markdown = this.getNonActionsOutputMD(output, checkRunName) - if (output.type === RefCheckOutputType.Actions || markdown === null) { - return null - } - - return ( - - ) - } - - private renderEmptyLogOutput = () => { - return ( -
-
- There is no output data to display for this check. -
- - {this.props.checkRun.htmlUrl !== null - ? 'View check details' - : 'View check pull request'} - -
-
- -
- ) - } - - private hasActionsWorkflowLogs() { - return this.props.checkRun.actionsWorkflowRunId !== undefined - } - public render() { const { - checkRun: { output, name }, + checkRun: { actionJobSteps }, } = this.props - const logsOutput = this.hasActionsWorkflowLogs() ? ( - - ) : ( - this.renderNonActionsLogOutput(output, name) - ) + if (actionJobSteps === undefined) { + return + } - const className = classNames('ci-check-list-item-logs', { - actions: this.hasActionsWorkflowLogs(), - }) + const className = classNames('ci-check-list-item-logs', 'actions') return (
{ onMouseOver={this.props.onMouseOver} onMouseLeave={this.props.onMouseLeave} > -
{logsOutput}
+
+ +
) } diff --git a/app/src/ui/check-runs/ci-check-run-popover.tsx b/app/src/ui/check-runs/ci-check-run-popover.tsx index 949b9777908..ffa845c5bf1 100644 --- a/app/src/ui/check-runs/ci-check-run-popover.tsx +++ b/app/src/ui/check-runs/ci-check-run-popover.tsx @@ -299,14 +299,16 @@ export class CICheckRunPopover extends React.PureComponent<
{this.renderRerunButton()} - + {!loadingActionLogs ? ( + + ) : null} ) From 5b923fda8a416082c9c9edcd882a5dd4e553e9d4 Mon Sep 17 00:00:00 2001 From: tidy-dev Date: Wed, 10 Nov 2021 17:47:37 -0500 Subject: [PATCH 2/4] Why no Code scanning results? --- app/src/ui/check-runs/ci-check-run-list-item.tsx | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/app/src/ui/check-runs/ci-check-run-list-item.tsx b/app/src/ui/check-runs/ci-check-run-list-item.tsx index 78a9dc58b6e..e5585d444f0 100644 --- a/app/src/ui/check-runs/ci-check-run-list-item.tsx +++ b/app/src/ui/check-runs/ci-check-run-list-item.tsx @@ -124,9 +124,14 @@ export class CICheckRunListItem extends React.PureComponent< public render() { const { checkRun, showLogs, baseHref } = this.props - const name = checkRun.actionsWorkflowName - ? `${checkRun.actionsWorkflowName} / ${checkRun.name}` - : checkRun.name + const wfName = + checkRun.actionsWorkflowName !== undefined + ? checkRun.actionsWorkflowName + : checkRun.appName === 'GitHub Code Scanning' + ? 'Code scanning results' // seems this is hardcoded on dotcom too :/ + : undefined + const name = + wfName !== undefined ? `${wfName} / ${checkRun.name}` : checkRun.name return ( <>
Date: Wed, 10 Nov 2021 17:48:11 -0500 Subject: [PATCH 3/4] Fix where forked repo pr branches fail..... --- app/src/ui/toolbar/branch-dropdown.tsx | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/app/src/ui/toolbar/branch-dropdown.tsx b/app/src/ui/toolbar/branch-dropdown.tsx index f929e7128bd..ff29ec85489 100644 --- a/app/src/ui/toolbar/branch-dropdown.tsx +++ b/app/src/ui/toolbar/branch-dropdown.tsx @@ -254,8 +254,18 @@ export class BranchDropdown extends React.Component< private renderPopover() { const pr = this.props.currentPullRequest const { tip } = this.props.repositoryState.branchesState - // It is ok if it doesn't exist, we just can't retrieve actions workflows - const currentBranchName = tip.kind === TipState.Valid ? tip.branch.name : '' + // This is used for retrieving the PR's action check runs (if exist). For + // forked repo PRs, we must use the upstreamWithoutRemote as we make are own + // temporary branch in Desktop for these that doesn't exist remotely (and + // thus doesn't exist in action's world). The upstreamWIthoutRemote will + // match a non forked PR. It _should_ only be null for a local branch.. + // which _should_ not happen in this context. But, worst case, the user + // simply won't be able to retreive action steps and will get check run list + // items that are given for non-action checks. + const currentBranchName = + tip.kind === TipState.Valid + ? tip.branch.upstreamWithoutRemote ?? tip.branch.name + : '' if (pr === null) { return null From b289bdf3f1f45b9b7f85d8afd31b35f484bd25a5 Mon Sep 17 00:00:00 2001 From: tidy-dev Date: Wed, 10 Nov 2021 18:00:51 -0500 Subject: [PATCH 4/4] Sort checks alphabetically --- app/src/lib/ci-checks/ci-checks.ts | 15 ++++++++ .../ui/check-runs/ci-check-run-list-item.tsx | 14 +++---- app/src/ui/check-runs/ci-check-run-list.tsx | 38 +++++++++++-------- 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/app/src/lib/ci-checks/ci-checks.ts b/app/src/lib/ci-checks/ci-checks.ts index 33f20aa4eca..247524d0559 100644 --- a/app/src/lib/ci-checks/ci-checks.ts +++ b/app/src/lib/ci-checks/ci-checks.ts @@ -530,3 +530,18 @@ export function getFormattedCheckRunDuration( .duration(getCheckDurationInSeconds(checkRun), 'seconds') .format('d[d] h[h] m[m] s[s]', { largest: 4 }) } + +/** Get check run display name + * + * Goal: Action Workflow Name / workflow run name + * If no workflow run name (non-actions check), then just return the name. + */ +export function getCheckRunDisplayName(checkRun: IRefCheck): string { + const wfName = + checkRun.actionsWorkflowName !== undefined + ? checkRun.actionsWorkflowName + : checkRun.appName === 'GitHub Code Scanning' + ? 'Code scanning results' // seems this is hardcoded on dotcom too :/ + : undefined + return wfName !== undefined ? `${wfName} / ${checkRun.name}` : checkRun.name +} diff --git a/app/src/ui/check-runs/ci-check-run-list-item.tsx b/app/src/ui/check-runs/ci-check-run-list-item.tsx index e5585d444f0..6790571c2a4 100644 --- a/app/src/ui/check-runs/ci-check-run-list-item.tsx +++ b/app/src/ui/check-runs/ci-check-run-list-item.tsx @@ -1,5 +1,8 @@ import * as React from 'react' -import { IRefCheck } from '../../lib/ci-checks/ci-checks' +import { + getCheckRunDisplayName, + IRefCheck, +} from '../../lib/ci-checks/ci-checks' import { Octicon } from '../octicons' import { getClassNameForCheck, getSymbolForCheck } from '../branches/ci-status' import classNames from 'classnames' @@ -124,14 +127,7 @@ export class CICheckRunListItem extends React.PureComponent< public render() { const { checkRun, showLogs, baseHref } = this.props - const wfName = - checkRun.actionsWorkflowName !== undefined - ? checkRun.actionsWorkflowName - : checkRun.appName === 'GitHub Code Scanning' - ? 'Code scanning results' // seems this is hardcoded on dotcom too :/ - : undefined - const name = - wfName !== undefined ? `${wfName} / ${checkRun.name}` : checkRun.name + const name = getCheckRunDisplayName(checkRun) return ( <>
{ - const list = [...this.props.checkRuns].sort().map((c, i) => { - return ( - + const list = [...this.props.checkRuns] + .sort((a, b) => + getCheckRunDisplayName(a).localeCompare(getCheckRunDisplayName(b)) ) - }) + .map((c, i) => { + return ( + + ) + }) return <>{list} }