Potentially fix timeouts/retries when fetching from remote #304
Conversation
|
👋 Is there a mechanism (even if very dumb/simple) to try and test this? |
|
@paulo-ferraz-oliveira Looks like GH can just pull directly from the branch and everything should work. So try instead of using |
src/setup-beam.js
Outdated
| if (contentType.indexOf('application/json') !== -1) { | ||
| return response.json() | ||
| } else { | ||
| return response.text() | ||
| } |
There was a problem hiding this comment.
It looks like the automatic JSON decoding was the only real feature we needed.
src/setup-beam.js
Outdated
| if (!response.ok) { | ||
| throw new Error(`Got ${response.statusCode} from ${url}`) | ||
| } |
There was a problem hiding this comment.
Just checking if the response is ok or not. We can get more detailed with status code checking if we need, but I don't see a reason.
There was a problem hiding this comment.
If we weren't getting more detail no more detail is required, at this moment. There's been requests to include more detail around errors, but that's for a different pull request.
src/setup-beam.js
Outdated
| } | ||
| async function getUrlResponse(url, headers, attempt = 1) { | ||
| try { | ||
| const response = await fetch(url, { headers }) |
There was a problem hiding this comment.
Fetch will throw an error on timeout, so that's why we are wrapping. Open to other options / ideas to make this cleaner.
There was a problem hiding this comment.
Try-catch should be Ok here. This is not a huge script, and the handling is isolated.
|
It'll work if CI passes, I'm not worried about that bit. I was thinking of what we could do to have reliable repeatable tests. |
|
@paulo-ferraz-oliveira Added a test using the built in node server. Verified it is working as intended locally. I added a 10 second timeout to fetch calls, though the original code had a 3 minute socket timeout by default which seems very long. |
paulo-ferraz-oliveira
requested review from
wojtekmach,
ericmj and
starbelly
|
D'you wanna be a pal and remove the failing test from CI? It happens because we're targetting OTP 24 and And would you also update If not, I can do both, but you'd have to rebase later... 👍 |
|
Looks like something is installing rebar3 and the action does not like that. I'd say it's another issue that should be addressed in another PR. Probably something about force overwriting files when installing. |
|
Thank you! I'll fix the test failure on main. We have this line for rebar: fs.mkdirSync(bindir)and we're doing it twice on CI, once for fs.mkdirSync(bindir, { recursive: true }) |
paulo-ferraz-oliveira
changed the title
|
Released in v1, v1.18 and v1.18.2. Thanks. |
Description
This action uses node 20, which now has the built in
fetchimplementation. We can use that instead of the@action/http-clientpackage. This allows us to do our own retry implementation to fix the long standing issue #260.Closes issue: #260