From 8a3493af470feff2dc9be3531bdcf2c8dfbd7a31 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 15 May 2020 17:21:01 +0200 Subject: [PATCH] feat(dev-infra): integrate merge script into ng-dev cli (#37138) Integrates the merge script into the `ng-dev` CLI. The goal is that caretakers can run the same command across repositories to merge a pull request. The command is as followed: `yarn ng-dev pr merge `. PR Close #37138 --- dev-infra/pr/BUILD.bazel | 14 +--- dev-infra/pr/cli.ts | 41 +++-------- .../pr/discover-new-conflicts/BUILD.bazel | 19 +++++ dev-infra/pr/discover-new-conflicts/cli.ts | 33 +++++++++ .../index.ts} | 10 ++- dev-infra/pr/merge/BUILD.bazel | 17 +++++ dev-infra/pr/merge/cli.ts | 33 +++++++++ dev-infra/pr/merge/config.ts | 70 +++++++++++++++---- dev-infra/pr/merge/git.ts | 20 +++--- dev-infra/pr/merge/index.ts | 46 +++++++----- dev-infra/pr/merge/pull-request.ts | 4 +- dev-infra/pr/merge/strategies/api-merge.ts | 4 +- dev-infra/pr/merge/task.ts | 5 +- dev-infra/utils/BUILD.bazel | 1 + dev-infra/utils/config.ts | 17 +++-- dev-infra/{pr/merge => utils}/console.ts | 0 16 files changed, 237 insertions(+), 97 deletions(-) create mode 100644 dev-infra/pr/discover-new-conflicts/BUILD.bazel create mode 100644 dev-infra/pr/discover-new-conflicts/cli.ts rename dev-infra/pr/{discover-new-conflicts.ts => discover-new-conflicts/index.ts} (94%) create mode 100644 dev-infra/pr/merge/BUILD.bazel create mode 100644 dev-infra/pr/merge/cli.ts rename dev-infra/{pr/merge => utils}/console.ts (100%) diff --git a/dev-infra/pr/BUILD.bazel b/dev-infra/pr/BUILD.bazel index ad1765c96e..b700e03f74 100644 --- a/dev-infra/pr/BUILD.bazel +++ b/dev-infra/pr/BUILD.bazel @@ -2,20 +2,12 @@ load("@npm_bazel_typescript//:index.bzl", "ts_library") ts_library( name = "pr", - srcs = glob([ - "*.ts", - ]), + srcs = ["cli.ts"], module_name = "@angular/dev-infra-private/pr", visibility = ["//dev-infra:__subpackages__"], deps = [ - "//dev-infra/utils", - "@npm//@types/cli-progress", - "@npm//@types/node", - "@npm//@types/shelljs", + "//dev-infra/pr/discover-new-conflicts", + "//dev-infra/pr/merge", "@npm//@types/yargs", - "@npm//cli-progress", - "@npm//shelljs", - "@npm//typed-graphqlify", - "@npm//yargs", ], ) diff --git a/dev-infra/pr/cli.ts b/dev-infra/pr/cli.ts index a27d39eb02..d5f4c7253f 100644 --- a/dev-infra/pr/cli.ts +++ b/dev-infra/pr/cli.ts @@ -7,39 +7,20 @@ */ import * as yargs from 'yargs'; -import {discoverNewConflictsForPr} from './discover-new-conflicts'; -/** A Date object 30 days ago. */ -const THIRTY_DAYS_AGO = (() => { - const date = new Date(); - // Set the hours, minutes and seconds to 0 to only consider date. - date.setHours(0, 0, 0, 0); - // Set the date to 30 days in the past. - date.setDate(date.getDate() - 30); - return date; -})(); +import {buildDiscoverNewConflictsCommand, handleDiscoverNewConflictsCommand} from './discover-new-conflicts/cli'; +import {buildMergeCommand, handleMergeCommand} from './merge/cli'; -/** Build the parser for the pr commands. */ +/** Build the parser for pull request commands. */ export function buildPrParser(localYargs: yargs.Argv) { - return localYargs.help().strict().demandCommand().command( - 'discover-new-conflicts ', - 'Check if a pending PR causes new conflicts for other pending PRs', - args => { - return args.option('date', { - description: 'Only consider PRs updated since provided date', - defaultDescription: '30 days ago', - coerce: Date.parse, - default: THIRTY_DAYS_AGO, - }); - }, - ({pr, date}) => { - // If a provided date is not able to be parsed, yargs provides it as NaN. - if (isNaN(date)) { - console.error('Unable to parse the value provided via --date flag'); - process.exit(1); - } - discoverNewConflictsForPr(pr, date); - }); + return localYargs.help() + .strict() + .demandCommand() + .command('merge ', 'Merge pull requests', buildMergeCommand, handleMergeCommand) + .command( + 'discover-new-conflicts ', + 'Check if a pending PR causes new conflicts for other pending PRs', + buildDiscoverNewConflictsCommand, handleDiscoverNewConflictsCommand) } if (require.main === module) { diff --git a/dev-infra/pr/discover-new-conflicts/BUILD.bazel b/dev-infra/pr/discover-new-conflicts/BUILD.bazel new file mode 100644 index 0000000000..a0dbdca87a --- /dev/null +++ b/dev-infra/pr/discover-new-conflicts/BUILD.bazel @@ -0,0 +1,19 @@ +load("@npm_bazel_typescript//:index.bzl", "ts_library") + +ts_library( + name = "discover-new-conflicts", + srcs = [ + "cli.ts", + "index.ts", + ], + module_name = "@angular/dev-infra-private/pr/discover-new-conflicts", + visibility = ["//dev-infra:__subpackages__"], + deps = [ + "//dev-infra/utils", + "@npm//@types/cli-progress", + "@npm//@types/node", + "@npm//@types/shelljs", + "@npm//@types/yargs", + "@npm//typed-graphqlify", + ], +) diff --git a/dev-infra/pr/discover-new-conflicts/cli.ts b/dev-infra/pr/discover-new-conflicts/cli.ts new file mode 100644 index 0000000000..ec0b8ff9f6 --- /dev/null +++ b/dev-infra/pr/discover-new-conflicts/cli.ts @@ -0,0 +1,33 @@ +import {Arguments, Argv} from 'yargs'; + +import {discoverNewConflictsForPr} from './index'; + +/** Builds the discover-new-conflicts pull request command. */ +export function buildDiscoverNewConflictsCommand(yargs: Argv) { + return yargs.option('date', { + description: 'Only consider PRs updated since provided date', + defaultDescription: '30 days ago', + coerce: Date.parse, + default: getThirtyDaysAgoDate, + }); +} + +/** Handles the discover-new-conflicts pull request command. */ +export async function handleDiscoverNewConflictsCommand({prNumber, date}: Arguments) { + // If a provided date is not able to be parsed, yargs provides it as NaN. + if (isNaN(date)) { + console.error('Unable to parse the value provided via --date flag'); + process.exit(1); + } + await discoverNewConflictsForPr(prNumber, date); +} + +/** Gets a date object 30 days ago from today. */ +function getThirtyDaysAgoDate(): Date { + const date = new Date(); + // Set the hours, minutes and seconds to 0 to only consider date. + date.setHours(0, 0, 0, 0); + // Set the date to 30 days in the past. + date.setDate(date.getDate() - 30); + return date; +} diff --git a/dev-infra/pr/discover-new-conflicts.ts b/dev-infra/pr/discover-new-conflicts/index.ts similarity index 94% rename from dev-infra/pr/discover-new-conflicts.ts rename to dev-infra/pr/discover-new-conflicts/index.ts index 3f5d0f9474..c688b4aa90 100644 --- a/dev-infra/pr/discover-new-conflicts.ts +++ b/dev-infra/pr/discover-new-conflicts/index.ts @@ -9,10 +9,10 @@ import {Bar} from 'cli-progress'; import {types as graphQLTypes} from 'typed-graphqlify'; -import {getConfig, NgDevConfig} from '../utils/config'; -import {getCurrentBranch, hasLocalChanges} from '../utils/git'; -import {getPendingPrs} from '../utils/github'; -import {exec} from '../utils/shelljs'; +import {getConfig, NgDevConfig} from '../../utils/config'; +import {getCurrentBranch, hasLocalChanges} from '../../utils/git'; +import {getPendingPrs} from '../../utils/github'; +import {exec} from '../../utils/shelljs'; /* GraphQL schema for the response body for each pending PR. */ @@ -67,8 +67,6 @@ export async function discoverNewConflictsForPr( const progressBar = new Bar({format: `[{bar}] ETA: {eta}s | {value}/{total}`}); /* PRs which were found to be conflicting. */ const conflicts: Array = []; - /* String version of the updatedAfter value, for logging. */ - const updatedAfterString = new Date(updatedAfter).toLocaleDateString(); console.info(`Requesting pending PRs from Github`); /** List of PRs from github currently known as mergable. */ diff --git a/dev-infra/pr/merge/BUILD.bazel b/dev-infra/pr/merge/BUILD.bazel new file mode 100644 index 0000000000..68609c668a --- /dev/null +++ b/dev-infra/pr/merge/BUILD.bazel @@ -0,0 +1,17 @@ +load("@npm_bazel_typescript//:index.bzl", "ts_library") + +ts_library( + name = "merge", + srcs = glob(["**/*.ts"]), + module_name = "@angular/dev-infra-private/pr/merge", + visibility = ["//dev-infra:__subpackages__"], + deps = [ + "//dev-infra/commit-message", + "//dev-infra/utils", + "@npm//@octokit/rest", + "@npm//@types/inquirer", + "@npm//@types/node", + "@npm//@types/yargs", + "@npm//chalk", + ], +) diff --git a/dev-infra/pr/merge/cli.ts b/dev-infra/pr/merge/cli.ts new file mode 100644 index 0000000000..8bec6a2231 --- /dev/null +++ b/dev-infra/pr/merge/cli.ts @@ -0,0 +1,33 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import chalk from 'chalk'; +import {Arguments, Argv} from 'yargs'; +import {GITHUB_TOKEN_GENERATE_URL, mergePullRequest} from './index'; + +/** Builds the options for the merge command. */ +export function buildMergeCommand(yargs: Argv) { + return yargs.help().strict().option('github-token', { + type: 'string', + description: 'Github token. If not set, token is retrieved from the environment variables.' + }) +} + +/** Handles the merge command. i.e. performs the merge of a specified pull request. */ +export async function handleMergeCommand(args: Arguments) { + const githubToken = args.githubToken || process.env.GITHUB_TOKEN || process.env.TOKEN; + if (!githubToken) { + console.error( + chalk.red('No Github token set. Please set the `GITHUB_TOKEN` environment variable.')); + console.error(chalk.red('Alternatively, pass the `--github-token` command line flag.')); + console.error(chalk.yellow(`You can generate a token here: ${GITHUB_TOKEN_GENERATE_URL}`)); + process.exit(1); + } + + await mergePullRequest(args.prNumber, githubToken); +} diff --git a/dev-infra/pr/merge/config.ts b/dev-infra/pr/merge/config.ts index b500fd4fc6..5476c611d9 100644 --- a/dev-infra/pr/merge/config.ts +++ b/dev-infra/pr/merge/config.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {getAngularDevConfig} from '../../utils/config'; +import {getConfig, NgDevConfig} from '../../utils/config'; import {GithubApiMergeStrategyConfig} from './strategies/api-merge'; @@ -31,10 +31,30 @@ export interface TargetLabel { branches: string[]|((githubTargetBranch: string) => string[]); } +/** Describes the remote used for merging pull requests. */ +export interface MergeRemote { + /** Owner name of the repository. */ + owner: string; + /** Name of the repository. */ + name: string; + /** Whether SSH should be used for merging pull requests. */ + useSsh?: boolean +} + +/** + * Configuration for the merge script with all remote options specified. The + * default `MergeConfig` has does not require any of these options as defaults + * are provided by the common dev-infra github configuration. + */ +export type MergeConfigWithRemote = MergeConfig&{remote: MergeRemote}; + /** Configuration for the merge script. */ export interface MergeConfig { - /** Configuration for the upstream repository. */ - repository: {user: string; name: string; useSsh?: boolean}; + /** + * Configuration for the upstream remote. All of these options are optional as + * defaults are provided by the common dev-infra github configuration. + */ + remote?: Partial; /** List of target labels. */ labels: TargetLabel[]; /** Required base commits for given branches. */ @@ -54,34 +74,56 @@ export interface MergeConfig { githubApiMerge: false|GithubApiMergeStrategyConfig; } +/** + * Configuration of the merge script in the dev-infra configuration. Note that the + * merge configuration is retrieved lazily as usually these configurations rely + * on branch name computations. We don't want to run these immediately whenever + * the dev-infra configuration is loaded as that could slow-down other commands. + */ +export type DevInfraMergeConfig = NgDevConfig<{'merge': () => MergeConfig}>; + /** Loads and validates the merge configuration. */ -export function loadAndValidateConfig(): {config?: MergeConfig, errors?: string[]} { - const config = getAngularDevConfig<'merge', MergeConfig>().merge; - if (config === undefined) { +export function loadAndValidateConfig(): {config?: MergeConfigWithRemote, errors?: string[]} { + const config: Partial = getConfig(); + + if (config.merge === undefined) { return { errors: ['No merge configuration found. Set the `merge` configuration.'] } } - const errors = validateConfig(config); + + if (typeof config.merge !== 'function') { + return { + errors: ['Expected merge configuration to be defined lazily through a function.'] + } + } + + const mergeConfig = config.merge(); + const errors = validateMergeConfig(mergeConfig); + if (errors.length) { return {errors}; } - return {config}; + + if (mergeConfig.remote) { + mergeConfig.remote = {...config.github, ...mergeConfig.remote}; + } else { + mergeConfig.remote = config.github; + } + + // We always set the `remote` option, so we can safely cast the + // config to `MergeConfigWithRemote`. + return {config: mergeConfig as MergeConfigWithRemote}; } /** Validates the specified configuration. Returns a list of failure messages. */ -function validateConfig(config: MergeConfig): string[] { +function validateMergeConfig(config: Partial): string[] { const errors: string[] = []; if (!config.labels) { errors.push('No label configuration.'); } else if (!Array.isArray(config.labels)) { errors.push('Label configuration needs to be an array.'); } - if (!config.repository) { - errors.push('No repository is configured.'); - } else if (!config.repository.user || !config.repository.name) { - errors.push('Repository configuration needs to specify a `user` and repository `name`.'); - } if (!config.claSignedLabel) { errors.push('No CLA signed label configured.'); } diff --git a/dev-infra/pr/merge/git.ts b/dev-infra/pr/merge/git.ts index 34468001f6..e43b7e0aab 100644 --- a/dev-infra/pr/merge/git.ts +++ b/dev-infra/pr/merge/git.ts @@ -8,7 +8,7 @@ import * as Octokit from '@octokit/rest'; import {spawnSync, SpawnSyncOptions, SpawnSyncReturns} from 'child_process'; -import {MergeConfig} from './config'; +import {MergeConfigWithRemote} from './config'; /** Error for failed Github API requests. */ export class GithubApiRequestError extends Error { @@ -28,14 +28,15 @@ export class GitCommandError extends Error { } export class GitClient { - /** Short-hand for accessing the repository configuration. */ - repoConfig = this._config.repository; - /** Octokit request parameters object for targeting the configured repository. */ - repoParams = {owner: this.repoConfig.user, repo: this.repoConfig.name}; + /** Short-hand for accessing the remote configuration. */ + remoteConfig = this._config.remote; + /** Octokit request parameters object for targeting the configured remote. */ + remoteParams = {owner: this.remoteConfig.owner, repo: this.remoteConfig.name}; /** URL that resolves to the configured repository. */ - repoGitUrl = this.repoConfig.useSsh ? - `git@github.com:${this.repoConfig.user}/${this.repoConfig.name}.git` : - `https://${this._githubToken}@github.com/${this.repoConfig.user}/${this.repoConfig.name}.git`; + repoGitUrl = this.remoteConfig.useSsh ? + `git@github.com:${this.remoteConfig.owner}/${this.remoteConfig.name}.git` : + `https://${this._githubToken}@github.com/${this.remoteConfig.owner}/${ + this.remoteConfig.name}.git`; /** Instance of the authenticated Github octokit API. */ api: Octokit; @@ -43,7 +44,8 @@ export class GitClient { private _tokenRegex = new RegExp(this._githubToken, 'g'); constructor( - private _projectRoot: string, private _githubToken: string, private _config: MergeConfig) { + private _projectRoot: string, private _githubToken: string, + private _config: MergeConfigWithRemote) { this.api = new Octokit({auth: _githubToken}); this.api.hook.error('request', error => { // Wrap API errors in a known error class. This allows us to diff --git a/dev-infra/pr/merge/index.ts b/dev-infra/pr/merge/index.ts index 4bc1e2a842..3efb5e8cc1 100644 --- a/dev-infra/pr/merge/index.ts +++ b/dev-infra/pr/merge/index.ts @@ -6,31 +6,45 @@ * found in the LICENSE file at https://angular.io/license */ -import {MergeResult, MergeStatus, PullRequestMergeTask} from './task'; -import {GithubApiRequestError} from './git'; import chalk from 'chalk'; -import {promptConfirm} from './console'; -import {loadAndValidateConfig} from './config'; + import {getRepoBaseDir} from '../../utils/config'; +import {promptConfirm} from '../../utils/console'; + +import {loadAndValidateConfig, MergeConfigWithRemote} from './config'; +import {GithubApiRequestError} from './git'; +import {MergeResult, MergeStatus, PullRequestMergeTask} from './task'; /** URL to the Github page where personal access tokens can be generated. */ export const GITHUB_TOKEN_GENERATE_URL = `https://github.com/settings/tokens`; /** - * Entry-point for the merge script CLI. The script can be used to merge individual pull requests - * into branches based on the `PR target` labels that have been set in a configuration. The script - * aims to reduce the manual work that needs to be performed to cherry-pick a PR into multiple - * branches based on a target label. + * Merges a given pull request based on labels configured in the given merge configuration. + * Pull requests can be merged with different strategies such as the Github API merge + * strategy, or the local autosquash strategy. Either strategy has benefits and downsides. + * More information on these strategies can be found in their dedicated strategy classes. + * + * See {@link GithubApiMergeStrategy} and {@link AutosquashMergeStrategy} + * + * @param prNumber Number of the pull request that should be merged. + * @param githubToken Github token used for merging (i.e. fetching and pushing) + * @param projectRoot Path to the local Git project that is used for merging. + * @param config Configuration for merging pull requests. */ -export async function mergePullRequest(prNumber: number, githubToken: string) { - const projectRoot = getRepoBaseDir(); - const {config, errors} = loadAndValidateConfig(); - - if (errors) { - console.error(chalk.red('Invalid configuration:')); - errors.forEach(desc => console.error(chalk.yellow(` - ${desc}`))); - process.exit(1); +export async function mergePullRequest( + prNumber: number, githubToken: string, projectRoot: string = getRepoBaseDir(), + config?: MergeConfigWithRemote) { + // If no explicit configuration has been specified, we load and validate + // the configuration from the shared dev-infra configuration. + if (config === undefined) { + const {config: _config, errors} = loadAndValidateConfig(); + if (errors) { + console.error(chalk.red('Invalid configuration:')); + errors.forEach(desc => console.error(chalk.yellow(` - ${desc}`))); + process.exit(1); + } + config = _config!; } const api = new PullRequestMergeTask(projectRoot, config, githubToken); diff --git a/dev-infra/pr/merge/pull-request.ts b/dev-infra/pr/merge/pull-request.ts index de9cfe1355..bf866dfe2e 100644 --- a/dev-infra/pr/merge/pull-request.ts +++ b/dev-infra/pr/merge/pull-request.ts @@ -62,7 +62,7 @@ export async function loadAndValidatePullRequest( } const {data: {state}} = - await git.api.repos.getCombinedStatusForRef({...git.repoParams, ref: prData.head.sha}); + await git.api.repos.getCombinedStatusForRef({...git.remoteParams, ref: prData.head.sha}); if (state === 'failure' && !ignoreNonFatalFailures) { return PullRequestFailure.failingCiJobs(); @@ -93,7 +93,7 @@ export async function loadAndValidatePullRequest( async function fetchPullRequestFromGithub( git: GitClient, prNumber: number): Promise { try { - const result = await git.api.pulls.get({...git.repoParams, pull_number: prNumber}); + const result = await git.api.pulls.get({...git.remoteParams, pull_number: prNumber}); return result.data; } catch (e) { // If the pull request could not be found, we want to return `null` so diff --git a/dev-infra/pr/merge/strategies/api-merge.ts b/dev-infra/pr/merge/strategies/api-merge.ts index 9b7766e0a2..2da887e9d2 100644 --- a/dev-infra/pr/merge/strategies/api-merge.ts +++ b/dev-infra/pr/merge/strategies/api-merge.ts @@ -77,7 +77,7 @@ export class GithubApiMergeStrategy extends MergeStrategy { const mergeOptions: PullsMergeParams = { pull_number: prNumber, merge_method: method, - ...this.git.repoParams, + ...this.git.remoteParams, }; if (needsCommitMessageFixup) { @@ -190,7 +190,7 @@ export class GithubApiMergeStrategy extends MergeStrategy { /** Gets all commit messages of commits in the pull request. */ private async _getPullRequestCommitMessages({prNumber}: PullRequest) { const request = this.git.api.pulls.listCommits.endpoint.merge( - {...this.git.repoParams, pull_number: prNumber}); + {...this.git.remoteParams, pull_number: prNumber}); const allCommits: PullsListCommitsResponse = await this.git.api.paginate(request); return allCommits.map(({commit}) => commit.message); } diff --git a/dev-infra/pr/merge/task.ts b/dev-infra/pr/merge/task.ts index 37f841bd37..e85065322d 100644 --- a/dev-infra/pr/merge/task.ts +++ b/dev-infra/pr/merge/task.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {MergeConfig} from './config'; +import {MergeConfigWithRemote} from './config'; import {PullRequestFailure} from './failures'; import {GitClient, GitCommandError} from './git'; import {isPullRequest, loadAndValidatePullRequest,} from './pull-request'; @@ -39,7 +39,8 @@ export class PullRequestMergeTask { git = new GitClient(this.projectRoot, this._githubToken, this.config); constructor( - public projectRoot: string, public config: MergeConfig, private _githubToken: string) {} + public projectRoot: string, public config: MergeConfigWithRemote, + private _githubToken: string) {} /** * Merges the given pull request and pushes it upstream. diff --git a/dev-infra/utils/BUILD.bazel b/dev-infra/utils/BUILD.bazel index fe3c27e477..48b015feeb 100644 --- a/dev-infra/utils/BUILD.bazel +++ b/dev-infra/utils/BUILD.bazel @@ -7,6 +7,7 @@ ts_library( visibility = ["//dev-infra:__subpackages__"], deps = [ "@npm//@octokit/graphql", + "@npm//@types/inquirer", "@npm//@types/node", "@npm//@types/shelljs", "@npm//shelljs", diff --git a/dev-infra/utils/config.ts b/dev-infra/utils/config.ts index a20279ae69..260c5a7930 100644 --- a/dev-infra/utils/config.ts +++ b/dev-infra/utils/config.ts @@ -9,13 +9,20 @@ import {join} from 'path'; import {exec} from 'shelljs'; +/** + * Describes the Github configuration for dev-infra. This configuration is + * used for API requests, determining the upstream remote, etc. + */ +export interface GithubConfig { + /** Owner name of the repository. */ + owner: string; + /** Name of the repository. */ + name: string; +} + /** The common configuration for ng-dev. */ type CommonConfig = { - /* Github repository configuration used for API Requests, determining upstream remote, etc. */ - github: { - owner: string, - name: string, - } + github: GithubConfig }; /** diff --git a/dev-infra/pr/merge/console.ts b/dev-infra/utils/console.ts similarity index 100% rename from dev-infra/pr/merge/console.ts rename to dev-infra/utils/console.ts