nodejs / node-core-utils Goto Github PK

$ ncu-config upstream your-remote-name
$ ncu-config branch your-branch-name 

• should this be ncu-config set upstream your-remote-name ?
• also, Error: ENOENT: no such file or directory, open '/Users/srl/src/node/.ncu/config'

It would be neat if it would log something on the console that the user is actually a first timer. That way we can write some nice things for the user after landing the commit and closing the PR.

I know that is visible in the PR itself but you have to pay attention to it and as soon as the PR landed, the label will be gone as well. So adding it to the console is probably best.

Tests seem to be randomly taking more than 1000ms on occasions, and end up ffor timeout reasons.

I'm going to up a bit the timeout, 1500ms seems a reasonable option for a start.

Currently if you run get-metadata on a closed PR, you don't get an obvious indication that it's closed.

▶▶▶ get-metadata 17899                                                                                                                                                                                                                ~/wrk/com/node-core-utils (xdg-config✦)
✔  Done loading data for nodejs/node/pull/17899
----------------------------------- PR info ------------------------------------
Title   doc: remove x86 from os.arch() options #17899
Author  gibfahn
Commits 1
Branch  gibfahn:process-arch -> nodejs:master
Labels  doc
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/17899
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
--------------------------------------------------------------------------------
✔  Requested Changes: 0
✔  Approvals: 7, 3 from TSC (jasnell, cjihrig, danbev)
⚠  Commits were pushed since the last review:
⚠  - doc: remove x86 from os.arch() options
ℹ  Last Full CI on 2018-01-03T22:59:42Z: https://ci.nodejs.org/job/node-test-commit/15215/
⚠  Commits were pushed after the last Full CI run:
⚠  - doc: remove x86 from os.arch() options

It should probably be made clear that the PR is closed.

*The three auth tests fail (looks like incompatible IPC parsing)
*MetadataGenerator has an unhandled rejection:

Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'currentRetry' of undefined

  auth
    1) asks for auth data if no ncurc is found
    2) asks for auth data if ncurc is invalid json
    3) returns ncurc data if it is present and valid

  CIparser
    √ should parse CI results

  Comparison
    √ should sort by ascending
    √ should sort by descending

  LinkParser
    √ should parse fixes and refs (87ms)

  MetadataGenerator
