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

Use lychee cache #6161

Merged
merged 5 commits into from
Oct 2, 2024
Merged

Use lychee cache #6161

merged 5 commits into from
Oct 2, 2024

Conversation

dmathieu
Copy link
Member

A few months ago, we enabled lychee cache in the SDK repo, to prevent rate limit issues, but forgot to enable it here.
open-telemetry/opentelemetry-go#5160

@dmathieu dmathieu added the Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG label Sep 30, 2024
@dmathieu
Copy link
Member Author

Note that the check needs to succeed once for the cache to be stored (lychee doesn't update the cache if there's a failure anyway).

@dmathieu dmathieu marked this pull request as ready for review September 30, 2024 09:02
@dmathieu dmathieu requested a review from a team as a code owner September 30, 2024 09:02
@pellared
Copy link
Member

pellared commented Oct 2, 2024

@dmathieu, this does not look like fixing the following issue:

However, I am not against merging it as I think it is good to have the same CI configuration.

@dmathieu
Copy link
Member Author

dmathieu commented Oct 2, 2024

The check needs to run successfully once (we can trigger it at a moment of low activity) to trigger the cache.

@dmathieu
Copy link
Member Author

dmathieu commented Oct 2, 2024

If we can't ever run the check once, we'd either need to figure out a way to split the check (I don't think lychee can do that), or to add an auth token to the GitHub requests so the rate limit is increased.

Alternatively, lycheeverse/lychee#1403 would allow us to update the cache with only the successful checks, making failures retry only what didn't succeed previously. That PR is stalled, and I haven't been able to figure out its issue so far.

@pellared
Copy link
Member

pellared commented Oct 2, 2024

add an auth token to the GitHub requests so the rate limit is increased

I think this is the way. We just need to make sure to limit the permissions for GITHUB_TOKEN using https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token#defining-access-for-the-github_token-permissions

My guess is that it just needs contents: read or even nothing 😉

Reference: https://github.com/lycheeverse/lychee/blob/master/docs/TROUBLESHOOTING.md#github-rate-limiting

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.8%. Comparing base (1c8a001) to head (9ed16ca).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6161   +/-   ##
=====================================
  Coverage   66.8%   66.8%           
=====================================
  Files        206     206           
  Lines      13211   13211           
=====================================
+ Hits        8833    8835    +2     
+ Misses      4111    4110    -1     
+ Partials     267     266    -1     

see 1 file with indirect coverage changes

@dmathieu dmathieu merged commit a80077a into open-telemetry:main Oct 2, 2024
24 checks passed
@dmathieu dmathieu deleted the lychee-cache branch October 2, 2024 13:46
dmathieu added a commit to open-telemetry/opentelemetry-go that referenced this pull request Oct 3, 2024
This sets up a github token to authenticate requests made by lychee, so
GitHub doesn't rate limit us so easily.

Related:
open-telemetry/opentelemetry-go-contrib#6161
@MrAlias MrAlias added this to the v1.31.0 milestone Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Allow PR to succeed without requiring an addition to the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants