Proxy Jenkins build status to GitHub #35

phillipj · 2016-04-28T19:58:24Z

This is work in progress atm, PR exists for public scrutiny and to show I've started working on it.

Any thoughts are much appreciated!

Refs #5

Fishrock123 · 2016-04-28T20:08:54Z

Is this page the only official docs on it? https://wiki.jenkins-ci.org/display/JENKINS/Remote+access+API

phillipj · 2016-05-02T20:29:02Z

Made some improvements to this today. My biggest question atm is how we should trigger Jenkins polling in the first place?

I've thought of two different approaches so far:

Does Jenkins have support for notifying the bot with a HTTP request? If so that could be a pretty clean approach.
Listen for PR comments matching "CI: https://ci.nodejs.org/job/node-test-pull-request/X".

Any other thoughts?

/cc @Fishrock123 @jbergstroem

Fishrock123 · 2016-05-02T21:22:17Z

All of my existing ideas have been around suggestion 1. -- by far the most reliable.

jbergstroem · 2016-05-02T22:41:10Z

@phillipj we could implement it [pinging the bot] as a build step. The problem here is security; the receiving end can't use much else than ip/hostname matching for security. If that's acceptable it would likely be the best path forward.

As for methodology; I'd like to suggest the following:

(based on option one) accept the incoming request
inspect what subjobs are being spawned
post a 'pending' request for all subjobs to the PR
wait for subjobs to complete
add a completion task at each job which posts info to bot (again, if trust can be established)
parse info and notify github. I'd really like to see something like:
if test passes, something basic (ci/linux: all tests passed)
if we get flakey/skips, use pass again but mention flakey/skips (ci/linux: tests passed (5 flakey, 5 skips))
if we get fails, post as failure with num fail tests (ci/linux: 2 failed tests)

Also, once we close in on this I reckon we should split linter from the test-pr job and start calling it separately from the gh bot on pretty much every commit, as well as create a set of slaves that does the same for commit message validation.

Fishrock123 · 2016-05-03T15:51:41Z

The problem here is security; the receiving end can't use much else than ip/hostname matching for security.

Can it be over https? Github and the like just send a secret to verify, or the hash of a secret. Would that be possible?

That being said ip/hostname matching probably isn't too bad?

I like the methodology suggestions. @jbergstroem how difficult would it be to make those subjobs so we can start trying this?

jbergstroem · 2016-05-03T21:51:42Z

@Fishrock123 the problem isn't so much man in the middle, but the fact that we can't store credentials at jenkins since a PR could potentially just do echo $FOO. I actually implemented posting progress to github as part of the jenkins jobs but abandoned due to security issues. More info about findings over here nodejs/build#236.

@Fishrock123 by subjobs i was referring to the fact that node-test-pr invokes node-test-commit which in turn invokes node-test-linux and so on. So what we'd do is likely send a request from each (if POST_STATUS_TO_PR is set at top level) at start and completion.

phillipj · 2016-05-04T13:48:52Z

@jbergstroem no problem going for ip/host matching for incoming HTTP requests.

To ensure we're on the same page here: if the bot gets HTTP requests upon subjob start and completion, it will not perform any Jenkins polling at all, but will work more like a proxy for updating github PR (commit) status? ..if so that sounds very good 👍

It would be very helpful if we could get our hands on some example payloads those subjob requests from Jenkins include.

Fishrock123 · 2016-05-04T14:52:53Z

if the bot gets HTTP requests upon subjob start and completion, it will not perform any Jenkins polling at all, but will work more like a proxy for updating github PR (commit) status?

@phillipj correct.

It would be very helpful if we could get our hands on some example payloads those subjob requests from Jenkins include.

Yes. this. :D

jbergstroem · 2016-05-05T01:16:06Z

Example payloads: I can craft them whatever way we want -- will essentially just pass stuff through curl.

phillipj · 2016-05-05T19:28:36Z

@jbergstroem we should come a long way with:

