Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the API URL from the environment if it is present. #1175

Merged
merged 3 commits into from
Aug 11, 2022
Merged

Conversation

chrisgavin
Copy link
Contributor

@chrisgavin chrisgavin commented Aug 10, 2022

Actions runners export a GITHUB_API_URL environment variable (and have done for around two years now). If we're running on Actions, it's more reliable to use that rather than trying to calculate the API URL ourselves.

It also means the Action then works on non-DotCom environments where the API URL doesn't start /api/v3.

On GitHub Actions we can assume this URL will always be available, though unfortunately we have to keep the code for deriving the API URL from the primary URL since this is still needed in the runner.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@chrisgavin chrisgavin force-pushed the local branch 7 times, most recently from 9a56f1a to d8a086d Compare August 11, 2022 07:01
@chrisgavin chrisgavin marked this pull request as ready for review August 11, 2022 08:07
@chrisgavin chrisgavin requested a review from a team as a code owner August 11, 2022 08:07
@aeisenberg
Copy link
Contributor

The runner is deprecated. The only reason why we still have the code around is because some of our integration tests rely on it. Unless the integration tests are failing, I don't think we need to keep the old code paths around.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion to make the code more idiomatic, but this is not necessary.

If all integration tests can pass with deriveApiUrl removed, then it can be deleted. Otherwise, we will need to wait until we remove the runner tech debt.

Approving now so you can finish this off one way or the other tomorrow while I am asleep.

src/api-client.ts Outdated Show resolved Hide resolved
chrisgavin and others added 2 commits August 11, 2022 17:10
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants