github / janky

Continuous integration server built on top of Jenkins and Hubot
MIT License
2.76k stars 237 forks source link

Send a Context to Github with Janky Status Updates #195

Closed Mezzle closed 10 years ago

Mezzle commented 10 years ago

This will send a "context" variable with status updates to Github.

It will send "ci/janky" by default, but can be configured with the JANKY_GITHUB_STATUS_CONTEXT environment variable.

parkr commented 10 years ago

What's the use-case for this?

Mezzle commented 10 years ago

The use case is based around the new combined status API.

If you provide a context in your status updates, then github will treat it as a different status, for example, I might want to send through a status that the tests Pass/Fail, I might want to send through a status that the code coverage is above/below our limits.

Previously, I'd have to send these results to somewhere that would combine them and send them as a single result to Github, but now, I can send them through with different contexts, and get an "overall" status, aswell as the statuses of the different components.

See https://developer.github.com/changes/2014-03-27-combined-status-api/ and https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref

bhuga commented 10 years ago

We've been using "janky/#{build.repo_name}" for some projects that have multiple builds for the same repo. How do you feel about making this "ci/janky/#{build.repo_name}"?

atmos commented 10 years ago

@bhuga I'd like the defaults to not even check the repo name if possible.

Mezzle commented 10 years ago

Having had this in production for about half an hour, I've spotted that if we send a context, Github currently won't display a status next to the commits on their web interface (I presume this is due to lack of dogfooding this particular feature), which also means we don't get the status on PRs.

I'm going to close this PR for now, and change it so that it has a "default" of not sending a context, and it'll only send a context if there is one specified in the environment.

Mezzle commented 10 years ago

@bhuga How would that help? If it's different builds for the same repo, adding the repo name won't differentiate?

If it's different contexts for the same repo, then these are surely different instances of janky? All you need to do then is name the context, it shouldn't need to know about the repo name.

rsanheim commented 10 years ago

Having had this in production for about half an hour, I've spotted that if we send a context, Github currently won't display a status next to the commits on their web interface (I presume this is due to lack of dogfooding this particular feature), which also means we don't get the status on PRs.

Where are you seeing this? This sounds like a bug or oversight. Pretty sure we are using combined statuses, and we see statuses in the UI, right @bhuga ?

Mezzle commented 10 years ago

In a private report.

Is it not possible that the combined status display is only viewable by githubbers? On 17 Apr 2014 16:52, "Rob Sanheim" notifications@github.com wrote:

Having had this in production for about half an hour, I've spotted that if we send a context, Github currently won't display a status next to the commits on their web interface (I presume this is due to lack of dogfooding this particular feature), which also means we don't get the status on PRs.

Where are you seeing this? This sounds like a bug or oversight. Pretty sure we are using combined statuses, and we see statuses in the UI, right @bhugahttps://github.com/bhuga?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email?

— Reply to this email directly or view it on GitHubhttps://github.com/github/janky/pull/195?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email#issuecomment-40729710 .

Mezzle commented 10 years ago

Give me a moment, I'll change up the code and retest

Mezzle commented 10 years ago

Figured it out - needs to have the Accept header to send context (which isn't documented)

Mezzle commented 10 years ago

Ok, I've cleaned up the code and it should now be working :)

Mezzle commented 10 years ago

Urgh... This isn't showing the proper diff...