Skip to content

Commit f841e36

Browse files
mheverymhevery
committed
ci: scripts to review PRs locally (angular#24623)
PR Close angular#24623
1 parent f229449 commit f841e36

File tree

4 files changed

+366
-0
lines changed

4 files changed

+366
-0
lines changed

‎docs/PR_REVIEW.md

+155
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
# PR Review
2+
3+
## Tools
4+
5+
A better way to do a code-review of a PR is to do it in your IDE.
6+
Here are two scripts which allow you to perform the review and create local changes which can be appended to the PR.
7+
8+
### 1. Loading PR
9+
10+
Run this command to load the changes into your local repository where your IDE is running.
11+
12+
```
13+
$ ./scripts/github/review-pr 24623
14+
```
15+
16+
This will result in output:
17+
18+
```
19+
Already on 'master'
20+
Your branch is up to date with 'origin/master'.
21+
Fetching pull request #24623 with 1 SHA(s) into branch range: pr/24623_base..pr/24623_top
22+
======================================================================================
23+
cef93a51b (pr/24623_top) ci: scripts to review PRs locally
24+
======================================================================================
25+
Switched to a new branch 'pr/24623'
26+
On branch pr/24623
27+
Untracked files:
28+
(use "git add <file>..." to include in what will be committed)
29+
30+
docs/PR_REVIEW.md
31+
scripts/github/push-pr
32+
scripts/github/review-pr
33+
34+
nothing added to commit but untracked files present (use "git add" to track)
35+
```
36+
37+
Note that the script created `pr/24623_top` and `pr/24623_base` branches which denote SHAs where the PR start and end.
38+
39+
```
40+
cef93a51b (pr/24623_top) ci: scripts to review PRs locally
41+
637805a0c (pr/24623_base) docs: update `lowercase` pipe example in "AngularJS to Angular" guide (#24588)
42+
```
43+
44+
Knowing `pr/24623_top` and `pr/24623_base` makes it convenient to refer to different SHAs in PR when rebasing or reseting.
45+
46+
### 2. Review PR
47+
48+
Because the script has reset the `HEAD` of the PR the changes show up as unstaged files.
49+
50+
```
51+
$ git status
52+
On branch pr/24623
53+
Untracked files:
54+
(use "git add <file>..." to include in what will be committed)
55+
56+
docs/PR_REVIEW.md
57+
scripts/github/push-pr
58+
scripts/github/review-pr
59+
60+
nothing added to commit but untracked files present (use "git add" to track)
61+
```
62+
63+
Use your IDE to review the untracked files as needed.
64+
A good trick is to use your IDE to stage the files which were already reviewed.
65+
When all files are staged the review is done.
66+
67+
### 3. Creating Edits
68+
69+
At any point you can edit any line in the repository.
70+
The idea is to create edits locally and push them to the PR later.
71+
This is useful because it is often times easier to make minor changes locally than to request the PR author to change and repush through a comment (often times the comment is larger than the change.)
72+
73+
Example of a local edit.
74+
```
75+
echo "# here is a change" >> docs/PR_REVIEW.md
76+
```
77+
78+
### 4. Creating a Commit From Local Edits
79+
80+
Since the HEAD has been reset to `pr/24623_base` so that changes show up in `git status` we have to reverse the reset to only see our local changes.
81+
To do that reset the `HEAD` to `pr/24623_top`.
82+
83+
```
84+
$ git reset pr/24623_top
85+
```
86+
87+
Doing so will remove all PR changes and only leave your local modifications which you have done.
88+
You can verify by running `git status` and `git diff` to see only your changes (PR changes have been removed.)
89+
90+
```
91+
$ git status
92+
On branch pr/24623
93+
Changes not staged for commit:
94+
(use "git add <file>..." to update what will be committed)
95+
(use "git checkout -- <file>..." to discard changes in working directory)
96+
97+
modified: docs/PR_REVIEW.md
98+
99+
no changes added to commit (use "git add" and/or "git commit -a")
100+
```
101+
```
102+
$ git diff
103+
diff --git a/docs/PR_REVIEW.md b/docs/PR_REVIEW.md
104+
index 184b5aeca..83517fbe0 100644
105+
--- a/docs/PR_REVIEW.md
106+
+++ b/docs/PR_REVIEW.md
107+
@@ -8,4 +8,4 @@ A better way to do code review of the PR is to do it in your IDE. Here are two s
108+
existing text
109+
-
110+
\ No newline at end of file
111+
+# here is a change
112+
```
113+
114+
Next step is to turn your local changes into a `fixup!` commit.
115+
Run `git commit --all --fixup HEAD` to create a `fixup!` commit.
116+
117+
NOTE: If you added new files they must be added using `git add .` or they will not be picked up by the `git commit --all` flag.
118+
119+
```
120+
$ git commit --all --fixup HEAD
121+
[pr/24623 45ae87ce4] fixup! ci: scripts to review PRs locally
122+
1 file changed, 1 insertion(+), 1 deletion(-)
123+
```
124+
125+
You can verify that the `fixup!` commit with your local modifications was created.
126+
```
127+
$ git log --oneline
128+
45ae87ce4 (HEAD -> pr/24623) fixup! ci: scripts to review PRs locally
129+
cef93a51b (pr/24623_top) ci: scripts to review PRs locally
130+
```
131+
132+
### 5. Pushing local edits back to the PR
133+
134+
The last step is to push your local changes back into the PR.
135+
Use `./scripts/github/push-pr` script for that.
136+
137+
```
138+
$ ./scripts/github/push-pr
139+
Assuming PR #24623
140+
>>> git push [email protected]:mhevery/angular.git HEAD:review_pr_script
141+
Counting objects: 4, done.
142+
Delta compression using up to 8 threads.
143+
Compressing objects: 100% (4/4), done.
144+
Writing objects: 100% (4/4), 392 bytes | 392.00 KiB/s, done.
145+
Total 4 (delta 3), reused 0 (delta 0)
146+
remote: Resolving deltas: 100% (3/3), completed with 3 local objects.
147+
To github.com:mhevery/angular.git
148+
cef93a51b..45ae87ce4 HEAD -> review_pr_script
149+
```
150+
151+
NOTE: Notice that we did not have to specify the PR number since the script can guess it from the branch name.
152+
153+
If you visit https://github.com/angular/angular/pull/24623/commits you will see that your `fixup!` commit has been added to the PR.
154+
This greatly simplifies the work for many minor changes to the PR.
155+

‎scripts/github/push-pr

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
#!/usr/bin/env node
2+
3+
const shell = require('shelljs');
4+
shell.config.fatal = true;
5+
const util = require('./utils/git_util');
6+
7+
if (require.main === module) {
8+
main(process.argv.splice(2)).then(
9+
(v) => process.exitCode,
10+
(e) => console.error(process.exitCode = 1, e)
11+
);
12+
}
13+
14+
async function main(args) {
15+
let forceWithLease = '';
16+
let prNumber = 0;
17+
let printHelp = false;
18+
19+
args.forEach((arg) => {
20+
if (prNumber == 0 && arg > 0) {
21+
prNumber = arg;
22+
} else if (arg == '--help') {
23+
printHelp = true;
24+
} else if (arg == '--force-with-lease') {
25+
forceWithLease = ' --force-with-lease';
26+
} else {
27+
shell.echo('Unexpected argument: ', arg);
28+
}
29+
});
30+
31+
if (!prNumber) {
32+
const branch = util.getCurrentBranch();
33+
const maybePr = branch.split('/')[1];
34+
if (maybePr > 0) {
35+
shell.echo(`PR number not specified. Defaulting to #${maybePr}.`);
36+
prNumber = maybePr;
37+
}
38+
}
39+
40+
if (!prNumber || printHelp) {
41+
shell.echo(`Push the current HEAD into an existing pull request.`);
42+
shell.echo(``);
43+
shell.echo(`${process.argv[1]} [PR_NUMBER] [--force-with-lease]`);
44+
shell.echo(``);
45+
shell.echo(` --force-with-lease Continues even \if change can\'t be fast-forwarded.`);
46+
shell.echo(` [PR_NUMBER] If not present the script guesses the PR from the branch name.`);
47+
return 1;
48+
}
49+
50+
const prInfo = await util.githubPrInfo(prNumber);
51+
const prPushCmd = `git push${forceWithLease} ${prInfo.repository.gitUrl} HEAD:${prInfo.branch}`;
52+
shell.echo(`>>> ${prPushCmd}`);
53+
shell.exec(prPushCmd);
54+
55+
return 0;
56+
}

‎scripts/github/review-pr

+60
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
#!/usr/bin/env node
2+
3+
const shell = require('shelljs');
4+
shell.config.fatal = true;
5+
const util = require('./utils/git_util');
6+
7+
if (require.main === module) {
8+
main(process.argv.splice(2)).then(
9+
(v) => process.exitCode = v,
10+
(e) => console.error(process.exitCode = 1, e)
11+
);
12+
}
13+
14+
async function main(args) {
15+
let prNumber = 0;
16+
17+
args.forEach((arg) => {
18+
if (prNumber == 0 && arg > 0) {
19+
prNumber = arg;
20+
} else {
21+
shell.echo('Unexpected argument: ', arg);
22+
}
23+
});
24+
25+
if (prNumber === 0) {
26+
shell.echo('Bring github pull request onto your local repo for review and edit');
27+
shell.echo('');
28+
shell.echo(`${process.argv[1]} PR_NUMBER`);
29+
shell.echo('');
30+
return 1;
31+
}
32+
33+
if (util.gitHasLocalModifications()) {
34+
shell.echo('Local modification detected. exiting...');
35+
return 1;
36+
}
37+
38+
let prShaCount = (await util.githubPrInfo(prNumber)).commits;
39+
40+
shell.exec(`git checkout master`);
41+
if (util.execNoFatal(`git rev-parse --verify --quiet pr/${prNumber}`).code == 0) {
42+
shell.exec(`git branch -D pr/${prNumber}`);
43+
}
44+
45+
shell.echo(`Fetching pull request #${prNumber} with ${prNumber} SHA(s) into branch range: pr/${prNumber}_base..pr/${prNumber}_top`);
46+
shell.exec(`git fetch -f [email protected]:angular/angular.git pull/${prNumber}/head:pr/${prNumber}_top`);
47+
48+
shell.exec(`git branch -f pr/${prNumber}_base pr/${prNumber}_top~${prShaCount}`);
49+
50+
shell.echo(`======================================================================================`);
51+
shell.exec(`git log --oneline --color pr/${prNumber}_base..pr/${prNumber}_top`);
52+
shell.echo(`======================================================================================`);
53+
54+
// Reset the HEAD so that we can see changed files for review
55+
shell.exec(`git checkout --force -b pr/${prNumber} pr/${prNumber}_top`);
56+
shell.exec(`git reset pr/${prNumber}_base`);
57+
shell.exec(`git status`);
58+
59+
return 0;
60+
}

‎scripts/github/utils/git_util.js

+95
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
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+
const https = require('https');
10+
const shell = require('shelljs');
11+
12+
function httpGet(server, path, headers) {
13+
return new Promise((resolve, reject) => {
14+
const options = {
15+
hostname: server,
16+
port: 443,
17+
path: path,
18+
method: 'GET',
19+
headers: {'User-Agent': 'script', ...headers}
20+
};
21+
https
22+
.get(
23+
options,
24+
(res) => {
25+
let json = '';
26+
res.on('data', (chunk) => json += chunk.toString());
27+
res.on('end', () => resolve(json));
28+
})
29+
.on('error', (e) => reject(e));
30+
});
31+
};
32+
33+
let warnNoToken = true;
34+
35+
async function githubGet(path) {
36+
const token = process.env['TOKEN'];
37+
const headers = {};
38+
if (token) {
39+
headers.Authorization = 'token ' + token;
40+
} else if (warnNoToken) {
41+
warnNoToken = false;
42+
console.warn('############################################################');
43+
console.warn('############################################################');
44+
console.warn('WARNING: you should set the TOKEN variable to a github token');
45+
console.warn('############################################################');
46+
console.warn('############################################################');
47+
}
48+
49+
return JSON.parse(await httpGet('api.github.com', '/repos/angular/angular/' + path, headers));
50+
};
51+
52+
async function githubPrInfo(prNumber) {
53+
const pr = (await githubGet('pulls/' + prNumber));
54+
const label = pr.head.label.split(':');
55+
const user = label[0];
56+
const branch = label[1];
57+
return {
58+
commits: pr.commits,
59+
repository: {
60+
user: user,
61+
gitUrl: `[email protected]:${user}/angular.git`,
62+
},
63+
branch: branch
64+
};
65+
}; // trailing ; so that clang-format is not confused on async function
66+
67+
function gitHasLocalModifications() {
68+
return execNoFatal('git diff-index --quiet HEAD --').code != 0;
69+
}
70+
71+
function execNoFatal(cmd, options) {
72+
const fatal = shell.config.fatal;
73+
try {
74+
shell.config.fatal = false;
75+
return shell.exec(cmd, options);
76+
} finally {
77+
shell.config.fatal = fatal;
78+
}
79+
}
80+
81+
function getCurrentBranch() {
82+
return shell.exec('git branch', {silent: true})
83+
.stdout.toString()
84+
.split('\n') // Break into lines
85+
.map((v) => v.trim()) // trim
86+
.filter((b) => b[0] == '*') // select current branch
87+
.map((b) => b.split(' ')[1])[0]; // remove leading `*`
88+
}
89+
90+
exports.httpGet = httpGet;
91+
exports.githubGet = githubGet;
92+
exports.githubPrInfo = githubPrInfo;
93+
exports.gitHasLocalModifications = gitHasLocalModifications;
94+
exports.execNoFatal = execNoFatal;
95+
exports.getCurrentBranch = getCurrentBranch;

0 commit comments

Comments
 (0)