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

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

Merged
merged 1 commit into from
Jul 23, 2020
Merged

Conversation

mmarchini
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 27, 2020

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.

lib/ci/run_ci.js Outdated Show resolved Hide resolved
lib/ci/run_ci.js Outdated Show resolved Hide resolved
lib/ci/run_ci.js Outdated Show resolved Hide resolved
@mmarchini
Copy link
Contributor Author

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

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).

bin/ncu-ci Outdated Show resolved Hide resolved
@mmarchini
Copy link
Contributor Author

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

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

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

Trott commented Jul 10, 2020

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

@mmarchini
Copy link
Contributor Author

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

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!

@codebytere codebytere merged commit 332576a into nodejs:master Jul 23, 2020
@mmarchini
Copy link
Contributor Author

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

@nodejs/node-core-utils

@codebytere
Copy link
Member

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

@mmarchini
Copy link
Contributor Author

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.

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.

5 participants