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

Make CLAHub compatible with other Commit Status tools #27

Closed
jasonm opened this issue Jan 27, 2013 · 34 comments
Closed

Make CLAHub compatible with other Commit Status tools #27

jasonm opened this issue Jan 27, 2013 · 34 comments

Comments

@jasonm
Copy link
Member

jasonm commented Jan 27, 2013

If e.g. travis-ci and clahub both update the status for a commit, only the last one is displayed.

A few options:

  • Provide alternative status indication methods; e.g. commenting on the commit instead of commit status.
  • Providing a flexible status indication API: e.g. you subscribe to a CLAHub webhook which emits new commit statuses.

CC @scottgonzalez re https://twitter.com/scott_gonzalez/status/294643971655884803

@scottgonzalez
Copy link

If there was an API for querying the list of signatures, that should be enough to tie into any other tool that is setting the commit status, assuming the developers have the ability to control the other tool. For jQuery, we're using mergeatron and we can just add the CLA check there.

@jasonm
Copy link
Member Author

jasonm commented Jan 28, 2013

We only display the most recent status for any given commit in our UI.

-- https://github.com/blog/1227-commit-status-api

@jasonm
Copy link
Member Author

jasonm commented Jan 28, 2013

Curious about any precedents https://twitter.com/jayunit/status/295714857611821056

@jasonm
Copy link
Member Author

jasonm commented Feb 17, 2013

@jamesarosen we chatted last night about this, so ping ping :)

@jasonm
Copy link
Member Author

jasonm commented Feb 17, 2013

Also ping @calebthompson from our Twitter conversation - looks like it's totally possible to build a status aggregation service that subscribes to the "status" repo webhook, reads all the existing statuses for a commit, and re-emits an aggregated status:

http://developer.github.com/v3/repos/hooks/
http://developer.github.com/v3/repos/statuses/

@jasonm
Copy link
Member Author

jasonm commented Feb 17, 2013

FWIW, asked joshk on #travis-ci, they're not actively working on this or know of someone who is.

@calebhearth
Copy link

I think an aggregator would be a pretty good thing.

@jasonm
Copy link
Member Author

jasonm commented Feb 19, 2013

Proof of concept is done: http://statusrollup.herokuapp.com/ - please don't lean on it too heavily, it might break sometimes, and I really need to background job some things.

@jamesarosen
Copy link

Currently, GitHub offers only very coarse-grained OAuth access. This means that any app that needs to post status messages to a pull-request needs write access to my full GitHub account, which is pretty worrying. GitHub should definitely offer OAuth access just for writing status messages and commenting on things.

In the meantime, something like StatusRollup could reduce the surface area by being the one thing that gets write access. Of course, that makes it an even better target for attack.

@calebhearth
Copy link

I haven't looked at the code, but I do know that based on Jason and my conversation the plan was to grab the annotations on the commits from GitHub and post a new aggregate annotation.

That means that, for example, Travis, Code Climate, Jenkins, etc. all get access to the repo as well as status rollup, not rather than.

Caleb Thompson
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Tuesday, February 19, 2013 at 10:39 AM, James Rosen wrote:

Currently, GitHub offers only very coarse-grained OAuth access. This means that any app that needs to post status messages to a pull-request needs write access to my full GitHub account, which is pretty worrying. GitHub should definitely offer OAuth access just for writing status messages and commenting on things.
In the meantime, something like StatusRollup could reduce the surface area by being the one thing that gets write access. Of course, that makes it an even better target for attack.


