search

nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

feat: add auto run v8 ci #652

Closed gengjiawen closed 1 year ago

gengjiawen commented 1 year ago

fix: https://github.com/nodejs/node-core-utils/issues/559 test pr: https://github.com/nodejs/node/pull/44986

Hopefully this will make us test v8 easier :)

local test

node bin/ncu-ci.js run 44986

currently I run into Jenkins credentials invalid. But I have used this credential before, and it worked.

cc @targos @richardlau @nodejs/v8-update

richardlau commented 1 year ago 
$ git rev-parse --short HEAD
8e866ef
$ node bin/ncu-ci.js --owner nodejs --repo node run 44961
✔  Jenkins credentials valid
⠋ Starting PR CI job(node:1644671) [https://github.com/node-fetch/node-fetch/issues/1167] DeprecationWarning: form-data doesn't follow the spec and requires special treatment. Use alternative package
(Use `node --trace-deprecation ...` to show where the warning was created)
✔  PR CI job successfully started
✖  Failed to start CI
$

More info on the failure -- the "Failed to start CI" message is from the try-catch handler https://github.com/nodejs/node-core-utils/blob/8e866ef03f9ae21301024eaf22db87eb212a58b2/lib/ci/run_ci.js#L102 With additional debug:

diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js
index e850a7a..51d6fb4 100644
--- a/lib/ci/run_ci.js
+++ b/lib/ci/run_ci.js
@@ -99,6 +99,7 @@ export class RunPRJob {
       }

     } catch (err) {
+      console.error(err);
       cli.stopSpinner('Failed to start CI', this.cli.SPINNER_STATUS.FAILED);
       return false;
     }

the error is:

$ node bin/ncu-ci.js --owner nodejs --repo node run 44986
✔  Jenkins credentials valid
✔  PR CI job successfully started
TypeError: Cannot read property 'gql' of undefined
    at PRData.getPR (file:///home/rlau/sandbox/github/node-core-utils/lib/pr_data.js:80:34)
    at RunPRJob.start (file:///home/rlau/sandbox/github/node-core-utils/lib/ci/run_ci.js:81:25)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
✖  Failed to start CI
gengjiawen commented 1 year ago

Is the credential put in ~/.ncurc

richardlau commented 1 year ago

Is the credential put in ~/.ncurc

Yes. Credential is working -- the pull request job was successfully started and it's the additions in this PR that are not working.

gengjiawen commented 1 year ago

Is the credential put in ~/.ncurc

Yes. Credential is working -- the pull request job was successfully started and it's the additions in this PR that are not working.

Yeap. On my local I stuck at credential issue. So I mark it as draft.

gengjiawen commented 1 year ago

@legendecas Is this because proxy issue, not supporting socks5 proxy ?

Update: a China only issue, fucking gfw. proxy-agent looks only work in http proxy.

gengjiawen commented 1 year ago

now jenkins with 500. Can you check the log @richardlau , thx.

gengjiawen commented 1 year ago

Now the issues is it's not auto-commenting pr ? which part should I check ?

richardlau commented 1 year ago

Looks good now (apart from the lint and test failures) and V8 CI is started:

$ node bin/ncu-ci.js --owner nodejs --repo node run 44986
✔  Jenkins credentials valid
⠋ Starting PR CI job(node:1665555) [https://github.com/node-fetch/node-fetch/issues/1167] DeprecationWarning: form-data doesn't follow the spec and requires special treatment. Use alternative package
(Use `node --trace-deprecation ...` to show where the warning was created)
✔  PR CI job successfully started
{ nodes: [ { name: 'v8 engine' } ] }
✔  V8 CI job successfully started
$

Started:

Refs: https://github.com/nodejs/node/pull/44986#issuecomment-1277653933

Now the issues is it's not auto-commenting pr ? which part should I check ?

This is done by the github-bot. I think this is the relevant code: https://github.com/nodejs/github-bot/blob/2f5b61f263ebeda2d9525733bb9dfa055fa24660/lib/push-jenkins-update.js#L15-L27

gengjiawen commented 1 year ago

test fixed.

please squash this commit for conventional commit.

codecov[bot] commented 1 year ago

Codecov Report

Base: 84.03% // Head: 83.60% // Decreases project coverage by -0.42% :warning:

Coverage data is based on head (232b7d2) compared to base (4e59a64). Patch coverage: 35.00% of modified lines in pull request are covered.

:exclamation: Current head 232b7d2 differs from pull request most recent head 5fa57b9. Consider uploading reports for the commit 5fa57b9 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #652 +/- ## ========================================== - Coverage 84.03% 83.60% -0.43% ========================================== Files 37 37 Lines 4077 4117 +40 ========================================== + Hits 3426 3442 +16 - Misses 651 675 +24 ``` | [Impacted Files](https://codecov.io/gh/nodejs/node-core-utils/pull/652?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [lib/ci/run\_ci.js](https://codecov.io/gh/nodejs/node-core-utils/pull/652/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-bGliL2NpL3J1bl9jaS5qcw==) | `78.33% <35.00%> (-21.67%)` | :arrow_down: | | [lib/verbosity.js](https://codecov.io/gh/nodejs/node-core-utils/pull/652/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-bGliL3ZlcmJvc2l0eS5qcw==) | `80.76% <0.00%> (+7.69%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

legendecas commented 1 year ago

@gengjiawen: @legendecas Is this because proxy issue, not supporting socks5 proxy ?

Update: a China only issue, fucking gfw. proxy-agent looks only work in http proxy.

Not sure what I can help here?

gengjiawen commented 1 year ago

@gengjiawen: @legendecas Is this because proxy issue, not supporting socks5 proxy ? Update: a China only issue, fucking gfw. proxy-agent looks only work in http proxy.

Not sure what I can help here?

fixed, proxy compatibility issue

gengjiawen commented 1 year ago

@joyeecheung Can you review this ?

gengjiawen commented 1 year ago

@aduh95 @nodejs/node-core-utils Can you take a look at this ? thx

Trott commented 1 year ago

This looks good to me. Is there anything we can/should do about the small decrease in test coverage? I'm not worried about it if there isn't much to do.

Trott commented 1 year ago

please squash this commit for conventional commit.

Do you want to squash and force push to your branch?

gengjiawen commented 1 year ago

please squash this commit for conventional commit.

Do you want to squash and force push to your branch?

I would like merger do this like we do in nodejs/node-gyp and nodejs/gyp-next.

gengjiawen commented 1 year ago

This looks good to me. Is there anything we can/should do about the small decrease in test coverage? I'm not worried about it if there isn't much to do.

It's a duplicate logic already tested (different URL and HTTP payload for v8 jenkins task). I think in this case, it's okay.

gengjiawen commented 1 year ago

@Trott Can you merge this and https://github.com/nodejs/github-bot/pull/353 (already reviewed by @joyeecheung ) ? thx