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

Cache dockerhub images from linux for macOS #768

Merged
merged 5 commits into from
Aug 31, 2021

Conversation

ChristopherHX
Copy link
Contributor

@ChristopherHX ChristopherHX commented Aug 8, 2021

An attempt to avoid hitting the dockerhub Pull Rate Limit.

There is a small overhead due to caching (ca. +2 min, next run within 7 days only some seconds), but it might avoid rerunning the macOS job due to this error.
Restoreing the cache doesn't take much extra job execution time, because the test step is pulling less images from dockerhub.

This PR failed due to this limit #776.
It seems to be triggered by multiple PR's are opened within some days.

Needs more testing, if this solves the dockerhub limit problems or only delays the problem to happen less frequently.

Add new dockerhub images to CACHED_DOCKER_IMAGES and increment the cache key (CACHED_DOCKER_IMAGES_KEY)

What do you think?

I need ACT_OWNER: ${{ github.repository_owner }} and ACT_REPOSITORY: ${{ github.repository }} to have all tests green within my fork

An attempt to avoid hitting the dockerhub Pull Rate Limit.
@codecov
Copy link

codecov bot commented Aug 8, 2021

Codecov Report

Merging #768 (2894487) into master (0f04942) will increase coverage by 4.65%.
The diff coverage is 62.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
+ Coverage   49.27%   53.92%   +4.65%     
==========================================
  Files          23       23              
  Lines        2401     2674     +273     
==========================================
+ Hits         1183     1442     +259     
+ Misses       1090     1086       -4     
- Partials      128      146      +18     
Impacted Files Coverage Δ
pkg/container/docker_build.go 0.00% <0.00%> (ø)
pkg/container/docker_run.go 1.77% <0.00%> (-0.16%) ⬇️
pkg/common/git.go 52.85% <30.15%> (-6.94%) ⬇️
pkg/runner/runner.go 68.96% <33.33%> (-7.51%) ⬇️
pkg/model/planner.go 52.14% <43.33%> (+19.06%) ⬆️
pkg/model/workflow.go 51.08% <62.68%> (+25.37%) ⬆️
pkg/container/docker_pull.go 36.17% <64.70%> (+17.98%) ⬆️
pkg/runner/expression.go 85.38% <71.87%> (-1.26%) ⬇️
pkg/runner/step_context.go 78.92% <79.90%> (+9.97%) ⬆️
pkg/runner/run_context.go 80.36% <98.55%> (+3.95%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c645b3...2894487. Read the comment docs.

@ChristopherHX ChristopherHX marked this pull request as ready for review August 11, 2021 14:33
@ChristopherHX ChristopherHX requested a review from a team as a code owner August 11, 2021 14:33
catthehacker
catthehacker previously approved these changes Aug 11, 2021
Copy link
Member

@catthehacker catthehacker left a comment

Choose a reason for hiding this comment

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

My plans are to escape DockerHub and move everything to other registries but this will suffice for now.

@mergify mergify bot requested a review from a team August 11, 2021 14:56
cplee
cplee previously approved these changes Aug 12, 2021
@mergify

This comment has been minimized.

@mergify mergify bot added the needs-work Extra attention is needed label Aug 30, 2021
@ChristopherHX
Copy link
Contributor Author

This PR doesn't need work, it only needs to be merged manually
However mergify added this label weeks later, once another PR was merged

@mergify

This comment has been minimized.

@mergify mergify bot removed the needs-work Extra attention is needed label Aug 30, 2021
@mergify

This comment has been minimized.

@mergify mergify bot added the conflict PR has conflicts label Aug 30, 2021
@ChristopherHX ChristopherHX dismissed stale reviews from cplee and catthehacker via 2894487 August 30, 2021 18:39
@mergify mergify bot removed the conflict PR has conflicts label Aug 30, 2021
@mergify mergify bot requested a review from a team August 30, 2021 18:50
@cplee cplee merged commit 6e5bd24 into nektos:master Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants