Use a single Octokit client for everything rather than a bunch of Octokits and an HTTP client. #82
Conversation
…okits and an HTTP client.
| baseUrl: githubAPIURL, | ||
| userAgent: "CodeQL Action", | ||
| log: consoleLogLevel({ level: "debug" }) | ||
| }); |
There was a problem hiding this comment.
Eventually could we make a class that wraps octokit.Octokit and supplies higher level functions a-la octokit?
For example
octokit.repos.codeScanning.uploadAnalysis({
owner,
repo,
analysis
});There was a problem hiding this comment.
Or hopefully once we're fully out of beta then we can have our methods in the proper Octokit.
There was a problem hiding this comment.
Yep. Octokit actually does have methods to add custom endpoints which I did experiment with, but it unfortunately adds typing-related complications. Ideally these methods are eventually part of the real API.
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Couple of points, but generally this looks like a big improvement in standardisation to me.
| import * as octokit from "@octokit/rest"; | ||
| import consoleLogLevel from "console-log-level"; | ||
|
|
||
| const githubAPIURL = process.env["GITHUB_API_URL"] || "https://api.github.com"; |
There was a problem hiding this comment.
Do you have any public documentation for this env var? It doesn't seem to exist on dotcom actions, but maybe it does for enterprise actions. Otherwise I was going to suggest always requiring it to exist.
There was a problem hiding this comment.
Hmm, in a test job I did it existed on both. e.g. See https://github.com/chrisgavin/test-actions/runs/803393860?check_suite_focus=true.
I'm happy to require it exists, though I experienced some very strange issues when trying to use util.getRequiredEnvParam(...) here. Specifically it seems like the Action fails at runtime with the error getRequiredEnvParam is not a function. I wondered if it could be due to an import loop or something but I never managed to figure it out.
| import consoleLogLevel from "console-log-level"; | ||
|
|
||
| const githubAPIURL = process.env["GITHUB_API_URL"] || "https://api.github.com"; | ||
| export const client = new octokit.Octokit({ |
There was a problem hiding this comment.
I think this makes sense as a const, however I am worried that https://github.com/github/codeql-action/blob/main/queries/undeclared-action-input.ql won't understand this. This shouldn't be impossible to fix, but maybe also not trivial.
For now every action does declare the token input, but it would be nice to have it be checked. I am happy for us to note this and fix it in a later pull request.
src/upload-lib.ts
Outdated
| // On any other status code that's not 5xx mark the upload as failed | ||
| if (!statusCode || statusCode < 500 || statusCode >= 600) { | ||
| core.setFailed('Upload failed (' + requestID + '): (' + statusCode + ') ' + await res.readBody()); | ||
| core.setFailed('Upload failed (' + requestID + '): (' + statusCode + ') ' + response.data); |
There was a problem hiding this comment.
I think response.data is an object, so you might want to JSON.stringify here. Also applies to other times that the data is appended to a string.
|
I think a couple of places that making using custom requests could use built-in octokit methods, but making that change at a later date sounds good to me. |
100%. I was actually pretty surprised we didn't already use built-in methods for things like getting the languages of a repository. |
Currently our requests to the GitHub API are pretty inconsistent. In a few places we use an Octokit, but we construct them at the point of use potentially leading to them being initialized inconsistently. We also sometimes just use an HTTP client.
Perhaps most importantly, none of these methods actually respect the
GITHUB_API_URLenvironment variable that tells you where to find the GitHub API.Because of this I've replaced these custom Octokits and the HTTP client with a single Octokit to ensure that all requests are made consistently and to the correct host.
Merge / deployment checklist