Use the API URL from the environment if it is present. - #1175
Merged
chrisgavin merged 3 commits intoAug 11, 2022
Merged
chrisgavin merged 3 commits into
chrisgavin merged 3 commits into
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
chrisgavin
force-pushed
the
local
branch
7 times, most recently
from
9a56f1a to
d8a086d
Compare
August 11, 2022 07:01
Copy link
Copy Markdown
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. |
Sorry, something went wrong.
aeisenberg
approved these changes
Aug 11, 2022
aeisenberg
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Comment thread
src/api-client.ts
Outdated
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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