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

Add ncu-ci resume <prid> command #642

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MoLow
Copy link
Member

@MoLow MoLow 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
Member Author

MoLow commented Jul 19, 2022

CC @richardlau @nodejs/node-core-utils

@codecov
Copy link

codecov bot 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 Files Coverage Δ
lib/ci/ci_type_parser.js 90.47% <42.85%> (-1.65%) ⬇️
lib/ci/jenkins_constants.js 100.00% <100.00%> (ø)
lib/ci/run_ci.js 100.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.

@MoLow MoLow changed the title Add ncu-cu resume <prid> command Add ncu-ci resume <prid> command Jul 19, 2022
@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/43919
Error: [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
Member Author

MoLow commented Jul 21, 2022

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

@aduh95 aduh95 requested a review from targos July 21, 2022 08:34
@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

aduh95 commented Feb 23, 2023

There are a few conflicts to address here.

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

4 participants