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

refactor: improve error message during weval pull failure #12

Conversation

vados-cosmonic
Copy link
Contributor

@vados-cosmonic vados-cosmonic commented Nov 8, 2024

Hey all, did a little bit of refactoring to get a slightly better (I think!) error message in for when weval has to be fetched.

@guybedford
Copy link
Collaborator

Thanks Victor for investigating this - this has been an ongoing issue for Weval when GitHub decides to ratelimit API requests.

@vados-cosmonic
Copy link
Contributor Author

Yeahhh I figured it was something like that -- it turns out most of the changes could go in one level up @ componetize-js but while I was there I figured I'd improve the error message! The other PRs @ componetize-js and jco are more important :)

@cfallin cfallin merged commit 0287122 into bytecodealliance:main Nov 8, 2024
11 checks passed
@guybedford
Copy link
Collaborator

I think the better fix that would be preferable would to be use token authentication with the GitHub API. The problem is that the fetch here doesn't respect the Authorization header per https://docs.github.com/en/rest/authentication/authenticating-to-the-rest-api?apiVersion=2022-11-28, per a env.GITHUB_TOKEN environment variable.

If that was supported then we could run the CI with this environment variable set to the secret of the same name in GitHub CI without hitting the ratelimiting issues.

@vados-cosmonic I do think an upstream fix in Weval like the above is preferable to a downstream workaround.

@cfallin
Copy link
Member

cfallin commented Nov 8, 2024

Actually, it looks like the direct file downloads have predictable URLs (e.g. https://github.com/bytecodealliance/weval/releases/download/v0.3.2/weval-v0.3.2-x86_64-linux.tar.xz); we already bake in the version tag and update it as part of our release checklist; perhaps we can avoid hitting the API altogether, and doing a (regular, unauthenticated) download of the direct URL?

Happy to take a PR if someone wants to take an attempt at that...

@guybedford
Copy link
Collaborator

It sounds like that could work well too.

@vados-cosmonic vados-cosmonic deleted the refactor=improve-error-message-for-failure-pulling-weval branch November 9, 2024 06:02
@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Nov 9, 2024

Thanks for the notes ya'll -- I agree, both of those solutions would be preferable -- I'll try and do both!

I think the average person won't actually have GITHUB_TOKEN set (similarly they probably may not want to use --weval-bin as in the upstream), but that definitely helps fix the CI issues (in a slightly spooky-at-a-distance way), and also using the actual URL of the release is much better.

In other projects we certainly use the direct URLs (but who's to say someday those won't be rate-limited as well!).

[EDIT] There's also the thought that we should probably start enforcing checksums possibly baked into the code for this -- this way if we ever run into some sort of issue we could stand up alternative delivery mechanisms and be able to trust them a little bit more/avoid compromise.

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.

3 participants