search

madjar / nox

Tools to make nix nicer to use
MIT License
308 stars 35 forks source link

Breaks if PR title is empty #29

Closed domenkozar closed 8 years ago

domenkozar commented 8 years ago

https://travis-ci.org/NixOS/nixpkgs/builds/87937406

ctheune commented 8 years ago

This is interesting. I tried to create a PR without a title but Github doesn't allow that. Do you know how the original pull request ended up without a title?

The code seems simple enough to adapt, but I can't test it.

matthewbauer commented 8 years ago

This is still an issue. The fix just hides the issue but the payload still is missing the necessary keys.

https://travis-ci.org/NixOS/nixpkgs/jobs/140989054#L926

I'm starting to think it's Travis that is doing this. There must be some issues with SSL verification? I can't figure out why the request call isn't giving valid JSON output.

madjar commented 8 years ago

Okay, I'll try to reproduce it to get an idea of what's happening. If I can't find anything, I'll fall back to the "log and retry" approach.

madjar commented 8 years ago

Found it :)

I wrote a little script to hammer the API until I have an invalid result, and I didn't have to wait long to get {'documentation_url': 'https://developer.github.com/v3/#rate-limiting', 'message': "API rate limit exceeded for REDACTED_IP. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"}

So I guess we might be hitting the rate limit for travis IP, which explains the transient failures.

I don't know how to fix that without dropping the API calls, because the rate limit can take up to one hour to reset, and we can't use an API token in the pull requests checked by travis.

matthewbauer commented 8 years ago

Perhaps we can add an optional argument to provide a GitHub token? Travis let's use provide "secure" environment variables and we could incorporate that into "travis-nox-review.sh".

madjar commented 8 years ago

Sadly, to quote the travis doc (https://docs.travis-ci.com/user/environment-variables/#Encrypted-Variables)

Encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code.

Since most of our PR are from forks, that wouldn't really help.

matthewbauer commented 8 years ago

Hmmm... that's annoying. I wonder what would happen if we just hardcoded a GitHub token?

I'm still wondering how we're hitting 5000 request per hour rate limit? Each job should only have 1 API request but maybe it's shared with all of the Travis jobs? I'd be interested in seeing if changing the user agent made any difference... But I'm wondering if we're only hitting this issue when Travis has had some downtime and tons of jobs are getting run one after another.

madjar commented 8 years ago

I think the limit without token is 60 requests per hour per IP.

Hardcoding the token to a dummy github account (one without anything important attached to it) might indeed do the trick.

matthewbauer commented 8 years ago

Okay, this is saying that secure environment variables work in pull requests:

https://blog.travis-ci.com/2013-06-10-secure-env-in-pull-requests/

Apparently they just disable it for "pull requests" on to forks of the repo?