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

ncu-ci: command to start CI for PRs #445

Closed mmarchini closed 3 years ago

codecov[bot] commented 3 years ago

Codecov Report

Merging #445 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #445   +/-   ##
=======================================
  Coverage   76.94%   76.94%           
=======================================
  Files          21       21           
  Lines        1501     1501           
=======================================
  Hits         1155     1155           
  Misses        346      346           

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 158efdf...9d2013d. Read the comment docs.

mmarchini commented 3 years ago

Done. I also made some changes so that if we fail to start the CI for any reason, the process exits with status code != 0 :)

mmarchini commented 3 years ago

I think this deserves a @nodejs/build ping to double-check if we're not doing anything we shouldn't in the Jenkins calls (for example, using a user token instead of the token defined in the Job configuration).

mmarchini commented 3 years ago

Added some tests. Would be great if we could include this in the next release, as it is required for the Request CI Queue on nodejs/node :)

codebytere commented 3 years ago

I might not have all the context i might need as i'm not a member of @nodejs/build, but if no one else comments then i think we may be fine to go ahead and merge?

mmarchini commented 3 years ago

That's what I'm thinking, yes. If something with this approach is not right/good, we can always improve on a future version (or remove if needed)

Trott commented 3 years ago

I'm pretty sure @nodejs/jenkins-admins is a subset of @nodejs/build, but uh, pinging them anyway....

mmarchini commented 3 years ago

If folks want to help review but don't want/have time to dabble into ncu code, the main question is: is this the right/best way to start a Jenkins job via API? Below is a simplified version of the flow implemented in the PR:

// this code probably doesn't work, but it shows 
// which calls the PR makes to start a job via 
// Jenkins API

// Use the token generated on Jenkins interface to
// authenticate.
const username = '***';
const personalJenkinsToken = '***';
const auth = Buffer
  .from(`${username}:${personalJenkinsToken}`)
  .toString('base64')
const headers = { Authorization: `Basic ${auth}` };

// Get breadcrumb for CSRF validation
const { crumb } = await (await fetch('https://ci.nodejs.org/crumbIssuer/api/json', { headers }));

// `/job/node-pull-request/build` expects a multipart
// request, so use FormData to generate the payload
const payload = new FormData();
payload.append('json', JSON.stringify({
  parameter: [
    { name: 'CERTIFY_SAFE', value: 'on' },
    { name: 'TARGET_GITHUB_ORG', value: 'nodejs' },
    { name: 'TARGET_REPO_NAME', value: 'node' },
    { name: 'PR_ID', value: 123456 },
    { name: 'REBASE_ONTO', value: '<pr base branch>' },
    { name: 'DESCRIPTION_SETTER_DESCRIPTION', value: '' }
  ]
}));

// Send POST with breadcrump + auth on headers, and 
// FormData payload on body
await fetch('https://ci.nodejs.org/job/node-pull-request/build', {
  method: 'POST',
  headers: {
    'Jenkins-Crumb': crumb,
    ...headers
  },
  body: payload
});
codebytere commented 3 years ago

Going to merge this just so we can move forward with it :) I think if there are any changes to the flow start we can update as needed!

mmarchini commented 3 years ago

Can we get a new release with this included? :)

@nodejs/node-core-utils

codebytere commented 3 years ago

@mmarchini https://github.com/nodejs/node-core-utils/releases/tag/v1.23.0 🌟

mmarchini commented 3 years ago

found a bug which doesn't prevent from using the tool, but if the user doens't have permission to start CI it will show as if everything worked fine. I already have a fix locally and will send a PR soon.