build parameters
subjob name
subjob URL
commit sha
build result (upon completion)

The structure of the payload isn't important, as long as it's JSON. Is it doable?

Fishrock123 · 2016-05-05T19:51:45Z

What do we need build params for?

Fishrock123 · 2016-05-05T19:52:11Z

Imo, we'll need:

name
url
commit sha
status

phillipj · 2016-05-05T20:23:56Z

What do we need build params for?

@Fishrock123 the POST_STATUS_TO_PR option. Should we simply ignore it and report 'em all?

Fishrock123 · 2016-05-05T20:36:07Z

@phillipj I presume that would determine if the bot gets contacted at all or not?

phillipj · 2016-05-05T20:43:53Z

@Fishrock123 ahh that makes sense. Agreed we could do wo/build params then.

jbergstroem · 2016-05-05T20:51:31Z

More a summary of yours, but yeah; this is what I'm thinking:

on start:

the jenkins job will check if POST_STATUS_TO_PR is set or bail
curl to $url/$start_endpoint with a pretty low timeout since this is sync, with:
- job name (we can map job name to something more user friendly at the gihub bot, for instance: node-test-linux -> test/linux)
- url to job
- commit sha
- designated slave

on completion:

the jenkins job will check if POST_STATUS_TO_PR is set or bail
curl to $url/$stop_endpoint
- job name
- url to job
- commit sha
- designated slave
- build results (this will be different between some jobs; for instance linter or commit-message-validator):
  - passed/flaky/fail
  - total tests/flaky/failed
  - time spent

We should poll jobs occasionally slave might go away during job. Not very likely but it happens. Also, we sometimes have ci-related failures. Reporting them as a "ci fail" would be a nice ui improvement.

phillipj · 2016-05-06T22:44:01Z

@jbergstroem @Fishrock123 just did some adjustments based on what we've been discussing here.

I did some assumptions on the structure and made three fixtures:

You can see the result of POSTing those payloads in TestOrgPleaseIgnore/node#13

What's missing from @jbergstroem last comment is use of designated slave and different kinds of build results. It also doesn't use two different endpoints for start / completion, I did see the need for that so far.

..and it would have been nice to get this covered by tests.

test/_fixtures/error-payload.json

@@ -0,0 +1,12 @@
+{
+    "fullDisplayName": "node-test-commit-arm #3087",
+    "result": "FAILURE",


phillipj · 2016-05-07T21:13:15Z

Thanks for reviewing so quick guys!

Mind explaining the rational behind this?

@Fishrock123 it's the same structure as the Jenkins json/api uses. If we're not tied to that structure/format and can do adhoc, I'm more than happy to keep it simpler.

phillipj · 2016-05-07T21:53:13Z

As just mentioned ^ if we're able to use whatever structure/format we want, just ignore the fixtures added here.

@jbergstroem would you be able to post an example of what these curl jobs could provide the bot with?

jbergstroem · 2016-05-08T08:24:22Z

@phillipj jenkins basically has the same information available as what you can get from the api; so I'll just build a query similar to what we expect. I will build different queries based on fail/success/etc.

Fishrock123 · 2016-05-08T14:37:33Z

@jbergstroem How about making it curl an object like this, and we'll go from there?

{
  "info": "something about node-test-commit-linux",
  "status": "FAILURE",
  "commit": "shasum",
  "url": "https://ci.nodejs.org/job/node-test-commit-linux/3176/"
}

jbergstroem · 2016-05-08T22:05:35Z

@Fishrock123 sounds good. in addition to that, we should probably add identifier (linux, linter, etc) and test results. I'll look at my old stuff.

jbergstroem · 2016-05-09T04:00:00Z

How about something like:

$ curl \
  --connect-timeout 5 \
  -X POST \
  -H "Content-Type: application/json" \
  -d "{ \
    \"identifier\": \"linter\", \
    \"status\": \"pending\", \
    \"url\": \"${BUILD_URL}\", \
    \"commit\": \"$(git rev-parse HEAD)\", \
    \"message\": \"checking for errors\" \
  }" \
  https://deepthought.nodejs.org/endpoint/

..and with payload:

curl \
  --connect-timeout 5 \
  -X POST \
  -H "Content-Type: application/json" \
  -d "{ \
    \"identifier\": \"test/linux\", \
    \"status\": \"failure\", \
    \"url\": \"${BUILD_URL}\", \
    \"commit\": \"$(git rev-parse HEAD)\", \
    \"message\": \"tests failed\",
    \"data\": { \
      \"total\": \"12345\", \
      \"pass\": \"12340\", \
      \"fail\": \"3\", \
      \"skip\": \"2\" \
    } \
  }" \
  https://deepthought.nodejs.org/endpoint/

The rationale behind abstracting data is that we could store it and start generating insights over time. I see something similar as part of the other data we collect from github by receiving all the events (briefly mentioned in #4). It's also easier working with the data -- one test failed (5 skipped, 2 flakey) vs 2 tests failed .. than through bash.

phillipj · 2016-05-09T13:43:59Z

@jbergstroem LGTM, I'll update the fixtures tonight. I suggest we leave data out of the picture initially to get this out asap, and add it later in a separate PR which also stores that data somehow.

jbergstroem · 2016-05-09T13:53:45Z

@phillipj that means I'll be formatting the failures from bash then?

phillipj · 2016-05-09T14:02:10Z

@jbergstroem hm... you would prefer the status description (the text displayed to the right of the status) to be a combination of curl's message and a human readable format of data?

Fishrock123 · 2016-05-09T14:07:19Z

@jbergstroem I think you can include the data but we will worry about processing that after we have the initial things up and running?

phillipj · 2016-05-09T18:46:03Z

Just updated the fixtures, and deleted lots of code which ended up giving a lot of flexibility from the curl side of things. It's now very much a proxy to update GH statuses, forwarding whatever it gets as input, rather than trying to do smart transformations.

PTAL and shout if I've missed something from your comments.

TestOrgPleaseIgnore/node#14

jbergstroem · 2016-05-10T04:26:28Z

@Fishrock123 yeah I agree. So, where do I post for now? Also, I'm kind of interested in finding a way of queueing/delegating all the curl requests so they return instantly -- anyone got any ideas?

phillipj · 2016-05-10T07:14:45Z

@Fishrock123 nope, that's for the github repo hooks (used to trigger Travis polling). I'll send the URL to @jbergstroem on IRC as the location of the bot shouldn't be public atm since we don't have a ip/hostname match or other security measures in place.

phillipj · 2016-05-10T14:01:50Z

@Fishrock123 @jbergstroem soooo, I just pushed a commit loading the gun so to speak ^.

ATM the bot gets curl updates from node-test-linter in production, but doesn't do anything since these changes hasn't been merged yet. Are we ready to merge & deploy this as is, or do we need more things before rolling forward?

Fishrock123 · 2016-05-10T14:58:20Z

I've made a test PR at nodejs/node#6674 -- forgot to make this on the repot itself though. LMK if it should open a new one from a node repo branch so you can push to it.

phillipj · 2016-05-10T19:18:21Z

..and now it even has tests ensuring github is requested with the expected payload as well 🎆

phillipj · 2016-05-10T19:39:57Z

@jbergstroem fyi I'm holding this 'til you respond. I could potentially deploy this specific branch instead of merging it to master, if you'd rather do a couple of test runs before we going full throttle ;)

jbergstroem · 2016-05-10T22:14:11Z

I've now implemented check for POST_STATUS_FOR_PR. I'll default that to off for now and start adding status updates to more jobs.

phillipj · 2016-05-11T05:44:54Z

This is now live in production 💥

phillipj added 2 commits May 2, 2016 20:22

First shot at polling Jenkins for PR/build status

15f4b71

Handled errors and mapped more Jenkins build statuses -> GH statuses

55e7fcb

phillipj force-pushed the pull-jenkins branch from f8a5cfe to 55e7fcb Compare May 2, 2016 20:20

First shot at pushing GH update from Jenkins

6bfdafa

Fishrock123 reviewed May 6, 2016
View reviewed changes

test/_fixtures/error-payload.json

@@ -0,0 +1,12 @@

{

"fullDisplayName": "node-test-commit-arm #3087",

"result": "FAILURE",

This comment was marked as off-topic.

Sign in to view

phillipj changed the title ~~WIP: polling Jenkins for build status~~ WIP: proxy Jenkins build status to GitHub May 9, 2016

Simplified fixtures and deleted lots of code

783220e

phillipj force-pushed the pull-jenkins branch from 954e44a to 783220e Compare May 9, 2016 18:47

Push updates to nodejs/node

77f3734

phillipj changed the title ~~WIP: proxy Jenkins build status to GitHub~~ Proxy Jenkins build status to GitHub May 10, 2016

phillipj added 2 commits May 10, 2016 16:32

Print commit sha when logging

7760e69

Merge branch 'master' into pull-jenkins

f342a05

Fishrock123 mentioned this pull request May 10, 2016

test: test commit for nodejs-github-bot nodejs/node#6674

Closed

phillipj force-pushed the pull-jenkins branch from 2e45fdd to d0137f3 Compare May 10, 2016 19:15

Added a couple of tests

41a8208

phillipj force-pushed the pull-jenkins branch from d0137f3 to 41a8208 Compare May 10, 2016 19:19

phillipj merged commit 39f2c49 into master May 11, 2016

phillipj deleted the pull-jenkins branch May 11, 2016 05:41

phillipj mentioned this pull request May 6, 2017

Monitor updates to pr and post Jenkins CI progress #5

Closed

Proxy Jenkins build status to GitHub #35

Proxy Jenkins build status to GitHub #35

Conversation

phillipj commented Apr 28, 2016

Fishrock123 commented Apr 28, 2016

phillipj commented May 2, 2016

Fishrock123 commented May 2, 2016

jbergstroem commented May 2, 2016

Fishrock123 commented May 3, 2016 • edited Loading

jbergstroem commented May 3, 2016

phillipj commented May 4, 2016

Fishrock123 commented May 4, 2016

jbergstroem commented May 5, 2016

phillipj commented May 5, 2016

Fishrock123 commented May 5, 2016

Fishrock123 commented May 5, 2016

phillipj commented May 5, 2016 • edited Loading

Fishrock123 commented May 5, 2016 • edited Loading

phillipj commented May 5, 2016

jbergstroem commented May 5, 2016

on start:

on completion:

phillipj commented May 6, 2016

This comment was marked as off-topic.

phillipj commented May 7, 2016

phillipj commented May 7, 2016 • edited Loading

jbergstroem commented May 8, 2016

Fishrock123 commented May 8, 2016 • edited Loading

jbergstroem commented May 8, 2016

jbergstroem commented May 9, 2016

phillipj commented May 9, 2016

jbergstroem commented May 9, 2016

phillipj commented May 9, 2016

Fishrock123 commented May 9, 2016 • edited Loading

phillipj commented May 9, 2016

jbergstroem commented May 10, 2016

phillipj commented May 10, 2016 • edited Loading

phillipj commented May 10, 2016

Fishrock123 commented May 10, 2016

phillipj commented May 10, 2016

phillipj commented May 10, 2016

jbergstroem commented May 10, 2016

phillipj commented May 11, 2016

Fishrock123 commented May 3, 2016 •

edited

Loading

phillipj commented May 5, 2016 •

edited

Loading

Fishrock123 commented May 5, 2016 •

edited

Loading

phillipj commented May 7, 2016 •

edited

Loading

Fishrock123 commented May 8, 2016 •

edited

Loading

Fishrock123 commented May 9, 2016 •

edited

Loading

phillipj commented May 10, 2016 •

edited

Loading