Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add auto run v8 ci #652

Merged
merged 1 commit into from
Oct 22, 2022
Merged

feat: add auto run v8 ci #652

merged 1 commit into from
Oct 22, 2022

Conversation

gengjiawen
Copy link
Member

@gengjiawen gengjiawen commented Oct 13, 2022

fix: #559
test pr: nodejs/node#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

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo the style changes causing the linter to fail. Also this doesn't work:

$ 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
$

lib/ci/run_ci.js Outdated Show resolved Hide resolved
@richardlau
Copy link
Member

$ 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

cli.stopSpinner('Failed to start CI', this.cli.SPINNER_STATUS.FAILED);

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
Copy link
Member Author

Is the credential put in ~/.ncurc

@richardlau
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

gengjiawen commented Oct 13, 2022

@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

This comment was marked as outdated.

@gengjiawen
Copy link
Member Author

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

@gengjiawen gengjiawen marked this pull request as ready for review October 13, 2022 13:00
lib/ci/run_ci.js Outdated Show resolved Hide resolved
@richardlau
Copy link
Member

richardlau commented Oct 13, 2022

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: nodejs/node#44986 (comment)

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
Copy link
Member Author

gengjiawen commented Oct 13, 2022

test fixed.

please squash this commit for conventional commit.

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

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

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

❗ 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
@@            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 Coverage Δ
lib/ci/run_ci.js 78.33% <35.00%> (-21.67%) ⬇️
lib/verbosity.js 80.76% <0.00%> (+7.69%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@legendecas
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member Author

@joyeecheung Can you review this ?

@gengjiawen
Copy link
Member Author

gengjiawen commented Oct 19, 2022

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

.gitignore Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Oct 19, 2022

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.

.npmrc Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Oct 19, 2022

please squash this commit for conventional commit.

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

@gengjiawen
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

@Trott Can you merge this and nodejs/github-bot#353 (already reviewed by @joyeecheung ) ? thx

.gitignore Outdated Show resolved Hide resolved
@Trott Trott merged commit 046bc0d into nodejs:main Oct 22, 2022
@gengjiawen gengjiawen deleted the feat/v8 branch October 22, 2022 00:38
richardlau added a commit to richardlau/node-1 that referenced this pull request Oct 25, 2022
Recent changes to node-core-utils attempt to read pull request labels
when deciding whether or not to start a V8 CI. This requires a valid
GitHub token with appropriate permissions to be set for node-core-utils.

Refs: nodejs/node-core-utils#652
nodejs-github-bot pushed a commit to nodejs/node that referenced this pull request Oct 26, 2022
Recent changes to node-core-utils attempt to read pull request labels
when deciding whether or not to start a V8 CI. This requires a valid
GitHub token with appropriate permissions to be set for node-core-utils.

Refs: nodejs/node-core-utils#652
PR-URL: #45185
Fixes: #45163
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this pull request Nov 1, 2022
Recent changes to node-core-utils attempt to read pull request labels
when deciding whether or not to start a V8 CI. This requires a valid
GitHub token with appropriate permissions to be set for node-core-utils.

Refs: nodejs/node-core-utils#652
PR-URL: #45185
Fixes: #45163
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
RafaelGSS pushed a commit to nodejs/node that referenced this pull request Nov 10, 2022
Recent changes to node-core-utils attempt to read pull request labels
when deciding whether or not to start a V8 CI. This requires a valid
GitHub token with appropriate permissions to be set for node-core-utils.

Refs: nodejs/node-core-utils#652
PR-URL: #45185
Fixes: #45163
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Dec 30, 2022
Recent changes to node-core-utils attempt to read pull request labels
when deciding whether or not to start a V8 CI. This requires a valid
GitHub token with appropriate permissions to be set for node-core-utils.

Refs: nodejs/node-core-utils#652
PR-URL: #45185
Fixes: #45163
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Dec 30, 2022
Recent changes to node-core-utils attempt to read pull request labels
when deciding whether or not to start a V8 CI. This requires a valid
GitHub token with appropriate permissions to be set for node-core-utils.

Refs: nodejs/node-core-utils#652
PR-URL: #45185
Fixes: #45163
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Jan 3, 2023
Recent changes to node-core-utils attempt to read pull request labels
when deciding whether or not to start a V8 CI. This requires a valid
GitHub token with appropriate permissions to be set for node-core-utils.

Refs: nodejs/node-core-utils#652
PR-URL: #45185
Fixes: #45163
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] ncu-ci run support v8 engine label
4 participants