Reply to this email directly or view it on GitHub (#27 (comment)).

@jamesarosen
Copy link

In that case, we're purely expanding the list of things that have write access to people's accounts, which many will balk at :/

I'd really like to hear someone from @github chime in about their work in this area.

@jasonm
Copy link
Member Author

jasonm commented Feb 19, 2013

grab the annotations on the commits from GitHub and post a new aggregate annotation.

Yes, that's what it does: commit_status.rb, status_roller_upper.rb.

I agree on reducing the OAuth surface area. See related: #17. A public_repo:status OAuth scope would be perfect, I think.

@jasonm
Copy link
Member Author

jasonm commented Feb 19, 2013

grab the annotations on the commits from GitHub and post a new aggregate annotation.

Yup, that's how it works - status_roller_upper.rb and commit_status.rb

I agree on reducing the OAuth surface - see related #17. I think public_repo:status would be the ideal scope, though it doesn't currently exist.

we're purely expanding the list of things that have write access to people's accounts, which many will balk a

Rightly so. It bears mentioning that could run all of these tools on your own infrastructure if you so choose, but of course that's super less-than-ideal.

@scottgonzalez
Copy link

It bears mentioning that could run all of these tools on your own infrastructure if you so choose, but of course that's super less-than-ideal.

Instead of having a status rollup service which looks at the other statuses, the service could instead provide hooks into other services that are currently setting the status. For example, it could hook directly into a CLAHub service to find out if there is a CLA on file, it could hook directly into Travis to find out if the commit passed tests, etc. Then the rollup would be the only thing that has access to the repo. This would require a lot more work for the service, but there are definitely other solutions than trying to set multiple statuses when the status API isn't designed for it.

Also, for jQuery we actually would run the tools on our own infrastructure and would want to prevent CLAHub from setting the commit status directly.

@jasonm
Copy link
Member Author

jasonm commented Apr 6, 2014

  • Provide alternative status indication methods; e.g. commenting on the commit instead of commit status.

This would likely be much simpler, for GH integration.

CLAHub itself could also offer webhooks for status, so people could hook it into their own systems.

@ClayShentrup
Copy link

++

@jasonm
Copy link
Member Author

jasonm commented May 20, 2014

Looks like Combined Status API as referenced in #62 is what we want:

https://developer.github.com/changes/2014-03-27-combined-status-api/

@ClayShentrup
Copy link

@jasonm Boom! Thanks.

@Floppy
Copy link
Contributor

Floppy commented Jun 16, 2014

+1, this would be great. Currently, CLAHub stomps on my other build status notifications.

@mitar
Copy link

mitar commented Aug 14, 2014

Any progress on this?

@Floppy
Copy link
Contributor

Floppy commented Aug 17, 2014

I've made what I think is the right change and done a PR. See what you think.

@jasonm
Copy link
Member Author

jasonm commented Aug 18, 2014

I've merged @Floppy 's PR #71 and verified here: https://api.github.com/repos/jasonm/clahub-test/commits/468e33b/statuses

@jasonm jasonm closed this as completed Aug 18, 2014
@Floppy
Copy link
Contributor

Floppy commented Aug 18, 2014

Nice! Looks like it's working!

@jasonm
Copy link
Member Author

jasonm commented Aug 18, 2014

I'm not sure the GitHub UI will show status per-context, even if the state differs. It does appear, however, that it will show the details for the failing context, e.g. jasonm/clahub-test#3

@jasonm
Copy link
Member Author

jasonm commented Aug 18, 2014

So given a commit with two successful statuses, we won't necessarily see "The Travis CI build passed, and all contributors signed the CLA", but rather the description for one of the successful statuses, likely last-writer-wins.

@mitar
Copy link

mitar commented Aug 18, 2014

But when one fail, then it is always shown? So it is not that last-writer-wins also for failed statuses and that a successful status could override the failed one?

@Floppy
Copy link
Contributor

Floppy commented Aug 18, 2014

I'm assuming there will be some update to the github UI at some point to display this stuff properly, but that's just a guess.

@mitar
Copy link

mitar commented Aug 19, 2014

I wrote to GitHub and this is their answer:

Also, just to clarify -- the combined status API doesn't have a UI representation on github.com currently. In other words, statuses shown for commits and pull requests are currently the latest statuses created for commits (we don't show multiple statuses or use the combined status). Showing multiple statuses or using a combined status in the Web UI is something we're considering, but I can't make any promises about if/when it might be available.

@Floppy
Copy link
Contributor

Floppy commented Aug 21, 2014

This is where I wish github was open source :(

@mitar
Copy link

mitar commented Aug 21, 2014

Does GitLab has such API and hooks?

@JamesMGreene
Copy link

@Floppy

This is where I wish github was open source :(

Oh so much. 😕

@Floppy
Copy link
Contributor

Floppy commented Dec 8, 2014

OMG it's finally here people, and it's great. See openpolitics/manifesto#245 for an example :)

@jasonm
Copy link
Member Author

jasonm commented Dec 9, 2014

!!!!!!!

@jasonm
Copy link
Member Author

jasonm commented Dec 9, 2014

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

No branches or pull requests

8 participants