Skip to content

internal/extsvc/github: Return httpResponseState from doRequest

Warren Gifford requested to merge ig/github-dorequest-refactor into main

Created by: indradhanush

Patch related to #25904. Pre-requisite to #31275.

In this commit, we change the internal API doRequest to return a new custom type httpResponseState which contains both the http.Response.StatusCode and the http.Response.Header.

👉 We want to support conditional requests[0] in the GitHub client. To do this, we need access to the response status code, which is a 304 if the resource has not been modified since the last time this resource was accessed. Conditional requests do not count against the API rate limits and will helps us alleviate stress on the GitHub API for resources that do not change very frequently most of the time. This helps us get away with not having to implement caching of the corresponding GitHub resources on our end and instead rely on these conditional requests. In the current design, the status code is not propagated to the callers of doRequest (apart from HTTP request errors). As a result the caller has no visibility for a conditional request.

👍 The positive side effects of this API change are that we are able to get rid of the following thin wrapper functions and make it easier to navigate the call stack when reading the GitHub client's code:

  • requestGet
  • requestGetWithHeader
  • requestBody

👎 The negative side effect of this API change is:

  • The doRequest returns an APIError object which already contains the HTTP StatusCode in case the HTTP request errored out. And with this method now also returning the http.Response.StatusCode as well, this feels somewhat redundant and could possibly be cleaned up further. However, external callers rely on the APIError object at the moment, so refactoring this increases the patch size with little gains.

Work to introduce conditional request support will follow in a successive PR #31275.

[0]: https://docs.github.com/en/rest/overview/resources-in-the-rest-api#conditional-requests

Test Plan

Internal API design change only. No functionality change. Tested locally that sg start works successfully.

Merge request reports

Loading