Skip to content

Commit 271a6d3

Browse files
josephperrottjosephperrott
committed
refactor(ng-dev): separate out logic for takeover, target and regular checkouts (#2560)
Separate the logic apart to be easier to parse as well as allow for intentional regular checkout. PR Close #2560
1 parent 8e95b77 commit 271a6d3

File tree

5 files changed

+202
-153
lines changed

5 files changed

+202
-153
lines changed

‎ng-dev/pr/checkout/checkout.ts‎

Lines changed: 41 additions & 146 deletions
Original file line numberDiff line numberDiff line change
@@ -1,184 +1,79 @@
1-
import {GithubConfig, NgDevConfig} from '../../utils/config.js';
2-
import {dirname, join} from 'path';
1+
/**
2+
* @license
3+
* Copyright Google LLC
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
39
import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client.js';
4-
import {Log, bold, green} from '../../utils/logging.js';
10+
import {green, Log, red} from '../../utils/logging.js';
511
import {checkOutPullRequestLocally} from '../common/checkout-pr.js';
6-
import {fileURLToPath} from 'url';
7-
import {ActiveReleaseTrains} from '../../release/versioning/active-release-trains.js';
8-
import {getNextBranchName} from '../../release/versioning/version-branches.js';
9-
import {addTokenToGitHttpsUrl} from '../../utils/git/github-urls.js';
1012
import {Prompt} from '../../utils/prompt.js';
11-
12-
/** List of accounts that are supported for takeover. */
13-
const takeoverAccounts = ['angular-robot'];
13+
import {checkoutToTargetBranch} from './target.js';
14+
import {checkoutAsPrTakeover} from './takeover.js';
1415

1516
export interface CheckoutPullRequestParams {
1617
pr: number;
1718
takeover?: boolean;
1819
target?: string;
1920
}
2021

21-
export async function checkoutPullRequest(
22-
params: CheckoutPullRequestParams,
23-
config: NgDevConfig<{github: GithubConfig}>,
24-
): Promise<void> {
22+
export async function checkoutPullRequest(params: CheckoutPullRequestParams): Promise<void> {
2523
const {pr, takeover, target} = params;
2624
/** An authenticated git client. */
2725
const git = await AuthenticatedGitClient.get();
2826

2927
if (takeover && target) {
30-
Log.error(` ✘ You cannot specify both takeover and target branch at the same time`);
28+
Log.error(` ${red('✘')} The --takeover and --target flags cannot be provided simultaneously`);
3129
return;
3230
}
3331

3432
// Make sure the local repository is clean.
3533
if (git.hasUncommittedChanges()) {
3634
Log.error(
37-
` ✘ Local working repository not clean. Please make sure there are no uncommitted changes`,
35+
` ${red('✘')} Local working repository not clean. Please make sure there are no uncommitted changes`,
3836
);
3937
return;
4038
}
4139

42-
const {resetGitState, pullRequest, pushToUpstreamCommand} = await checkOutPullRequestLocally(pr, {
40+
const localCheckoutResult = await checkOutPullRequestLocally(pr, {
4341
allowIfMaintainerCannotModify: true,
4442
});
4543

46-
if (!target) {
47-
const branchName = `pr-takeover-${pr}`;
48-
// if maintainer can modify is false or if takeover is provided do takeover
49-
50-
if (pullRequest.maintainerCanModify === false || takeover) {
51-
if (takeover !== true) {
52-
Log.info('The author of this pull request does not allow maintainers to modify the pull');
53-
Log.info(
54-
'request. Since you will not be able to push changes to the original pull request',
55-
);
56-
Log.info('you will instead need to perform a "takeover." In a takeover the original pull');
57-
Log.info('request will be checked out, the commits are modified to close the original on');
58-
Log.info('merge of the newly created branch.\n');
59-
60-
if (
61-
!(await Prompt.confirm({
62-
message: `Would you like to create a takeover pull request?`,
63-
default: true,
64-
}))
65-
) {
66-
Log.info('Aborting takeover..');
67-
await resetGitState();
68-
return;
69-
}
70-
}
71-
72-
if (git.runGraceful(['rev-parse', '-q', '--verify', branchName]).status === 0) {
73-
Log.error(` ✘ Expected branch name \`${branchName}\` already exists locally`);
74-
return;
75-
}
76-
77-
// Confirm that the takeover request is being done on a valid pull request.
78-
if (!takeoverAccounts.includes(pullRequest.author.login)) {
79-
Log.warn(
80-
` ⚠ ${bold(pullRequest.author.login)} is not an account fully supported for takeover.`,
81-
);
82-
Log.warn(` Supported accounts: ${bold(takeoverAccounts.join(', '))}`);
83-
if (
84-
await Prompt.confirm({
85-
message: `Continue with pull request takeover anyway?`,
86-
default: true,
87-
})
88-
) {
89-
Log.debug('Continuing per user confirmation in prompt');
90-
} else {
91-
Log.info('Aborting takeover..');
92-
await resetGitState();
93-
return;
94-
}
95-
}
96-
97-
Log.info(`Setting local branch name based on the pull request`);
98-
git.run(['checkout', '-q', '-b', branchName]);
99-
100-
Log.info('Updating commit messages to close previous pull request');
101-
git.run([
102-
'filter-branch',
103-
'-f',
104-
'--msg-filter',
105-
`${getCommitMessageFilterScriptPath()} ${pr}`,
106-
`${pullRequest.baseRefOid}..HEAD`,
107-
]);
44+
if (takeover) {
45+
return await checkoutAsPrTakeover(pr, localCheckoutResult);
46+
}
10847

109-
Log.info(` ${green('✔')} Checked out pull request #${pr} into branch: ${branchName}`);
110-
return;
111-
}
48+
if (target) {
49+
return await checkoutToTargetBranch(pr, target, localCheckoutResult);
50+
}
11251

113-
Log.info(`Checked out the remote branch for pull request #${pr}\n`);
114-
Log.info('To push the checked out branch back to its PR, run the following command:');
115-
Log.info(` $ ${pushToUpstreamCommand}`);
116-
} else {
117-
const branchName = `pr-${target.toLowerCase().replaceAll(/[\W_]/gm, '-')}-${pr}`;
118-
const {owner, name: repo} = config.github;
119-
const activeReleaseTrains = await ActiveReleaseTrains.fetch({
120-
name: repo,
121-
owner: owner,
122-
nextBranchName: getNextBranchName(config.github),
123-
api: git.github,
124-
});
52+
/**
53+
* Whether the pull request is configured to allow for the maintainers to modify the pull request.
54+
*/
55+
const maintainerCanModify = localCheckoutResult.pullRequest.maintainerCanModify;
12556

126-
let targetBranch = target;
127-
let targetName = target;
57+
if (!maintainerCanModify) {
58+
Log.info('The author of this pull request does not allow maintainers to modify the pull');
59+
Log.info('request. Since you will not be able to push changes to the original pull request');
60+
Log.info('you will instead need to perform a "takeover." In a takeover, the original pull');
61+
Log.info('request will be checked out, the commits are modified to close the original on');
62+
Log.info('merge of the newly created branch.');
12863

12964
if (
130-
target === 'patch' ||
131-
target === 'latest' ||
132-
activeReleaseTrains.latest.branchName === target
65+
await Prompt.confirm({
66+
message: `Would you like to create a takeover pull request?`,
67+
default: true,
68+
})
13369
) {
134-
targetName = 'patch';
135-
targetBranch = activeReleaseTrains.latest.branchName;
136-
} else if (
137-
target === 'main' ||
138-
target === 'next' ||
139-
target === 'minor' ||
140-
activeReleaseTrains.next.branchName === target
141-
) {
142-
targetName = 'main';
143-
targetBranch = activeReleaseTrains.next.branchName;
144-
} else if (
145-
activeReleaseTrains.releaseCandidate &&
146-
(target === 'rc' || activeReleaseTrains.releaseCandidate.branchName === target)
147-
) {
148-
targetName = 'rc';
149-
targetBranch = activeReleaseTrains.releaseCandidate.branchName;
150-
}
151-
Log.info(`Targeting '${targetBranch}' branch\n`);
152-
153-
const baseRefUrl = addTokenToGitHttpsUrl(pullRequest.baseRef.repository.url, git.githubToken);
154-
155-
git.run(['checkout', '-q', targetBranch]);
156-
git.run(['fetch', '-q', baseRefUrl, targetBranch, '--deepen=500']);
157-
git.run(['checkout', '-b', branchName]);
158-
159-
Log.info(`Running cherry-pick\n`);
160-
161-
try {
162-
const revisionRange = `${pullRequest.baseRefOid}..${pullRequest.headRefOid}`;
163-
git.run(['cherry-pick', revisionRange]);
164-
Log.info(`Cherry-pick is complete. You can now push to create a new pull request.`);
165-
} catch {
166-
Log.info(
167-
`Cherry-pick resulted in conflicts. Please resolve them manually and push to create your patch PR`,
168-
);
169-
return;
70+
return await checkoutAsPrTakeover(pr, localCheckoutResult);
17071
}
171-
172-
return;
17372
}
174-
}
17573

176-
/** Gets the absolute file path to the commit-message filter script. */
177-
function getCommitMessageFilterScriptPath(): string {
178-
// This file is getting bundled and ends up in `<pkg-root>/bundles/<chunk>`. We also
179-
// bundle the commit-message-filter script as another entry-point and can reference
180-
// it relatively as the path is preserved inside `bundles/`.
181-
// *Note*: Relying on package resolution is problematic within ESM and with `local-dev.sh`
182-
const bundlesDir = dirname(fileURLToPath(import.meta.url));
183-
return join(bundlesDir, './pr/checkout/commit-message-filter.mjs');
74+
Log.info(` ${green('✔')} Checked out the remote branch for pull request #${pr}`);
75+
if (maintainerCanModify) {
76+
Log.info('To push the checked out branch back to its PR, run the following command:');
77+
Log.info(` $ ${localCheckoutResult.pushToUpstreamCommand}`);
78+
}
18479
}

‎ng-dev/pr/checkout/cli.ts‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
import {Argv, Arguments, CommandModule} from 'yargs';
1010

11-
import {assertValidGithubConfig, getConfig, GithubConfig, NgDevConfig} from '../../utils/config.js';
1211
import {addGithubTokenOption} from '../../utils/git/github-yargs.js';
1312
import {checkoutPullRequest, CheckoutPullRequestParams} from './checkout.js';
1413

@@ -34,10 +33,7 @@ function builder(yargs: Argv) {
3433

3534
/** Handles the checkout pull request command. */
3635
async function handler({pr, takeover, target}: Arguments<CheckoutPullRequestParams>) {
37-
const config = await getConfig();
38-
assertValidGithubConfig(config);
39-
40-
await checkoutPullRequest({pr, takeover, target}, config);
36+
await checkoutPullRequest({pr, takeover, target});
4137
}
4238

4339
/** yargs command module for checking out a PR */

‎ng-dev/pr/checkout/takeover.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import {dirname, join} from 'path';
10+
import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client.js';
11+
import {bold, green, Log} from '../../utils/logging.js';
12+
import {Prompt} from '../../utils/prompt.js';
13+
import {checkOutPullRequestLocally} from '../common/checkout-pr.js';
14+
import {fileURLToPath} from 'url';
15+
16+
/** List of accounts that are supported for takeover. */
17+
const takeoverAccounts = ['angular-robot'];
18+
19+
/**
20+
* Checkout the provided pull request in preperation for a new takeover pull request to be made
21+
*/
22+
export async function checkoutAsPrTakeover(
23+
prNumber: number,
24+
{resetGitState, pullRequest}: Awaited<ReturnType<typeof checkOutPullRequestLocally>>,
25+
) {
26+
/** An authenticated git client. */
27+
const git = await AuthenticatedGitClient.get();
28+
/** The branch name to be used for the takeover attempt. */
29+
const branchName = `pr-takeover-${prNumber}`;
30+
31+
if (git.runGraceful(['rev-parse', '-q', '--verify', branchName]).status === 0) {
32+
Log.error(` ✘ Expected branch name \`${branchName}\` already exists locally`);
33+
return;
34+
}
35+
36+
// Validate that the takeover attempt is being made against a pull request created by an
37+
// expected account.
38+
if (!takeoverAccounts.includes(pullRequest.author.login)) {
39+
Log.warn(
40+
` ⚠ ${bold(pullRequest.author.login)} is not an account fully supported for takeover.`,
41+
);
42+
Log.warn(` Supported accounts: ${bold(takeoverAccounts.join(', '))}`);
43+
if (
44+
await Prompt.confirm({
45+
message: `Continue with pull request takeover anyway?`,
46+
default: true,
47+
})
48+
) {
49+
Log.debug('Continuing per user confirmation in prompt');
50+
} else {
51+
Log.info('Aborting takeover..');
52+
resetGitState();
53+
return;
54+
}
55+
}
56+
57+
Log.info(`Setting local branch name based on the pull request`);
58+
git.run(['checkout', '-q', '-b', branchName]);
59+
60+
Log.info('Updating commit messages to close previous pull request');
61+
git.run([
62+
'filter-branch',
63+
'-f',
64+
'--msg-filter',
65+
`${getCommitMessageFilterScriptPath()} ${prNumber}`,
66+
`${pullRequest.baseRefOid}..HEAD`,
67+
]);
68+
69+
Log.info(` ${green('✔')} Checked out pull request #${prNumber} into branch: ${branchName}`);
70+
}
71+
72+
/** Gets the absolute file path to the commit-message filter script. */
73+
function getCommitMessageFilterScriptPath(): string {
74+
// This file is getting bundled and ends up in `<pkg-root>/bundles/<chunk>`. We also
75+
// bundle the commit-message-filter script as another entry-point and can reference
76+
// it relatively as the path is preserved inside `bundles/`.
77+
// *Note*: Relying on package resolution is problematic within ESM and with `local-dev.sh`
78+
const bundlesDir = dirname(fileURLToPath(import.meta.url));
79+
return join(bundlesDir, './pr/checkout/commit-message-filter.mjs');
80+
}

0 commit comments

Comments
 (0)