Try to prevent 403 when accessing the GitHub API #168
Conversation
paulo-ferraz-oliveira
changed the title
|
Moving to "Ready to review", to open it to reviewers, but don't merge just yet. I wanna make sure that running CI repeatedly does not surface any issues. (and I'm still missing a self-review) |
|
Side note from one user perspective: with this patch applied, we have not seen any setup-beam 403 issues in our CI piplines, of which we have run many dozen (some using the action in multiple jobs) since applying. I believe the GITHUB_TOKEN usage is the thing that did the trick, so the retries are some kind of extra fallback. If you want to get some testing done on a branch that only makes changes to use the GITHUB_TOKEN I'd be happy to get that tested. Perhaps it can even use the GitHub context to avoid having to thread the secret through the environment an extra time: |
|
@kivra-fabber, thanks for your feedback. It wouldn't hurt to merge, I guess, and expect further feedback from more consumers. We can even remove the retry bits. |
|
If not for any other reasons I think it would be a simpler fix (just using the GITHUB_TOKEN) to discuss in this PR and merge, and then if we see later that it was not enough we can look at a solution for retrying. If you read it from the GitHub context ( I think that separate discussion in the original issue, about not even using the API if the user says the version is "locked" (I think?) also is a good thing™️. But can also be a separate fix with its own discussion. I won't interfere with how you would like to approach this, but I will happily help test and review the PR if it just uses the token. Testing the retries is a bit harder, and like starbelly I think any retry if added should use the timing information in the response headers. |
💯 agree that we should just do the github token part, and then work on retry on a separate PR. |
paulo-ferraz-oliveira
changed the title
|
@kivra-fabber, I've updated this to get rid of all non-env.-changed -related code. If you want, give it a go. |
|
@paulo-ferraz-oliveira nice! I'll get it bumped soon in the repo we had issues in. Given that the first iteration of your fix remediated our problems I'm not going to be as fast on it this time, but I expect we can be testing it this week. |
|
@lpil, also tagging you here to see if you wanna test this on top of Gleam. 😁 |
There was a problem hiding this comment.
@paulo-ferraz-oliveira I got a 401 when trying this with my fork of gleam.
https://github.com/starbelly/gleam/actions/runs/3830636416/jobs/6518761952
src/setup-beam.js
Outdated
| @@ -423,7 +423,10 @@ async function get(url0, pageIdxs) { | |||
| .get( | |||
| url, | |||
| { | |||
| headers: { 'user-agent': 'setup-beam' }, | |||
| headers: { | |||
| 'user-agent': 'setup-beam', | |||
There was a problem hiding this comment.
Right, we a need get_json function I suppose without the header (seemingly) for the content type a 401 will occur.
There was a problem hiding this comment.
Upon some investigation it's not the headers, but it consistently fails for windows, all the other envs are fine.
There was a problem hiding this comment.
Strange it worked at all. I skipped specifying the GITHUB_TOKEN env var in my gleam fork. So, this will have to be optional. That is, we can only conditionally add the header otherwise we'll definitely break what's out there.
There was a problem hiding this comment.
Right, we a need get_json function I suppose without the header (seemingly) for the content type a 401 will occur.
I implemented jsonParse which should output the non-JSON, the exception, and error out.
This seems to affect Windows builds, so might not be the best solution, but we're trying to isolate the issue to move forward
We still throw the error, but in a more controlled manner
paulo-ferraz-oliveira
changed the title
|
Oh nice, released already 👍 I think the optional bit inside the JS can be avoided by updating https://github.com/erlef/setup-beam/blob/main/action.yml to pass |
I'll double check that, if we can avoid people having to modify their workflow, all the better. |
I was unaware the token would sometimes not be present, but maybe I overlooked that since I added it to the tests... 😞 |
|
@kivra-fabber #172 Thanks for an awesome suggestion 👍 |
Closes #167.
Edit: I wanted to open this on my fork, but got duped by GitHub once more and opened it here. It's a draft, for the time being.
JSON.parseif the response is not JSON (or otherwise catch that exception to make it more usable for consumers)