ci: scripts to review PRs locally by mhevery · Pull Request #24623 · angular/angular

mhevery · 2018-06-22T04:57:56Z

No description provided.

alxhub · 2018-06-22T06:10:50Z

+
+
+# Fetch the userame:branchname for the PR
+HEAD_LABEL=`curl -s https://api.github.com/repos/angular/angular/pulls/$PR_NUMBER | node $BASEDIR/utils/json_extract.js head.label`


I believe many of our scripts use a TOKEN environment variable to avoid Github request limits.

alxhub · 2018-06-22T06:12:07Z

+PULL_JSON=`$GHCURL -s https://api.github.com/repos/angular/angular/pulls/$PR_NUMBER`
+PR_SHA_COUNT=`node $BASEDIR/utils/json_extract.js commits <<< """$PULL_JSON"""`
+
+## reset the state of the current repo to a know state so that following commands don't collide.


I would prefer it if the script failed if:

The current branch is not master, or

If the current branch isn't in a clean state.

Doing otherwise risks losing work in progress if the script is run on a working branch accidentally.

Added check for clean state.

gkalpak · 2018-06-22T07:57:01Z

+cef93a51b (pr/24623_top) ci: scripts to review PRs locally
+```
+
+### 5. Pushing local edits pack to the PR


pack --> back

gkalpak · 2018-06-22T08:03:55Z

+
+  case $key in
+      --force)
+      FORCE=-f


😱   😱                                                      😱
         😱             😱                                                    😱
     😱              😱                                                           😱      😱
        😱         😱      😱        😱                                 😱           😱    😱      😱
    😱                         😱   😱     😱                                                 😱
        😱              😱                                                   😱
         😱                                               😱  😱                        😱  😱

-f is evil.
Always --force-with-lease 😁

did not know about --force-with-lease updated with it. :-)

IgorMinar · 2018-06-22T09:14:56Z

+echo "# here is a change" >> docs/PR_REVIEW.md
+```
+
+### 4. Creating a SHA from local Edit


incorrect terminology: SHA is the fingerprint of a commit, you mean the "commit" not "fingerprint of a commit" here.

Maybe say: "Committing local changes" ?

IgorMinar

Why don't you just use https://hub.github.com/, it gives you all the scripts and

With hub all I do is:

hub checkout https://github.com/angular/angular/pull/24623/files
# hack hack hack
git commit -p --fixup HEAD
git push
# copy the suggested origin and branch from `git push`
git push mhevery HEAD:review_pr_script

IgorMinar · 2018-06-22T09:19:17Z

+  echo
+  echo "$0 PR_NUMBER [--force]"
+  echo
+  echo    --force    Continues even if PR status is not green.


"not green" ?

copy pasta. Fixed

IgorMinar · 2018-06-22T09:20:14Z

@@ -0,0 +1,52 @@
+#!/usr/bin/env bash


I thought we agreed to write future scripts in shelljs because bash scripts are hard to maintain

rewritten as shelljs

mhevery

Fixed all comments, please take another look.

The reason I am not using https://hub.github.com/ is because it does not do anything close to this script.

https://hub.github.com/ has an easy way to check out change, but that is it. It does not know how to reset, do local mods and push back to PR.

mhevery · 2018-06-24T21:12:22Z

@@ -0,0 +1,52 @@
+#!/usr/bin/env bash


rewritten as shelljs

mhevery · 2018-06-24T21:12:48Z

+
+  case $key in
+      --force)
+      FORCE=-f


did not know about --force-with-lease updated with it. :-)

mhevery · 2018-06-24T21:12:59Z

+  echo
+  echo "$0 PR_NUMBER [--force]"
+  echo
+  echo    --force    Continues even if PR status is not green.


copy pasta. Fixed

mhevery · 2018-06-24T21:13:06Z

+
+
+# Fetch the userame:branchname for the PR
+HEAD_LABEL=`curl -s https://api.github.com/repos/angular/angular/pulls/$PR_NUMBER | node $BASEDIR/utils/json_extract.js head.label`


mhevery · 2018-06-24T21:13:20Z

+PULL_JSON=`$GHCURL -s https://api.github.com/repos/angular/angular/pulls/$PR_NUMBER`
+PR_SHA_COUNT=`node $BASEDIR/utils/json_extract.js commits <<< """$PULL_JSON"""`
+
+## reset the state of the current repo to a know state so that following commands don't collide.


Added check for clean state.

petebacondarwin · 2018-06-24T21:23:52Z

+nothing added to commit but untracked files present (use "git add" to track)
+```
+
+Use your IDE to review the untractked files as needed.


untracked typo

petebacondarwin · 2018-06-24T21:24:25Z

+nothing added to commit but untracked files present (use "git add" to track)
+```
+
+Note that the script created `pr/24623_top` and `pr/24623_base` branches which denote where the PR start and end SHA.


"which denote the PR start and end SHAs"?

petebacondarwin · 2018-06-24T21:26:01Z

+
+### 4. Creating a Commit From Local Edits
+
+Since the HEAD has been reset to `pr/24623_base` so that changes show up in `git status` we have to do reverse the reset to only see our local changes.


"we have to reverse the reset"

petebacondarwin · 2018-06-24T21:27:57Z

