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

Report Tox Envs to GitHub #73

Closed
RonnyPfannschmidt opened this issue Aug 13, 2017 · 10 comments
Closed

Report Tox Envs to GitHub #73

RonnyPfannschmidt opened this issue Aug 13, 2017 · 10 comments

Comments

@RonnyPfannschmidt
Copy link

for environments like documentation checks or lining it would be really neat, if there was a way to report their failure/success as github status instead of failing the complete ci build

this would help to quickly identify broke the world vs missed a space

@ryanhiebert
Copy link
Collaborator

I've often wished that Travis had this feature natively, to report to GitHub statuses on each Job, with enough context to make this easy.

I'd be interested in a feature like that for Tox-Travis to build on top of Travis. I'm not sure when I'll have the time to get to it, but if there's anyone interested in making it happen, I'll make myself available for questions, discussion, and code review.

@ryanhiebert ryanhiebert changed the title support for reporting toxenvs to github instead offailing the build Report Tox Envs to GitHub Oct 17, 2017
@ryanhiebert
Copy link
Collaborator

The implication in this issue seems to be that we would not want to fail the Travis build for an env failure. I think that would be a mistake. I don't want to change the semantics of Tox that way.

I do find the idea of reporting individual Tox envs, even further than just Travis jobs, really compelling. Thank you for the suggestion, @RonnyPfannschmidt . This is a feature I would very much like to see happen.

@RonnyPfannschmidt
Copy link
Author

thanks for pointing this detail out, its indeed not sensible to let the "build" pass when there are failed parts

@ryanhiebert
Copy link
Collaborator

The hooks that we'll want to use for this are:

  • tox_runtest_pre(venv)
  • tox_runtest_post(venv)

@ryanhiebert
Copy link
Collaborator

ryanhiebert commented Nov 13, 2017

As far as I can tell, the new GraphQL API for github (v4) does not (yet) have a way to add statuses to commits. It seems that will require v3 of the REST API instead.

https://developer.github.com/v3/repos/statuses/

@ryanhiebert
Copy link
Collaborator

I'd really appreciate some feedback on this question. I've been playing around with the GitHub API, and working up what it would take to get it working on Travis. I've run into a pretty major hurdle.

This feature is obviously intended in large part for pull requests. However, secure environment variables are not available on pull request builds unless they are on the same repository, which makes it not useful for testing outside contributions.

I see two options:

  1. Live with the limitation.
  2. Put the GitHub token (limited to repo statuses) in an insecure env var.

Neither 1 nor 2 seem like a good option either. Allowing a GitHub token to be in cleartext seems like a recipe for disaster, and the limitation of secure env vars really hamstrings the idea, to the point of it not really even being worth it, it seems to me.

I even wondered if it would be possible to create a 3rd party service to act as an intermediary, that would store the GitHub tokens, and manage the calls coming from Travis. Unfortunately, I can't figure out how that service would be secured either, and it just ends up moving the problem to another layer.

At this point, I'm kinda out of ideas. I really thought this was a great idea, but I'm having trouble figuring out a way that I can reasonably implement this. I would be thrilled to have someone prove that I was missing something obvious, but at this moment it feels like it's probably going to be best to abandon the idea, unfortunately.

Do you agree with my assessment of the options? Do you see another option that I've missed?

@rpkilby
Copy link
Member

rpkilby commented Nov 19, 2017

This external service sounds like it would have a similar workflow to codecov. Any idea on how that works? Regardless, the amount of effort for this service does not seem like it would be worth it.

@ryanhiebert
Copy link
Collaborator

You're right, there's a lot in common with codecov for this purpose. I'm not sure how codecov validates the uploads it gets from Travis for public repositories (which don't require any secret tokens). Perhaps it doesn't validate those uploads?

Anyway, I've been looking at the Travis issue for it (travis-ci/travis-ci#5035), and one of the first things I saw mentioned there was Travieso, and I'm looking at that to see if there's some seed of inspiration to get from it.

@ryanhiebert
Copy link
Collaborator

I'm waiting on response from codecov, but I suspect that they actually don't authenticate any coverage uploads. They do have uuid-like repository tokens for private repos, but I think that's just for identification rather than really authentication.

This is probably less of a concern for them because (a) there is a limited number of statuses they deal with, and (b) coverage reports don't directly map to statuses, so are more difficult to abuse.

Travieso uses Travis webhooks to authenticate, but they come with their own limitations, most notably that jobs aren't updated in real-time, but rather the summary report is given as a webhook and authenticated with a secret token that isn't sent over the wire, and then all of the statuses are reported to GitHub at once.

@ryanhiebert
Copy link
Collaborator

A possibility for the 3rd party service would be to allow envs to be reported by identification only (by build id, perhaps), but limit it in two ways:

  1. Only allow statuses to be submitted between the build starting and completing. The webhooks are sent authenticated, so they can be verified. Then we've got a window of time for these notifications, which can be opened and closed.
  2. Allow the list of statuses to be defined ahead-of-time. Probably by setting a global environment variable, like STATUSES='py35 py36'. I think this probably should be optional, but it can be a way to further limit the amount of havoc that a bad actor can wreak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants