[BUG] GITHUB_URL is magic in Github-hosted-runners, but shouldn't be used for API Calls

[fbartho]

Describe the bug

We recently switched from Github-hosted Actions runners to Custom Runners because our CI minutes were getting too expensive, and hosting our own runners solved that problem.

But we ran into one last problem: some API calls were being sent to https://github.com/<API_PATH> -- but these API Paths are invalid when you use Octokit from outside of Github.

Workaround: set DANGER_GITHUB_API_BASE_URL: "https://api.github.com"

We verified that our environment variables had been the same in both locations, so my hypothesis is that when running in Github Infrastructure, you can make API calls against github.com instead of being required to use api.github.com

Looking at danger-js' code, I find a handful of places where it's using `|| process.env["GITHUB_URL"]` as a fallback URL for `DANGER_GITHUB_API_BASE_URL` -- when I think it shouldn't?

7 results - 5 files

CHANGELOG.md:
  1722  - Updated TypeScript and Jest dependencies - [@orta]
  1723: - Add support for Github Enterprise via DANGER_GITHUB_API_BASE_URL env var - mashbourne
  1724  

source/platforms/github/customGitHubRequire.ts:
  63    const containsBase = path.startsWith("http")
  64:   const baseUrl = process.env["DANGER_GITHUB_API_BASE_URL"] || process.env["GITHUB_URL"] || "https://api.github.com"
  65    const URLPath = `repos/${repoSlug}/contents/${path}${refString}`

source/platforms/github/GitHubAPI.ts:
   47  
   48:     const host = process.env["DANGER_GITHUB_API_BASE_URL"] || process.env["GITHUB_URL"] || undefined
   49      const options: ConstructorParameters<typeof GitHubNodeAPI>[0] & { debug: boolean } = {

  451      const containsBase = path.startsWith("http")
  452:     const baseUrl = process.env["DANGER_GITHUB_API_BASE_URL"] || process.env["GITHUB_URL"] || "https://api.github.com"
  453      const url = containsBase ? path : `${baseUrl}/${path}`

source/platforms/github/comms/checks/githubAppSupport.ts:
  22  const requestAccessTokenForInstallation = (appID: string, installationID: number, key: string) => {
  23:   const apiUrl = process.env["DANGER_GITHUB_API_BASE_URL"]
  24:     ? process.env["DANGER_GITHUB_API_BASE_URL"]
  25      : "https://api.github.com"

source/runner/dslGenerator.ts:
  41          additionalHeaders: {},
  42:         baseURL: process.env["DANGER_GITHUB_API_BASE_URL"] || process.env["GITHUB_URL"] || undefined,
  43        },

To Reproduce
Steps to reproduce the behavior:

1. Setup Github Custom Runners
2. Run DangerJS in Github Custom Runners
3. Notice the immediate API call failures

Expected behavior
DangerJS should work equally well in Github Actions Runners (on Github Infrastructure) and on Custom Runners.

Your Environment

software	version
danger.js	11.2.4
node	16, 18
npm	8.19.2 (yarn used)
yarn	1.22.19
Operating System	ubuntu-latest

Additional context
Add any other context about the problem here.

[fbartho]

My recommendation:

Change all the places in this general pattern:

- process.env["DANGER_GITHUB_API_BASE_URL"] || process.env["GITHUB_URL"] || "https://api.github.com" 
+ process.env["DANGER_GITHUB_API_BASE_URL"] || "https://api.github.com"

- process.env["DANGER_GITHUB_API_BASE_URL"] || process.env["GITHUB_URL"] || undefined 
+ process.env["DANGER_GITHUB_API_BASE_URL"] || "https://api.github.com"

Any objections? I'm happy to submit this change.

[orta]

I've never had an enterprise account, so it was/is mostly guesswork - this does look like a breaking change though?

[fbartho]

@orta While this is a paid account, it's not a "Github Enterprise" account issue. It is, however, when you want to host GitHub runners on your own machines which is very enterprise-y.

I think technically it's a breaking change, but it would only break some hypothetical environment that doesn't behave like Github-Public.
I guess I can't test what the Github Enterprise / self-hosted experience behaves like.

When you're not on one of github-public's hosted runners you can't make github-api calls against https://github.com so to me this is probably a safe change.

[orta]

Agree

[jbiers]

Today I ran into the same issue as @fbartho mentions in this thread. Is it ok for me to open a PR with the changes mentioned above to solve this issue? @orta