From 85b6c94cc6d4dd1cfae68fc14575d0efa57574b3 Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Wed, 27 May 2020 15:39:31 -0700 Subject: [PATCH] refactor(dev-infra): move GitClient to common util (#37318) Moves GitClient from merge script into common utils for unified method of performing git actions throughout the ng-dev toolset. PR Close #37318 --- dev-infra/pr/discover-new-conflicts/index.ts | 7 +- dev-infra/pr/merge/config.ts | 16 +- dev-infra/pr/merge/git.ts | 177 ------------------ dev-infra/pr/merge/index.ts | 2 +- dev-infra/pr/merge/pull-request.ts | 3 +- dev-infra/pr/merge/strategies/api-merge.ts | 2 +- dev-infra/pr/merge/strategies/strategy.ts | 2 +- dev-infra/pr/merge/task.ts | 5 +- dev-infra/pr/rebase/index.ts | 7 +- dev-infra/utils/BUILD.bazel | 1 + dev-infra/utils/config.ts | 15 +- dev-infra/utils/git.ts | 186 ++++++++++++++++++- 12 files changed, 209 insertions(+), 214 deletions(-) delete mode 100644 dev-infra/pr/merge/git.ts diff --git a/dev-infra/pr/discover-new-conflicts/index.ts b/dev-infra/pr/discover-new-conflicts/index.ts index 3e98e5f167..e4dfa6fef3 100644 --- a/dev-infra/pr/discover-new-conflicts/index.ts +++ b/dev-infra/pr/discover-new-conflicts/index.ts @@ -11,7 +11,7 @@ import {types as graphQLTypes} from 'typed-graphqlify'; import {getConfig, NgDevConfig} from '../../utils/config'; import {error, info} from '../../utils/console'; -import {getCurrentBranch, hasLocalChanges} from '../../utils/git'; +import {GitClient} from '../../utils/git'; import {getPendingPrs} from '../../utils/github'; import {exec} from '../../utils/shelljs'; @@ -55,15 +55,16 @@ const tempWorkingBranch = '__NgDevRepoBaseAfterChange__'; /** Checks if the provided PR will cause new conflicts in other pending PRs. */ export async function discoverNewConflictsForPr( newPrNumber: number, updatedAfter: number, config: Pick = getConfig()) { + const git = new GitClient(); // If there are any local changes in the current repository state, the // check cannot run as it needs to move between branches. - if (hasLocalChanges()) { + if (git.hasLocalChanges()) { error('Cannot run with local changes. Please make sure there are no local changes.'); process.exit(1); } /** The active github branch when the run began. */ - const originalBranch = getCurrentBranch(); + const originalBranch = git.getCurrentBranch(); /* Progress bar to indicate progress. */ const progressBar = new Bar({format: `[{bar}] ETA: {eta}s | {value}/{total}`}); /* PRs which were found to be conflicting. */ diff --git a/dev-infra/pr/merge/config.ts b/dev-infra/pr/merge/config.ts index 08abfa29cc..861b46bbc1 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 {getConfig, NgDevConfig} from '../../utils/config'; +import {getConfig, GitClientConfig, NgDevConfig} from '../../utils/config'; import {GithubApiMergeStrategyConfig} from './strategies/api-merge'; @@ -31,22 +31,12 @@ 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}; +export type MergeConfigWithRemote = MergeConfig&{remote: GitClientConfig}; /** Configuration for the merge script. */ export interface MergeConfig { @@ -54,7 +44,7 @@ export interface MergeConfig { * Configuration for the upstream remote. All of these options are optional as * defaults are provided by the common dev-infra github configuration. */ - remote?: Partial; + remote?: GitClientConfig; /** List of target labels. */ labels: TargetLabel[]; /** Required base commits for given branches. */ diff --git a/dev-infra/pr/merge/git.ts b/dev-infra/pr/merge/git.ts deleted file mode 100644 index b89cccb953..0000000000 --- a/dev-infra/pr/merge/git.ts +++ /dev/null @@ -1,177 +0,0 @@ -/** - * @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 * as Octokit from '@octokit/rest'; -import {spawnSync, SpawnSyncOptions, SpawnSyncReturns} from 'child_process'; - -import {info, yellow} from '../../utils/console'; - -import {MergeConfigWithRemote} from './config'; - -/** Github response type extended to include the `x-oauth-scopes` headers presence. */ -type RateLimitResponseWithOAuthScopeHeader = Octokit.Response&{ - headers: {'x-oauth-scopes': string}; -}; - -/** Error for failed Github API requests. */ -export class GithubApiRequestError extends Error { - constructor(public status: number, message: string) { - super(message); - } -} - -/** Error for failed Git commands. */ -export class GitCommandError extends Error { - constructor(client: GitClient, public args: string[]) { - // Errors are not guaranteed to be caught. To ensure that we don't - // accidentally leak the Github token that might be used in a command, - // we sanitize the command that will be part of the error message. - super(`Command failed: git ${client.omitGithubTokenFromMessage(args.join(' '))}`); - } -} - -export class GitClient { - /** 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.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; - - /** The OAuth scopes available for the provided Github token. */ - private _oauthScopes: Promise|null = null; - /** Regular expression that matches the provided Github token. */ - private _tokenRegex = new RegExp(this._githubToken, 'g'); - - constructor( - 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 - // expect Github API errors better and in a non-ambiguous way. - throw new GithubApiRequestError(error.status, error.message); - }); - } - - /** Executes the given git command. Throws if the command fails. */ - run(args: string[], options?: SpawnSyncOptions): Omit, 'status'> { - const result = this.runGraceful(args, options); - if (result.status !== 0) { - throw new GitCommandError(this, args); - } - // Omit `status` from the type so that it's obvious that the status is never - // non-zero as explained in the method description. - return result as Omit, 'status'>; - } - - /** - * Spawns a given Git command process. Does not throw if the command fails. Additionally, - * if there is any stderr output, the output will be printed. This makes it easier to - * debug failed commands. - */ - runGraceful(args: string[], options: SpawnSyncOptions = {}): SpawnSyncReturns { - // To improve the debugging experience in case something fails, we print all executed - // Git commands. Note that we do not want to print the token if is contained in the - // command. It's common to share errors with others if the tool failed. - info('Executing: git', this.omitGithubTokenFromMessage(args.join(' '))); - - const result = spawnSync('git', args, { - cwd: this._projectRoot, - stdio: 'pipe', - ...options, - // Encoding is always `utf8` and not overridable. This ensures that this method - // always returns `string` as output instead of buffers. - encoding: 'utf8', - }); - - if (result.stderr !== null) { - // Git sometimes prints the command if it failed. This means that it could - // potentially leak the Github token used for accessing the remote. To avoid - // printing a token, we sanitize the string before printing the stderr output. - process.stderr.write(this.omitGithubTokenFromMessage(result.stderr)); - } - - return result; - } - - /** Whether the given branch contains the specified SHA. */ - hasCommit(branchName: string, sha: string): boolean { - return this.run(['branch', branchName, '--contains', sha]).stdout !== ''; - } - - /** Gets the currently checked out branch. */ - getCurrentBranch(): string { - return this.run(['rev-parse', '--abbrev-ref', 'HEAD']).stdout.trim(); - } - - /** Gets whether the current Git repository has uncommitted changes. */ - hasUncommittedChanges(): boolean { - return this.runGraceful(['diff-index', '--quiet', 'HEAD']).status !== 0; - } - - /** Sanitizes a given message by omitting the provided Github token if present. */ - omitGithubTokenFromMessage(value: string): string { - return value.replace(this._tokenRegex, ''); - } - - /** - * Assert the GitClient instance is using a token with permissions for the all of the - * provided OAuth scopes. - */ - async hasOauthScopes(...requestedScopes: string[]): Promise { - const missingScopes: string[] = []; - const scopes = await this.getAuthScopesForToken(); - requestedScopes.forEach(scope => { - if (!scopes.includes(scope)) { - missingScopes.push(scope); - } - }); - // If no missing scopes are found, return true to indicate all OAuth Scopes are available. - if (missingScopes.length === 0) { - return true; - } - - /** - * Preconstructed error message to log to the user, providing missing scopes and - * remediation instructions. - **/ - const error = - `The provided does not have required permissions due to missing scope(s): ` + - `${yellow(missingScopes.join(', '))}\n\n` + - `Update the token in use at:\n` + - ` https://github.com/settings/tokens\n\n` + - `Alternatively, a new token can be created at: https://github.com/settings/tokens/new\n`; - - return {error}; - } - - - /** - * Retrieves the OAuth scopes for the loaded Github token, returning the already - * retrieved list of OAuth scopes if available. - **/ - private async getAuthScopesForToken() { - // If the OAuth scopes have already been loaded, return the Promise containing them. - if (this._oauthScopes !== null) { - return this._oauthScopes; - } - // OAuth scopes are loaded via the /rate_limit endpoint to prevent - // usage of a request against that rate_limit for this lookup. - return this._oauthScopes = this.api.rateLimit.get().then(_response => { - const response = _response as RateLimitResponseWithOAuthScopeHeader; - const scopes: string = response.headers['x-oauth-scopes'] || ''; - return scopes.split(',').map(scope => scope.trim()); - }); - } -} diff --git a/dev-infra/pr/merge/index.ts b/dev-infra/pr/merge/index.ts index c01abd0865..fcd3c16fdd 100644 --- a/dev-infra/pr/merge/index.ts +++ b/dev-infra/pr/merge/index.ts @@ -9,9 +9,9 @@ import {getRepoBaseDir} from '../../utils/config'; import {error, green, info, promptConfirm, red, yellow} from '../../utils/console'; +import {GithubApiRequestError} from '../../utils/git'; 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. */ diff --git a/dev-infra/pr/merge/pull-request.ts b/dev-infra/pr/merge/pull-request.ts index bf866dfe2e..f0bb691841 100644 --- a/dev-infra/pr/merge/pull-request.ts +++ b/dev-infra/pr/merge/pull-request.ts @@ -8,8 +8,9 @@ import * as Octokit from '@octokit/rest'; +import {GitClient} from '../../utils/git'; + import {PullRequestFailure} from './failures'; -import {GitClient} from './git'; import {matchesPattern} from './string-pattern'; import {getBranchesFromTargetLabel, getTargetLabelFromPullRequest} from './target-label'; import {PullRequestMergeTask} from './task'; diff --git a/dev-infra/pr/merge/strategies/api-merge.ts b/dev-infra/pr/merge/strategies/api-merge.ts index ca8106a6d5..77d0a7970a 100644 --- a/dev-infra/pr/merge/strategies/api-merge.ts +++ b/dev-infra/pr/merge/strategies/api-merge.ts @@ -10,9 +10,9 @@ import {PullsListCommitsResponse, PullsMergeParams} from '@octokit/rest'; import {prompt} from 'inquirer'; import {parseCommitMessage} from '../../../commit-message/validate'; +import {GitClient} from '../../../utils/git'; import {GithubApiMergeMethod} from '../config'; import {PullRequestFailure} from '../failures'; -import {GitClient} from '../git'; import {PullRequest} from '../pull-request'; import {matchesPattern} from '../string-pattern'; diff --git a/dev-infra/pr/merge/strategies/strategy.ts b/dev-infra/pr/merge/strategies/strategy.ts index 63209ef9bc..9c29c1b8d9 100644 --- a/dev-infra/pr/merge/strategies/strategy.ts +++ b/dev-infra/pr/merge/strategies/strategy.ts @@ -6,8 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import {GitClient} from '../../../utils/git'; import {PullRequestFailure} from '../failures'; -import {GitClient} from '../git'; import {PullRequest} from '../pull-request'; /** diff --git a/dev-infra/pr/merge/task.ts b/dev-infra/pr/merge/task.ts index cadfe60b49..8ea89e67d6 100644 --- a/dev-infra/pr/merge/task.ts +++ b/dev-infra/pr/merge/task.ts @@ -6,9 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ +import {GitClient, GitCommandError} from '../../utils/git'; + import {MergeConfigWithRemote} from './config'; import {PullRequestFailure} from './failures'; -import {GitClient, GitCommandError} from './git'; import {isPullRequest, loadAndValidatePullRequest,} from './pull-request'; import {GithubApiMergeStrategy} from './strategies/api-merge'; import {AutosquashMergeStrategy} from './strategies/autosquash-merge'; @@ -40,7 +41,7 @@ export interface MergeResult { */ export class PullRequestMergeTask { /** Git client that can be used to execute Git commands. */ - git = new GitClient(this.projectRoot, this._githubToken, this.config); + git = new GitClient(this._githubToken, {github: this.config.remote}); constructor( public projectRoot: string, public config: MergeConfigWithRemote, diff --git a/dev-infra/pr/rebase/index.ts b/dev-infra/pr/rebase/index.ts index f9b76e8479..7524b9a743 100644 --- a/dev-infra/pr/rebase/index.ts +++ b/dev-infra/pr/rebase/index.ts @@ -11,7 +11,7 @@ import {URL} from 'url'; import {getConfig, NgDevConfig} from '../../utils/config'; import {error, info, promptConfirm} from '../../utils/console'; -import {getCurrentBranch, hasLocalChanges} from '../../utils/git'; +import {GitClient} from '../../utils/git'; import {getPr} from '../../utils/github'; import {exec} from '../../utils/shelljs'; @@ -42,8 +42,9 @@ const PR_SCHEMA = { */ export async function rebasePr( prNumber: number, githubToken: string, config: Pick = getConfig()) { + const git = new GitClient(); // TODO: Rely on a common assertNoLocalChanges function. - if (hasLocalChanges()) { + if (git.hasLocalChanges()) { error('Cannot perform rebase of PR with local changes.'); process.exit(1); } @@ -52,7 +53,7 @@ export async function rebasePr( * The branch originally checked out before this method performs any Git * operations that may change the working branch. */ - const originalBranch = getCurrentBranch(); + const originalBranch = git.getCurrentBranch(); /* Get the PR information from Github. */ const pr = await getPr(PR_SCHEMA, prNumber, config.github); diff --git a/dev-infra/utils/BUILD.bazel b/dev-infra/utils/BUILD.bazel index 062f2080af..87d242ae72 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//@octokit/rest", "@npm//@types/inquirer", "@npm//@types/node", "@npm//@types/shelljs", diff --git a/dev-infra/utils/config.ts b/dev-infra/utils/config.ts index 8642fcdfc0..bcf6a82d60 100644 --- a/dev-infra/utils/config.ts +++ b/dev-infra/utils/config.ts @@ -13,17 +13,22 @@ import {error} from './console'; import {exec} from './shelljs'; import {isTsNodeAvailable} from './ts-node'; -/** - * Describes the Github configuration for dev-infra. This configuration is - * used for API requests, determining the upstream remote, etc. - */ -export interface GithubConfig { +/** Configuration for Git client interactions. */ +export interface GitClientConfig { /** Owner name of the repository. */ owner: string; /** Name of the repository. */ name: string; + /** If SSH protocol should be used for git interactions. */ + useSsh?: boolean; } +/** + * Describes the Github configuration for dev-infra. This configuration is + * used for API requests, determining the upstream remote, etc. + */ +export interface GithubConfig extends GitClientConfig {} + /** The common configuration for ng-dev. */ type CommonConfig = { github: GithubConfig diff --git a/dev-infra/utils/git.ts b/dev-infra/utils/git.ts index cb5be5d560..147f6a625a 100644 --- a/dev-infra/utils/git.ts +++ b/dev-infra/utils/git.ts @@ -6,15 +6,187 @@ * found in the LICENSE file at https://angular.io/license */ -import {exec} from './shelljs'; +import * as Octokit from '@octokit/rest'; +import {spawnSync, SpawnSyncOptions, SpawnSyncReturns} from 'child_process'; + +import {getConfig, getRepoBaseDir, NgDevConfig} from './config'; +import {info, yellow} from './console'; -/** Whether the repo has any local changes. */ -export function hasLocalChanges() { - return !!exec(`git status --porcelain`).trim(); +/** Github response type extended to include the `x-oauth-scopes` headers presence. */ +type RateLimitResponseWithOAuthScopeHeader = Octokit.Response&{ + headers: {'x-oauth-scopes': string}; +}; + +/** Error for failed Github API requests. */ +export class GithubApiRequestError extends Error { + constructor(public status: number, message: string) { + super(message); + } } -/** Get the currently checked out branch. */ -export function getCurrentBranch() { - return exec(`git symbolic-ref --short HEAD`).trim(); +/** Error for failed Git commands. */ +export class GitCommandError extends Error { + constructor(client: GitClient, public args: string[]) { + // Errors are not guaranteed to be caught. To ensure that we don't + // accidentally leak the Github token that might be used in a command, + // we sanitize the command that will be part of the error message. + super(`Command failed: git ${client.omitGithubTokenFromMessage(args.join(' '))}`); + } +} + +/** + * Common client for performing Git interactions. + * + * Takes in two optional arguements: + * _githubToken: the token used for authentifation in github interactions, by default empty + * allowing readonly actions. + * _config: The dev-infra configuration containing GitClientConfig information, by default + * loads the config from the default location. + **/ +export class GitClient { + /** Short-hand for accessing the remote configuration. */ + remoteConfig = this._config.github; + /** 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.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; + + /** The file path of project's root directory. */ + private _projectRoot = getRepoBaseDir(); + /** The OAuth scopes available for the provided Github token. */ + private _oauthScopes = Promise.resolve([]); + /** Regular expression that matches the provided Github token. */ + private _tokenRegex = new RegExp(this._githubToken, 'g'); + + constructor( + private _githubToken = '', private _config: Pick = getConfig()) { + this.api = new Octokit({auth: _githubToken}); + this.api.hook.error('request', error => { + // Wrap API errors in a known error class. This allows us to + // expect Github API errors better and in a non-ambiguous way. + throw new GithubApiRequestError(error.status, error.message); + }); + } + + /** Executes the given git command. Throws if the command fails. */ + run(args: string[], options?: SpawnSyncOptions): Omit, 'status'> { + const result = this.runGraceful(args, options); + if (result.status !== 0) { + throw new GitCommandError(this, args); + } + // Omit `status` from the type so that it's obvious that the status is never + // non-zero as explained in the method description. + return result as Omit, 'status'>; + } + + /** + * Spawns a given Git command process. Does not throw if the command fails. Additionally, + * if there is any stderr output, the output will be printed. This makes it easier to + * debug failed commands. + */ + runGraceful(args: string[], options: SpawnSyncOptions = {}): SpawnSyncReturns { + // To improve the debugging experience in case something fails, we print all executed + // Git commands. Note that we do not want to print the token if is contained in the + // command. It's common to share errors with others if the tool failed. + info('Executing: git', this.omitGithubTokenFromMessage(args.join(' '))); + + const result = spawnSync('git', args, { + cwd: this._projectRoot, + stdio: 'pipe', + ...options, + // Encoding is always `utf8` and not overridable. This ensures that this method + // always returns `string` as output instead of buffers. + encoding: 'utf8', + }); + + if (result.stderr !== null) { + // Git sometimes prints the command if it failed. This means that it could + // potentially leak the Github token used for accessing the remote. To avoid + // printing a token, we sanitize the string before printing the stderr output. + process.stderr.write(this.omitGithubTokenFromMessage(result.stderr)); + } + + return result; + } + + /** Whether the given branch contains the specified SHA. */ + hasCommit(branchName: string, sha: string): boolean { + return this.run(['branch', branchName, '--contains', sha]).stdout !== ''; + } + + /** Gets the currently checked out branch. */ + getCurrentBranch(): string { + return this.run(['rev-parse', '--abbrev-ref', 'HEAD']).stdout.trim(); + } + + /** Gets whether the current Git repository has uncommitted changes. */ + hasUncommittedChanges(): boolean { + return this.runGraceful(['diff-index', '--quiet', 'HEAD']).status !== 0; + } + + /** Whether the repo has any local changes. */ + hasLocalChanges(): boolean { + return !!this.runGraceful(['git', 'status', '--porcelain']).stdout.trim(); + } + + /** Sanitizes a given message by omitting the provided Github token if present. */ + omitGithubTokenFromMessage(value: string): string { + return value.replace(this._tokenRegex, ''); + } + + /** + * Assert the GitClient instance is using a token with permissions for the all of the + * provided OAuth scopes. + */ + async hasOauthScopes(...requestedScopes: string[]): Promise { + const missingScopes: string[] = []; + const scopes = await this.getAuthScopesForToken(); + requestedScopes.forEach(scope => { + if (!scopes.includes(scope)) { + missingScopes.push(scope); + } + }); + // If no missing scopes are found, return true to indicate all OAuth Scopes are available. + if (missingScopes.length === 0) { + return true; + } + + /** + * Preconstructed error message to log to the user, providing missing scopes and + * remediation instructions. + **/ + const error = + `The provided does not have required permissions due to missing scope(s): ` + + `${yellow(missingScopes.join(', '))}\n\n` + + `Update the token in use at:\n` + + ` https://github.com/settings/tokens\n\n` + + `Alternatively, a new token can be created at: https://github.com/settings/tokens/new\n`; + + return {error}; + } + + + /** + * Retrieves the OAuth scopes for the loaded Github token, returning the already + * retrieved list of OAuth scopes if available. + **/ + private async getAuthScopesForToken() { + // If the OAuth scopes have already been loaded, return the Promise containing them. + if (this._oauthScopes !== null) { + return this._oauthScopes; + } + // OAuth scopes are loaded via the /rate_limit endpoint to prevent + // usage of a request against that rate_limit for this lookup. + return this._oauthScopes = this.api.rateLimit.get().then(_response => { + const response = _response as RateLimitResponseWithOAuthScopeHeader; + const scopes: string = response.headers['x-oauth-scopes'] || ''; + return scopes.split(',').map(scope => scope.trim()); + }); + } }