(node:11352) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'currentRetry' of undefined
(node:11352) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
    √ should generate metadata properly

  PRChecker
    √ should warn if no CI runs detected
    √ should summarize CI runs detected
    √ should check odd commits for first timers

  ReviewAnalyzer
    √ should parse reviews and comments that all approve


  9 passing (758ms)
  3 failing

  1) auth
       asks for auth data if no ncurc is found:

      AssertionError [ERR_ASSERTION]:   # test\unit\auth.test.js:75

  assert.strictEqual(expect.length, 0)
                     |      |
                     |      7
                     ["Reading configuration for node-core-utils failed:",/ENOENT: no such file or directory, open/,"Please enter your Github user information:",/Github tokens can be created as described in/,#Object#,#Object#,"bnlhbmNhdDowMTIzNDU2Nzg5YWJjZGVm"]

      + expected - actual

      -7
      +0

      at Decorator._callFunc (node_modules\empower-core\lib\decorator.js:110:20)
      at Decorator.concreteAssert (node_modules\empower-core\lib\decorator.js:103:17)
      at Function.decoratedAssert [as strictEqual] (node_modules\empower-core\lib\decorate.js:49:30)
      at ChildProcess.proc.on (d:\code\4node\node-core-utils/test\unit\auth.test.js:75:16)
      at maybeClose (internal/child_process.js:927:16)
      at Socket.stream.socket.on (internal/child_process.js:348:11)
      at Pipe._handle.close [as _onclose] (net.js:547:12)

  2) auth
       asks for auth data if ncurc is invalid json:
     Uncaught AssertionError [ERR_ASSERTION]: cmVmYWNrOmY4YzZkNmMyZWNhNDlkOWY1NDYzZTM0ODI3ODA5M2QzODI2ZDFlNTA=
 should match /^Reading configuration for node-core-utils failed:$/   # test\unit\auth.test.js:122

  assert(line.match(expected), `${ line } should match ${ expected }`)
         |    |     |
         |    null  /^Reading configuration for node-core-utils failed:$/
         "cmVmYWNrOmY4YzZkNmMyZWNhNDlkOWY1NDYzZTM0ODI3ODA5M2QzODI2ZDFlNTA=\n"

      at Decorator._callFunc (node_modules\empower-core\lib\decorator.js:110:20)
      at Decorator.concreteAssert (node_modules\empower-core\lib\decorator.js:103:17)
      at decoratedAssert (node_modules\empower-core\lib\decorate.js:49:30)
      at powerAssert (node_modules\empower-core\index.js:63:32)
      at onLine (d:\code\4node\node-core-utils/test\unit\auth.test.js:122:7)
      at Timeout.setTimeout (d:\code\4node\node-core-utils/test\unit\auth.test.js:101:13)

  3) auth
       returns ncurc data if it is present and valid:

      AssertionError [ERR_ASSERTION]:   # test\unit\auth.test.js:75

  assert.strictEqual(expect.length, 0)
                     |      |
                     |      7
                     ["Reading configuration for node-core-utils failed:",/Unexpected token h in JSON at position 1/,"Please enter your Github user information:",/Github tokens can be created as described in/,#Object#,#Object#,"bnlhbmNhdDowMTIzNDU2Nzg5YWJjZGVm"]

      + expected - actual

      -7
      +0

      at Decorator._callFunc (node_modules\empower-core\lib\decorator.js:110:20)
      at Decorator.concreteAssert (node_modules\empower-core\lib\decorator.js:103:17)
      at Function.decoratedAssert [as strictEqual] (node_modules\empower-core\lib\decorate.js:49:30)
      at ChildProcess.proc.on (d:\code\4node\node-core-utils/test\unit\auth.test.js:75:16)
      at maybeClose (internal/child_process.js:927:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:211:5)




d:\code\4node\node-core-utils\node_modules\empower-core\lib\decorator.js:110
        ret = func.apply(thisObj, args);
                   ^
AssertionError [ERR_ASSERTION]: cmVmYWNrOmY4YzZkNmMyZWNhNDlkOWY1NDYzZTM0ODI3ODA5M2QzODI2ZDFlNTA=
 should match /^bnlhbmNhdDowMTIzNDU2Nzg5YWJjZGVm$/   # test\unit\auth.test.js:122

  assert(line.match(expected), `${ line } should match ${ expected }`)
         |    |     |
         |    null  /^bnlhbmNhdDowMTIzNDU2Nzg5YWJjZGVm$/
         "cmVmYWNrOmY4YzZkNmMyZWNhNDlkOWY1NDYzZTM0ODI3ODA5M2QzODI2ZDFlNTA=\n"

    at Decorator._callFunc (d:\code\4node\node-core-utils\node_modules\empower-core\lib\decorator.js:110:20)
    at Decorator.concreteAssert (d:\code\4node\node-core-utils\node_modules\empower-core\lib\decorator.js:103:17)
    at decoratedAssert (d:\code\4node\node-core-utils\node_modules\empower-core\lib\decorate.js:49:30)
    at powerAssert (d:\code\4node\node-core-utils\node_modules\empower-core\index.js:63:32)
    at onLine (d:\code\4node\node-core-utils/test\unit\auth.test.js:122:7)
    at Timeout.setTimeout (d:\code\4node\node-core-utils/test\unit\auth.test.js:101:13)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:306:5)
    at Timer.listOnTimeout (timers.js:266:5)

Refs: nodejs/node#18210

On a side note, we should check that the latest CI runs should be reasonably fresh (at least not older than a week?)

Mocha by default cannot test async function it will mark it passed if the function does not throw Error of any sort synchronously.

I am working on fix. I just need to know all the async functions.

We have a few rules about commits to our V8 copy, that are documented in the maintaining V8 guide.
Some things could be easy to check:

• For backports / cherry-picks
  • Format of the commit title
  • Presence / correctness of the original commit message in the message
  • Presence of the Refs: github_url in the message
  • Has the embedder string been incremented?
• For regular (minor or major) updates
  • Format of the commit title

Those are just a few ideas. Let me know if it makes sense to implement them here.

nodejs/node#16604

Body HTML is:

<p>Included reference to \'constant time\' in crypto.timingSafeEqual description</p>\n<p><span aria-label="This pull request closes issue #16504." class="issue-keyword tooltipped tooltipped-se">Fixes</span> : <a href="https://github.com/nodejs/node/issues/16504" class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="268571876" data-permission-text="Issue title is private" data-url="https://github.com/nodejs/node/issues/16504">#16504</a></p>

Where there is a space before :

See nodejs/node#17705

Windows 7 x64
Node.js 9.2.0
get-metadata 1.6.1

Please, compare the outputs in a cmd.exe:

and Git Bash for Windows:

1. Git Bash output lacks colors.
2. Git Bash output duplicates metadata.

It passes locally and on appveyor but emits error on CI.

The existing https://www.npmjs.com/package/ghauth module would likely make the github authentication bits easier here. It is used by some of the other command line tools we use already (like make-node-meeting)

Quick question here, mainly for @joyeecheung :D

What is your preferred merge policy for PRs?
Do we stick to Node Core policy? (no using the "green button", etc etc)?
Do you prefer to review every PR and merge them yourself?

Got a branch working locally but need to write some better hint messages before opening a PR..

We need to read the email of the PR author in order to check if it matches the email of the commit author. This requires checking the boxes user:email for the personal access token.

Will update README later.

Should be behind a prompt, but would be very useful:

1. Wrap commit message bodies
2. Correct system labels (typos, common alias, e.g. docs -> doc, tls_wrap -> tls), or even generates one based on files changed
3. Remove duplicate links: see #133

Opening this issue to start working on the --help command arg and help define what needs to be shown with it.

For now, what I see:

• Add yargs
• General name and purpose of the command
• Basic usage example
• Arguments details

Please feel free to complete task list!
As of now, WIP!

If there are new commits after the last detected CI run (right now, any CI would do because we don't have labels to understand what type of CI a PR must go through), warn about it and print the commit titles (so the lander would know if it's just fixing nits, or can be reminded to double check).

I think this can be done in PRChecker.checkCI()

For instance...

$ get-metadata 16391
[TRACE] Getting collaborator contacts from README of nodejs/node
[TRACE] Getting PR from nodejs/node/pull/16391
[TRACE] Getting reviews from nodejs/node/pull/16391
[TRACE] Getting comments from nodejs/node/pull/16391
[INFO] Generated metadata:
-------------------------------- >8 --------------------------------
PR-URL: https://github.com/nodejs/node/pull/16391
Reviewed-By: Anna Henningsen <[email protected]>>
Reviewed-By: Colin Ihrig <[email protected]>>
Reviewed-By: Timothy Gu <[email protected]>>
Reviewed-By: Refael Ackermann <[email protected]>>
Reviewed-By: James M Snell <[email protected]>>
-------------------------------- 8< --------------------------------
[INFO] Rejections: 0
[INFO] Approvals: 5, 3 from TSC (addaleax, cjihrig, jasnell)
[INFO] Last Full CI on 2017-10-25T05:53:48Z: https://ci.nodejs.org/job/node-test-pull-request/10955/
[INFO] Last CITGM CI on 2017-10-25T05:53:48Z: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1028/

We need eslint plugin for json to fix this issue.

It'd be nice if I could do get-metadata (or get-metadata @) if I'm on a fork branch with an open PR.

Could do something like:

# Get the remote url of the upstream branch (if it exists).
remote=$(git remote get-url $(git rev-parse --abbrev-ref --symbolic-full-name @{u} | cut -d / -f 1))
branch=$(git symbolic-ref --short HEAD) # Or branch parameter passed.

# Convert to https:// syntax if it's ssh syntax ([email protected])

