From 51882b9b7bf81e2f5e9786ab7cf7d166dbc31a03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Louv-Jansen?= Date: Wed, 1 Apr 2020 11:19:18 +0200 Subject: [PATCH] Find related backports by looking at `mergedCommit.oid` (#173) - Find related backports by looking at `mergedCommit.oid` - Improve error reporting by writing the error.message in console and the rest in logs - Change `setInterval` to recursive `setTimeout` to avoid flooding with requests (mostly relevant for huge git repos) --- src/runWithArgs.ts | 8 ++++- src/runWithOptions.ts | 1 - src/services/git.ts | 6 ++-- .../fetchCommitsByAuthor.test.ts.snap | 3 ++ .../github/fetchCommitsByAuthor.test.ts | 6 ++-- src/services/github/fetchCommitsByAuthor.ts | 15 ++++++-- .../github/mocks/commitsByAuthorMock.ts | 6 ++++ ...rtPRsMock.ts => getCommitsByAuthorMock.ts} | 5 ++- .../github/mocks/getPullRequestEdgeMock.ts | 3 ++ src/services/logger.ts | 12 ++++++- .../__snapshots__/integration.test.ts.snap | 6 ++++ src/ui/cherrypickAndCreatePullRequest.test.ts | 24 +++++++++---- src/ui/cherrypickAndCreatePullRequest.ts | 36 +++++++++++++------ 13 files changed, 101 insertions(+), 30 deletions(-) rename src/services/github/mocks/{getExistingBackportPRsMock.ts => getCommitsByAuthorMock.ts} (90%) diff --git a/src/runWithArgs.ts b/src/runWithArgs.ts index 25bd6b1d..5d061fcc 100755 --- a/src/runWithArgs.ts +++ b/src/runWithArgs.ts @@ -3,6 +3,7 @@ import { runWithOptions } from './runWithOptions'; import { HandledError } from './services/HandledError'; import { initLogger } from './services/logger'; import { getLogfilePath } from './services/env'; +import chalk from 'chalk'; export async function runWithArgs(args: string[]) { const logger = initLogger(); @@ -14,8 +15,13 @@ export async function runWithArgs(args: string[]) { if (e instanceof HandledError) { console.error(e.message); } else { + console.error('\n'); + console.error(chalk.bold('⚠️ An unknown error occurred ⚠️')); + console.error(e.message); console.error( - `An unknown error occurred. Please check the logs for addtional details: ${getLogfilePath()}` + chalk.italic( + `Please check the logs for addtional details: ${getLogfilePath()}` + ) ); logger.info('Unknown error:'); logger.info(e); diff --git a/src/runWithOptions.ts b/src/runWithOptions.ts index 198a1d57..3e33f3dd 100755 --- a/src/runWithOptions.ts +++ b/src/runWithOptions.ts @@ -25,7 +25,6 @@ export async function runWithOptions(options: BackportOptions) { if (e instanceof HandledError) { console.error(e.message); } else { - console.error(e); throw e; } } diff --git a/src/services/git.ts b/src/services/git.ts index fbf455d1..3d7089ca 100644 --- a/src/services/git.ts +++ b/src/services/git.ts @@ -77,7 +77,7 @@ export async function deleteRemote( try { await exec(`git remote rm ${remoteName}`, { cwd: getRepoPath(options) }); } catch (e) { - const isExecError = e.code === 128; + const isExecError = e.cmd && e.code === 128; // re-throw if error is not an exec related error if (!isExecError) { throw e; @@ -111,7 +111,7 @@ export async function cherrypickContinue(options: BackportOptions) { cwd: getRepoPath(options), }); } catch (e) { - const isCherrypickError = e.code === 128; + const isCherrypickError = e.cmd && e.code === 128; if (!isCherrypickError) { throw e; } @@ -131,7 +131,7 @@ export async function getFilesWithConflicts(options: BackportOptions) { return []; } catch (e) { - const isConflictError = e.code === 2; + const isConflictError = e.cmd && e.code === 2; if (isConflictError) { const files = (e.stdout as string) .split('\n') diff --git a/src/services/github/__snapshots__/fetchCommitsByAuthor.test.ts.snap b/src/services/github/__snapshots__/fetchCommitsByAuthor.test.ts.snap index 251a3aba..e78e146d 100644 --- a/src/services/github/__snapshots__/fetchCommitsByAuthor.test.ts.snap +++ b/src/services/github/__snapshots__/fetchCommitsByAuthor.test.ts.snap @@ -60,6 +60,9 @@ Array [ name } number + mergeCommit { + oid + } timelineItems( last: 20 itemTypes: CROSS_REFERENCED_EVENT diff --git a/src/services/github/fetchCommitsByAuthor.test.ts b/src/services/github/fetchCommitsByAuthor.test.ts index de94813e..ba1e2a66 100644 --- a/src/services/github/fetchCommitsByAuthor.test.ts +++ b/src/services/github/fetchCommitsByAuthor.test.ts @@ -4,7 +4,7 @@ import { getExistingBackportPRs, } from './fetchCommitsByAuthor'; import { commitsWithPullRequestsMock } from './mocks/commitsByAuthorMock'; -import { getExistingBackportPRsMock } from './mocks/getExistingBackportPRsMock'; +import { getCommitsByAuthorMock } from './mocks/getCommitsByAuthorMock'; import { getDefaultOptions } from '../../test/getDefaultOptions'; import axios from 'axios'; import { getPullRequestEdgeMock } from './mocks/getPullRequestEdgeMock'; @@ -185,11 +185,11 @@ async function getExistingBackportsByRepoName( repoName1: string, repoName2: string ) { - const existingPrsMock = getExistingBackportPRsMock(repoName1); + const commitsMock = getCommitsByAuthorMock(repoName1); spyOn(axios, 'post').and.returnValues( { data: { data: currentUserMock } }, - { data: { data: existingPrsMock } } + { data: { data: commitsMock } } ); const options = getDefaultOptions({ diff --git a/src/services/github/fetchCommitsByAuthor.ts b/src/services/github/fetchCommitsByAuthor.ts index 1faff90c..91384446 100644 --- a/src/services/github/fetchCommitsByAuthor.ts +++ b/src/services/github/fetchCommitsByAuthor.ts @@ -53,6 +53,9 @@ export async function fetchCommitsByAuthor( name } number + mergeCommit { + oid + } timelineItems( last: 20 itemTypes: CROSS_REFERENCED_EVENT @@ -124,7 +127,8 @@ export async function fetchCommitsByAuthor( const associatedPullRequest = getAssociatedPullRequest( pullRequestEdge, - options + options, + sha ); const existingBackports = getExistingBackportPRs( @@ -161,11 +165,13 @@ function getPullNumberFromMessage(firstMessageLine: string) { function getAssociatedPullRequest( pullRequestEdge: PullRequestEdge | undefined, - options: BackportOptions + options: BackportOptions, + sha: string ) { const isAssociated = pullRequestEdge?.node.repository.name === options.repoName && - pullRequestEdge?.node.repository.owner.login === options.repoOwner; + pullRequestEdge?.node.repository.owner.login === options.repoOwner && + pullRequestEdge?.node.mergeCommit.oid === sha; if (isAssociated) { return pullRequestEdge; @@ -250,6 +256,9 @@ interface HistoryEdge { export interface PullRequestEdge { node: { number: number; + mergeCommit: { + oid: string; + }; repository: { owner: { login: string; diff --git a/src/services/github/mocks/commitsByAuthorMock.ts b/src/services/github/mocks/commitsByAuthorMock.ts index dc83a1aa..4b6d93b4 100644 --- a/src/services/github/mocks/commitsByAuthorMock.ts +++ b/src/services/github/mocks/commitsByAuthorMock.ts @@ -23,6 +23,9 @@ export const commitsWithPullRequestsMock: DataResponse = { edges: [ { node: { + mergeCommit: { + oid: 'f3b618b9421fdecdb36862f907afbdd6344b361d', + }, repository: { name: 'kibana', owner: { @@ -48,6 +51,9 @@ export const commitsWithPullRequestsMock: DataResponse = { edges: [ { node: { + mergeCommit: { + oid: '79cf18453ec32a4677009dcbab1c9c8c73fc14fe', + }, repository: { name: 'kibana', owner: { diff --git a/src/services/github/mocks/getExistingBackportPRsMock.ts b/src/services/github/mocks/getCommitsByAuthorMock.ts similarity index 90% rename from src/services/github/mocks/getExistingBackportPRsMock.ts rename to src/services/github/mocks/getCommitsByAuthorMock.ts index a311dbe1..7af8e135 100644 --- a/src/services/github/mocks/getExistingBackportPRsMock.ts +++ b/src/services/github/mocks/getCommitsByAuthorMock.ts @@ -1,6 +1,6 @@ import { DataResponse } from '../fetchCommitsByAuthor'; -export const getExistingBackportPRsMock = (repoName: string): DataResponse => ({ +export const getCommitsByAuthorMock = (repoName: string): DataResponse => ({ repository: { ref: { target: { @@ -15,6 +15,9 @@ export const getExistingBackportPRsMock = (repoName: string): DataResponse => ({ edges: [ { node: { + mergeCommit: { + oid: '79cf18453ec32a4677009dcbab1c9c8c73fc14fe', + }, repository: { name: repoName, owner: { diff --git a/src/services/github/mocks/getPullRequestEdgeMock.ts b/src/services/github/mocks/getPullRequestEdgeMock.ts index d5cef70b..7a35b3d1 100644 --- a/src/services/github/mocks/getPullRequestEdgeMock.ts +++ b/src/services/github/mocks/getPullRequestEdgeMock.ts @@ -12,6 +12,9 @@ export function getPullRequestEdgeMock({ }): PullRequestEdge { return { node: { + mergeCommit: { + oid: 'f3b618b9421fdecdb36862f907afbdd6344b361d', + }, repository: { name: 'kibana', owner: { diff --git a/src/services/logger.ts b/src/services/logger.ts index a4cd437c..2eb0402b 100644 --- a/src/services/logger.ts +++ b/src/services/logger.ts @@ -38,7 +38,17 @@ export function initLogger() { } function formatMessage(message: string | Record) { - return isString(message) ? message : JSON.stringify(message, null, 2); + if (message == null) { + return ''; + } + + if (isString(message)) { + return message; + } + + if (typeof message === 'object') { + return JSON.stringify(message, Object.getOwnPropertyNames(message), 2); + } } // log levels: diff --git a/src/test/integration/__snapshots__/integration.test.ts.snap b/src/test/integration/__snapshots__/integration.test.ts.snap index b75c42c3..ccdedb1d 100644 --- a/src/test/integration/__snapshots__/integration.test.ts.snap +++ b/src/test/integration/__snapshots__/integration.test.ts.snap @@ -49,6 +49,9 @@ Object { name } number + mergeCommit { + oid + } timelineItems( last: 20 itemTypes: CROSS_REFERENCED_EVENT @@ -178,6 +181,9 @@ Object { name } number + mergeCommit { + oid + } timelineItems( last: 20 itemTypes: CROSS_REFERENCED_EVENT diff --git a/src/ui/cherrypickAndCreatePullRequest.test.ts b/src/ui/cherrypickAndCreatePullRequest.test.ts index 31faa3d5..6b7f1a25 100644 --- a/src/ui/cherrypickAndCreatePullRequest.test.ts +++ b/src/ui/cherrypickAndCreatePullRequest.test.ts @@ -200,7 +200,7 @@ describe('cherrypickAndCreatePullRequest', () => { sourceBranch: 'myDefaultRepoBaseBranch', } as BackportOptions; - const res = await runTimers(() => + const res = await runTimersUntilResolved(() => cherrypickAndCreatePullRequest({ options, commits: [ @@ -291,7 +291,9 @@ function setupExecSpy() { if (conflictCheckCounts >= 4) { return {}; } - const e = new Error('cherrypick failed'); + const e = new Error('Not all conflicts resolved'); + // @ts-ignore + e.cmd = cmd; // @ts-ignore e.code = 2; // @ts-ignore @@ -330,13 +332,23 @@ function setupExecSpy() { }) as typeof childProcess.exec); } -async function runTimers(fn: () => Promise) { +/* + * Run timers (setInterval/setTimeout) every tick continuously until the promise has been resolved + */ +async function runTimersUntilResolved(fn: () => Promise) { jest.useFakeTimers(); + let isResolved = false; const p = fn(); - await new Promise((resolve) => setImmediate(resolve)); - jest.advanceTimersByTime(1000); - jest.runOnlyPendingTimers(); + p.finally(() => (isResolved = true)); + + while (isResolved === false) { + // tick + await new Promise((resolve) => setImmediate(resolve)); + + // run timers + jest.runAllTimers(); + } return p; } diff --git a/src/ui/cherrypickAndCreatePullRequest.ts b/src/ui/cherrypickAndCreatePullRequest.ts index 3e9e2a34..0be958f0 100644 --- a/src/ui/cherrypickAndCreatePullRequest.ts +++ b/src/ui/cherrypickAndCreatePullRequest.ts @@ -96,19 +96,24 @@ async function waitForCherrypick( options: BackportOptions, commit: CommitSelected ) { - const spinner = ora( + const cherrypickSpinner = ora( `Cherry-picking commit ${getShortSha(commit.sha)}` ).start(); try { await cherrypick(options, commit); - spinner.succeed(); + cherrypickSpinner.succeed(); } catch (e) { + cherrypickSpinner.fail(); + const filesWithConflicts = await getFilesWithConflicts(options); if (isEmpty(filesWithConflicts)) { throw e; } - spinner.fail(); + /* + * Conflict resolution phase starts here... + */ + if (options.editor) { const repoPath = getRepoPath(options); await exec(`${options.editor} ${repoPath}`); @@ -120,13 +125,23 @@ async function waitForCherrypick( // list unstaged files and require user to confirm adding+comitting them await waitForEnterAndListUnstaged(options); - // add unstaged files - await addUnstagedFiles(options); + /* + * Conflicts resolved and cherrypicking can be continued... + */ + + const stagingSpinner = ora(`Staging and committing files`).start(); + try { + // add unstaged files + await addUnstagedFiles(options); - // continue cherrypick (similar to commit) - await cherrypickContinue(options); + // continue cherrypick (similar to `git commit`) + await cherrypickContinue(options); + } catch (e) { + stagingSpinner.fail(); + throw e; + } - ora(`Staging and committing files`).start().succeed(); + stagingSpinner.succeed(); } } @@ -138,19 +153,18 @@ async function waitForConflictsToBeResolved(options: BackportOptions) { const checkForConflicts = async () => { const filesWithConflicts = await getFilesWithConflicts(options); if (isEmpty(filesWithConflicts)) { - clearInterval(intervalId); resolve(); spinner.succeed(spinnerText); } else { spinner.text = dedent(`${spinnerText} - Resolve the conflicts the following files and then return here. You do not need to \`git add\` or \`git commit\`: + Resolve the conflicts in the following files and then return here. You do not need to \`git add\` or \`git commit\`: ${filesWithConflicts.join('\n')}`); + setTimeout(checkForConflicts, 1000); } }; checkForConflicts(); - const intervalId = setInterval(checkForConflicts, 1000); }); }