+Run `git commit --all --fixup HEAD` to create a `fixup!` commit.
+
+```
+$ git commit --all --fixup HEAD


Should we note that newly added file (that git doesn't know about) will not be included via the --all flag?. They must be added using git add first.

added comment.

petebacondarwin

LGTM except for a handful of typos

gkalpak · 2018-06-25T13:04:31Z

+let warnNoToken = true;
+
+async function githubGet(path) {
+  const token = process.env['TOKEN'];


TOKEN is a bit generic. Something like GITHUB_TOKEN would be preferrable imo.

True, but all other scripts use it so I need to be consistent. We should probably have a separate refactoring to change TOKEN to GITHUB_TOKEN

gkalpak · 2018-06-25T13:16:34Z

+          forceWithLease = ' --force-with-lease';
+      } else {
+          shell.echo('Unexpected argument: ', arg);
+      }


Inconsistent indentation 😇

gkalpak · 2018-06-25T13:19:11Z

+    shell.echo(``);
+    shell.echo(`${process.argv[1]} [PR_NUMBER] [--force-with-lease]`);
+    shell.echo(``);
+    shell.echo(`    --force-with-lease    Continues even \if change can\'t be fast-forwarded.`);


\if --> if

Unfortunately if I don't escape if than the the editor gets confuse and color coding gets messed up. The reason why the editor is confused is that the file does not end with .ts and so the editor thinks it is a shell file.

Aha! That explains the weird escaping 😁
OOC, why does the file have no extension (.js for example)?

gkalpak · 2018-06-25T13:21:40Z

+
+if (require.main === module) {
+  main(process.argv.splice(2)).then(
+    (v) => process.exitCode = v ? 0 : 1, 


The ? 0 : 1 part seems confusing (and redundant?).

gkalpak · 2018-06-25T13:27:02Z

@@ -0,0 +1,95 @@
+/**


Inconsistent file naming (git_util vs git-util).

git_util is consistent with the rest of the repo, but merge-pr is not. merge-pr is the inconsistent one, but I am doing it to be consistent with merge-pr.

👍
_{The repo is full of both `_`- and `-`-separated filenames 😁

I was mainly referring to `merge-pr` and `push-pr` (vs `git_util`).}

gkalpak · 2018-06-25T13:32:54Z

+    shell.exec(`git branch -D pr/${prNumber}`);
+  }
+
+  shell.echo(`Fetching pull request \#${prNumber} with ${prNumber} SHA\(s\) into branch range: pr/${prNumber}_base..pr/${prNumber}_top`);


with ${prNumber} SHA$s$ --> with ${prShaCount} SHA(s)

JoostK · 2018-06-25T16:24:40Z

 if (require.main === module) {
  main(process.argv.splice(2)).then(
-    (v) => process.exitCode = v ? 0 : 1, 
+    (v) => process.exitCode, 


Oops, this can't be right. Statement no longer has side-effects.

You are right. The = v part should stay (only the ? 0 : 1 was redundant).
Feel free to submit a PR with the fix, @JoostK 😉

angular-automatic-lock-bot · 2019-09-13T18:51:07Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ci: scripts to review PRs locally

cef93a5

googlebot added the cla: yes label Jun 22, 2018

fixup! ci: scripts to review PRs locally

b2d7566

mhevery force-pushed the review_pr_script branch from 45ae87c to b2d7566 Compare June 22, 2018 05:33

fixup! ci: scripts to review PRs locally

d6a6d8d

mhevery added the target: major This PR is targeted for the next major release label Jun 22, 2018

mhevery requested a review from alxhub June 22, 2018 05:43

alxhub reviewed Jun 22, 2018

View reviewed changes

alxhub approved these changes Jun 22, 2018

View reviewed changes

gkalpak reviewed Jun 22, 2018

View reviewed changes

IgorMinar reviewed Jun 22, 2018

View reviewed changes

mhevery commented Jun 24, 2018

View reviewed changes

mhevery added 2 commits June 24, 2018 14:15

fixup! ci: scripts to review PRs locally

3dfe5c4

fixup! ci: scripts to review PRs locally

727d478

mhevery added the action: merge The PR is ready for merge by the caretaker label Jun 24, 2018

petebacondarwin reviewed Jun 24, 2018

View reviewed changes

petebacondarwin approved these changes Jun 24, 2018

View reviewed changes

gkalpak reviewed Jun 25, 2018

View reviewed changes

fixup! ci: scripts to review PRs locally

3f87e95

mhevery force-pushed the review_pr_script branch from 2804b2d to 3f87e95 Compare June 25, 2018 15:32

mhevery closed this in f841e36 Jun 25, 2018

JoostK reviewed Jun 25, 2018

View reviewed changes

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019



		# Fetch the userame:branchname for the PR
		HEAD_LABEL=`curl -s https://api.github.com/repos/angular/angular/pulls/$PR_NUMBER \| node $BASEDIR/utils/json_extract.js head.label`


		### 4. Creating a Commit From Local Edits

		Since the HEAD has been reset to `pr/24623_base` so that changes show up in `git status` we have to do reverse the reset to only see our local changes.

Uh oh!

Conversation

mhevery commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar left a comment •

edited

Loading

mhevery Jun 25, 2018 •

edited

Loading

mhevery Jun 25, 2018 •

edited

Loading