Skip to content

Conversation

@MoLow
Copy link
Member

@MoLowMoLow commented Jul 19, 2022

refs: nodejs/node#40817
refs: nodejs/node#42125

I was able to test most of the flow except for the actual resume part - since my user dose not have resume permissions
, but this resume url worked on a local jenkins I use - if any reviewer can check the entire flow for me it will be great

@MoLow
Copy link
MemberAuthor

MoLow commented Jul 19, 2022

CC @richardlau @nodejs/node-core-utils

@codecov
Copy link

codecovbot commented Jul 19, 2022

Codecov Report

Merging #642 (acfb36b) into main (bbad9a0) will increase coverage by 0.14%.
The diff coverage is 94.11%.

❗ Current head acfb36b differs from pull request most recent head ed06933. Consider uploading reports for the commit ed06933 to get more accurate results

@@ Coverage Diff @@## main #642 +/- ## ========================================== + Coverage 84.09% 84.24% +0.14%  ========================================== Files 37 37 Lines 4074 4138 +64 ========================================== + Hits 3426 3486 +60 - Misses 648 652 +4 
Impacted FilesCoverage Δ
lib/ci/ci_type_parser.js90.47% <42.85%> (-1.65%)⬇️
lib/ci/jenkins_constants.js100.00% <100.00%> (ø)
lib/ci/run_ci.js100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbad9a0...ed06933. Read the comment docs.

@MoLowMoLow changed the title Add ncu-cu resume <prid> commandAdd ncu-ci resume <prid> commandJul 19, 2022
@MoLowMoLowforce-pushed the add-ci-resume-command branch from 6957918 to 547c5a8CompareJuly 19, 2022 16:19
@MoLowMoLowforce-pushed the add-ci-resume-command branch from acfb36b to ed06933CompareJuly 19, 2022 17:48
@aduh95
Copy link
Contributor

aduh95 commented Jul 21, 2022

So I tried this branch and got the following errors:

$ ./bin/ncu-ci.js resume https://github.com/nodejs/node/pull/43919 ✖ GitHub repository is missing, please set it via ncu-config or pass it via the --repo option ✖ GitHub owner is missing, please set it via ncu-config or pass it via the --owner option $ ./bin/ncu-ci.js resume --repo node --owner nodejs https://github.com/nodejs/node/pull/43919Error: [undefined] GraphQL request Error: Variable $prid of type Int! was provided invalid value at Request.query (file://…/lib/request.js:112:19) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at async Request.gql (file://…/lib/request.js:69:22) at async PRData.getPR (file://…/lib/pr_data.js:80:20) at async Promise.all (index 0) at async JobParser.fromPRId (file://…/lib/ci/ci_type_parser.js:194:3) at async ResumePRJobCommand.start (file://…/bin/ncu-ci.js:319:20){ data:{variables:{prid: NaN, owner: 'nodejs', repo: 'node' } }}

The following works though, and produces the expected result (nodejs/node#43919 (comment)):

$ ./bin/ncu-ci.js resume --repo node --owner nodejs 43919✔ Jenkins credentials valid✔ Build data downloaded✔ PR CI job successfully resumed

@MoLow
Copy link
MemberAuthor

@aduh95 that is expected the ncu-ci resume command expects a pull-request id , not a pull-request URL, just like ncu-ci run

@aduh95aduh95 requested a review from targosJuly 21, 2022 08:34
@MoLowMoLow mentioned this pull request Jul 21, 2022
@tniessen
Copy link
Member

This should not say Fixes: https://github.com/nodejs/node/issues/40817 unless the referenced issue (nodejs/node#40817) should actually be closed once this PR is merged.

@aduh95
Copy link
Contributor

There are a few conflicts to address here.

@targostargos removed their request for review July 11, 2023 14:14
Sign up for freeto 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.

4 participants

@MoLow@aduh95@tniessen@richardlau