# Work out what PR has that as a base branch

# Pass PR number through to the rest of get-metadata.

The lib/collaborators.js test suite is very short. I'm improving it.

I create the issue just for inform you. Nodoby likes duplicating work.

I guess the lander will notice this when they push...but a warning in advance wouldn't hurt.

With the project growing everyday... Don't we should include a CHANGELOG.md?

So I have been holding this off because previously we did not have enough test coverage, the README was meh, and we didn't have help text which was confusing. Now that these problems seem to be fixed (thanks for everyone working on this project!) I think it's about time we move this into the foundation.

(I don't think anyone will object, but I may be wrong, so open an issue here first).

Right now if the tool is used as it the commit message that is generated duplicates Refs and Fixes entries by amending those blindly. We might come up with a way to remove those - and verify that they are identical with the ones being amended (and if not, it should result in an error).

fantastic work on this ... loving it.

A couple wish list items that I will try to get to but would love if others get to it first:

• An informational listing of the labels applied to the PR
• Printing the PR title
• Printing the number of individual commits in the PR (useful to know if squashing may be necessary)
• If any commits use [Squash] in their title line, a nice warning saying that Squashing is required
• Print the PR target branch, along with a warning if running get-metadata from within the node source tree and the current git branch does not match the target branch... to help defend against accidentally landing something in the wrong branch.
• Warning for "Dont Land on ..." labels

Refs: nodejs/node#18090

We need

1. A tool that
   • Collects the Github activities of a particular contributor, and a list of contributors
   • Prinst a summary based on the data (..or puts it in a json that's viewable with a custom UI in the browser?)
2. A tool that generates a collaborator nomination based on the contributor's activity

I am working on this, also I accidentally committed my query from my local branch in #106....
https://github.com/nodejs/node-core-utils/blob/master/lib/queries/SearchIssue.gql

module.js:529
    throw err;
    ^

Error: Cannot find module 'rimraf'
    at Function.Module._resolveFilename (module.js:527:15)
    at Function.Module._load (module.js:476:23)
    at Module.require (module.js:568:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/usr/local/lib/node_modules/node-core-utils/lib/session.js:6:16)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)

looks like rimraf is a dev and not a regular dependency

Those PRs usually have code-and-learn labels.

Refs: nodejs/node#16846

Hey!

Related to #89 , and as seen in #92(comment):

• check for labels doc and test
• check for a max number of files

Question: what is the ideal number of files? I would go for three, but I'm not an expert...

Seems like it's an old custom, but I think it's cute.

There are no tests for the ncu-config command at the moment. Related files:

• bin/ncu-config
• lib/file.js
• lib/config.js

Currently:

get-metadata 163754
× Getting commits from nodejs/node/pull/163754
×  GraphQL request Error
Error: GraphQL request Error
    at Request.request (C:\Users\targo\AppData\Roaming\nvm\v8.9.0\node_modules\node-core-utils\lib\request.js:50:19)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
{
  "variables": {
    "prid": 163754,
    "owner": "nodejs",
    "repo": "node"
  },
  "errors": [
    {
      "message": "Could not resolve to a PullRequest with the number of 163754.",
      "type": "NOT_FOUND",
      "path": [
        "repository",
        "pullRequest"
      ],
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ]
    }
  ]
}

At the moment my plan is:

1. Tests (meanwhile, fixing more TODOs)
2. CI (Travis?)
3. Making sure the repo name/npm package name and entries are right
   • I named this node-core-utils because I think a lot of checking and parsing stuff in lib could be useful for other tools, and if we don't want to maintain & document a stable API then putting them into one repo would make things easier.
   • get-metadata as it is now could be split into multiple binaries IMO, and we can make another binary that chains them together.
4. Create a pipeline that automates things that a collaborator needs to do as much as possible.

Pseudo targets:

1. land-pr master 12345
   1. Check PR status, warn about stuff
2. land-pr --continue
   1. Fetch patches
   2. Get the git status ready for a rebase (on a landing-12345 branch)
   3. (Alternatively?) squash commits with [squash] or [fixup] message
3. (...user does a git rebase to squash/fix nits)
4. land-pr --continue
   1. Generate metadata
   2. Somehow know what commits should be amended after the rebase (git rev-list upstream/master...HEAD?)
   3. (interactively?) append the metadata to the commits
   4. Call git rev-list upstream/master...HEAD | xargs core-validate-commit
   5. Probably do a bit more summaries, e.g. diffs, logs
5. land-pr --continue
   1. git checkout master && git reset --hard landing-12345
   2. Stop at this point, let the user do some more checks, ask them to manually trigger git push upstream master
   3. If fails, suggest a git pull --rebase
6. land-pr --continue
   1. git branch -D landing-12345
   2. Output the landed in <sha>...<sha> comments for copy-and-paste.

(^ can be modified a bit for backporting as well)

To make the pipeline eventually useful for a bot, the core would need to change the workflow a little bit (collaborators need to edit the branch first with a rebase in advance, then kick off the bot).

I am not sure at which stage we can transfer this to the Node.js foundation, I am thinking at least stage 3, maybe 4, because transferring it to the foundation would attract users and we need to get most things right first.

Opening a separate issue so it does not get buried...

After https://github.com/joyeecheung/node-core-utils/pull/70 we've fixed the fixes parsing bug (kudos to @apapirovski !) but now it would skip cross-repo fixes. I think we can reliably get the fixes url with getRefUrlFromOP() which parses the HTML to get the link.

It works! But would need some tests to make sure it does not regress..

That way we can do

# fetch and apply a PR to master
# rebase and edit every commit that will be pushed
# on every edit
$ git show -s --format=%B > msg.txt  # existing message
$ get-metadata $PRID -a msg.txt  # append metadata
$ git commit --amend -F msg.txt  # the whole thing

Hello! I just posted here: nodejs/automation#4 (comment).

I think a good way to get started and understand the code is play with it and make some tests.
So, if it seems good to you, I can improve the code coverage.

Regards.

Feature idea:
There's a good base in bin/metadata.js:parsePRId (lines 72 -78), but I think we could do more and directly parse the url for owner and repo, without removing actual options.

Usage would then be :
get-metadata <id-or-url> [<owner> <repo>]
If called with url: id, owner and repo would be extracted from it
If called with integer id: owner and repo would have to be given as args or else would default to nodejs and node respectively.

Todo:

• When first arg is url, extract owner, repo, and pr-id from it
• When first arg is integer:
  • Check second arg for owner and third arg for repo if given
  • Default owner to nodejs and repo to node if not
  • Allow args to be given in any other order when named? (--repo=node --owner=nodejs)

Feel free to add things!

I think it would be nice if the call would return a non-zero exit code in case a requirement is missing.

Example: nodejs/node#17107

IIRC, nodejs/node-review adds this data.

There are cases where one has to add a collaborator manually to the metadata since our heuristics are limited. It's worth adding a command or something that allows the users to add a collaborator there by specifying a login. Maybe something like

get-metadata -i msg.txt --add joyeecheung -f msg.txt

Really enjoyed it!

Have some ideas, but will open issues for them

Thanks for all the hard word everyone!!!

Refs: in the nodejs/node#17295, the Fixes https://github.com/nodejs/node/issues/17294 commit message line was not included in the metadata.

If a PR has a fast-track label on it, it should not be complained about. Right now there is a warning sign in case the PR is younger than 48 hours (do we check 72 hours on the weekend by the way?). But if the label is applied, that should not warn out of my perspective. And if the label is not applied, I would expect a error instead of just a warning.

Example: nodejs/node#16248 (review)

I wrote this:

#! /bin/bash

set -x
set -e

curl -L https://github.com/nodejs/node/pull/$1.patch | git am --whitespace=fix
get-metadata $1 -f msg.txt
echo -e "$(git show -s --format=%B)\n\n$(cat msg.txt)" > msg.txt
git commit --amend -F msg.txt
git show -s --format=%B
./configure
make -j4 test

I think it might be something we might want to add here, in node, or mention it in the README