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

Prefer authenticated request to github api when the token is available #14263

Merged
merged 1 commit into from
May 26, 2023

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented May 24, 2023

The PR #14242 changed the install_foundry command to perform authenticated requests to the Github API when querying the foundry release SHA hash, since unauthenticated requests have a lower limit of requests per hour, which was causing intermittent failures.

However, external contributors do not have access to the GITHUB_ACCESS_TOKEN environment variable set in the CircleCI project settings, and thus fail to perform the authenticated request to the Github API:
https://app.circleci.com/pipelines/github/ethereum/solidity/29901/workflows/d2d574cb-6624-486f-904b-598b20118606/jobs/1328298

This PR changes the previous behavior to only add the Authorization header when the GITHUB_ACCESS_TOKEN is available.

.circleci/config.yml Outdated Show resolved Hide resolved
cameel
cameel previously approved these changes May 25, 2023
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Approving since the change is correct, but I also have a suggestion on how to improve it. Fine with merging it either way.

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@cameel cameel merged commit bb16f61 into develop May 26, 2023
@cameel cameel deleted the gh-auth-token-foundry-cmd branch May 26, 2023 10:30
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