nodejs / node-core-utils Goto Github PK
View Code? Open in Web Editor NEWCLI tools for Node.js Core collaborators
Home Page: https://nodejs.github.io/node-core-utils/
License: MIT License
CLI tools for Node.js Core collaborators
Home Page: https://nodejs.github.io/node-core-utils/
License: MIT License
$ ncu-config upstream your-remote-name
$ ncu-config branch your-branch-name
ncu-config
set upstream your-remote-name
?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:
Refs: github_url
in the messageThose are just a few ideas. Let me know if it makes sense to implement them here.
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 :
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:
docs
-> doc
, tls_wrap
-> tls
), or even generates one based on files changedOpening 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:
yargs
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:
[Squash]
in their title line, a nice warning saying that Squashing is requiredget-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.Refs: nodejs/node#18090
We need
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
$ get-metadata 16571
module.js:515
throw err;
^
Error: Cannot find module '../steps/metadata'
at Function.Module._resolveFilename (module.js:513:15)
at Function.Module._load (module.js:463:25)
at Module.require (module.js:556:17)
at require (internal/module.js:11:18)
at Object.<anonymous> (/home/mzasso/.nvm/versions/node/v8.8.0/lib/node_modules/node-core-utils/bin/get_metadata.js:5:21)
at Module._compile (module.js:612:30)
at Object.Module._extensions..js (module.js:623:10)
at Module.load (module.js:531:32)
at tryModuleLoad (module.js:494:12)
at Function.Module._load (module.js:486:3)
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):
doc
and test
Question: what is the ideal number of files? I would go for three, but I'm not an expert...
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:
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.land-pr master 12345
land-pr --continue
landing-12345
branch)[squash]
or [fixup]
messageland-pr --continue
git rev-list upstream/master...HEAD
?)git rev-list upstream/master...HEAD | xargs core-validate-commit
land-pr --continue
git checkout master && git reset --hard landing-12345
git push upstream master
git pull --rebase
land-pr --continue
git branch -D landing-12345
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:
nodejs
and repo to node
if not--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
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.