Inconsistent fetching of commits

martindekov commented 4 years ago

Inconsistent error:

exit status 1
time="2020-07-24T10:05:39Z" level=fatal msg="Error getting commits for PR 44\nGET https://api.github.com/repos/martindekov/push2/pulls/44/commits: 401 Bad credentials []"

This error is here: https://github.com/alexellis/derek/blob/16d7143de95fbc1516be7377b7388995a78b19c0/handler/pullrequest_handler.go#L242

And the function is: https://github.com/alexellis/derek/blob/16d7143de95fbc1516be7377b7388995a78b19c0/handler/pullrequest_handler.go#L235

Things I went through while trying to make the error consistent and it didn't:

[x] Using the same context across all Derek's code for the pull logic as the OAuth2 object has description that the client is valid as long as the context with which it was created is used
- [x] Check rate limits as per description https://docs.github.com/en/rest/reference/rate-limit#understanding-your-rate-limit-status since the DCO feature increases the number of API calls.

Expected Behaviour

Every request for the commits has the same behavior on error or success.

Current Behaviour

Requesting the commits in a PR is inconsistent and might fail.

Possible Solution

In this case we can re-try the request. Possibly if the problem is in the access token we can revisit how we request the token, or fix the error flow of the token.

Steps to Reproduce (for bugs)

Run derek
Start sending requests by opening PRs and adding commits with and without signature
Check that sometimes this error is present and commits cannot be fetched leading to the dco label and checks not being applied properly

Context

This can potentially lead to inconsistency when checking for DCO as if the error is present and derek will fail to list the commits and recognize if there is unsigned one.

Your Environment

[ ] You're using the hosted Derek service

or

[x] You host your own OpenFaaS cluster with Derek installed

Docker version docker version (e.g. Docker 17.0.05 ): N/A
Are you using Docker Swarm or Kubernetes (FaaS-netes)? N/A
Operating System and version (e.g. Linux, Windows, MacOS): N/A

alexellis commented 4 years ago

We didn't have this issue prior to merging the status change for the DCO and the change @Waterdrips made to use the new endpoint for installation tokens.

I would expect one of the above to be related to the regression, as Derek has never shown this error before - neither in local testing or deployed for users.

Can one of you raise a support issue with GitHub please?

https://support.github.com

alexellis commented 4 years ago

Check rate limits as per description https://docs.github.com/en/rest/reference/rate-limit#understanding-your-rate-limit-status since the DCO feature increases the number of API calls.

You have 5000 (up from 50) per hour for properly authenticated requests, this appears to be a misconfiguration? Are there new limits on how the access tokens can be used?

martindekov commented 4 years ago

You have 5000 (up from 50) per hour for properly authenticated requests, this appears to be a misconfiguration? Are there new limits on how the access tokens can be used?

I marked this ^ as not a problem, at least from my testing. We generate new token for every request which has those limits refreshed. I opened a ticket here: https://support.github.com/ticket/personal/0/796320

martindekov commented 4 years ago

https://github.com/martindekov/push2/pull/48

Red is check_suite which is not supported event.

Can't seem to replicate the issue now with alexellis/derek:0.10.0 image

alexellis / derek