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

fix: match cache restore-keys in creation reverse order #2153

Merged
merged 4 commits into from
Jan 20, 2024

Conversation

fuufen
Copy link
Contributor

@fuufen fuufen commented Jan 15, 2024

Fixes #1823

Will now return from cache in the order mentioned in GitHub Actions cache docs:

When a key doesn't match directly, the action searches for keys prefixed with the restore key. If there are multiple partial matches for a restore key, the action returns the most recently created cache.

@fuufen fuufen marked this pull request as ready for review January 15, 2024 09:59
@fuufen fuufen requested a review from a team as a code owner January 15, 2024 09:59
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 1021 lines in your changes are missing coverage. Please review.

Comparison is base (4989f44) 61.22% compared to head (c0d859d) 60.78%.
Report is 301 commits behind head on master.

Files Patch % Lines
pkg/artifactcache/handler.go 65.32% 103 Missing and 43 partials ⚠️
pkg/runner/run_context.go 73.37% 75 Missing and 19 partials ⚠️
pkg/runner/expression.go 55.17% 66 Missing and 12 partials ⚠️
pkg/runner/action_cache.go 50.74% 49 Missing and 17 partials ⚠️
pkg/container/docker_run.go 1.51% 64 Missing and 1 partial ⚠️
pkg/container/docker_network.go 0.00% 56 Missing ⚠️
pkg/model/planner.go 28.57% 53 Missing and 2 partials ⚠️
pkg/runner/reusable_workflow.go 52.47% 42 Missing and 6 partials ⚠️
pkg/model/workflow.go 43.37% 40 Missing and 7 partials ⚠️
pkg/common/outbound_ip.go 0.00% 44 Missing ⚠️
... and 27 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2153      +/-   ##
==========================================
- Coverage   61.22%   60.78%   -0.45%     
==========================================
  Files          46       53       +7     
  Lines        7141     8970    +1829     
==========================================
+ Hits         4372     5452    +1080     
- Misses       2462     3080     +618     
- Partials      307      438     +131     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mergify mergify bot requested a review from a team January 18, 2024 20:38
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

Can confirm that this works and the old implementation was incorrect, both can be checked by running this workflow file: It only succeeds after applying this PR after the fix from today

on: push
defaults:
  run:
    shell: bash

jobs:
    createcache:
        runs-on: ubuntu-latest
        steps:
        -   uses: actions/cache@v3
            id: cache
            with:
                path: |
                    ./.cache
                key: prefix0-keya
        -   run: |
                mkdir ./.cache
                echo createcache > ./.cache/Test
            if: steps.cache.outputs.cache-hit != 'true'
        -   run: exit 1
            if: steps.cache.outputs.cache-hit == 'true'
    createcache2:
        runs-on: ubuntu-latest
        needs: createcache
        steps:
        -   uses: actions/cache@v3
            id: cache
            with:
                path: |
                    ./.cache
                key: prefix0-keyb
        -   run: |
                mkdir ./.cache
                echo createcache2 > ./.cache/Test
            if: steps.cache.outputs.cache-hit != 'true'
        -   run: exit 1
            if: steps.cache.outputs.cache-hit == 'true'
    createcache3:
        runs-on: ubuntu-latest
        needs: createcache2
        steps:
        -   uses: actions/cache@v3
            id: cache
            with:
                path: |
                    ./.cache
                key: prefix1-keya
        -   run: |
                mkdir ./.cache
                echo createcache3 > ./.cache/Test
            if: steps.cache.outputs.cache-hit != 'true'
        -   run: exit 1
            if: steps.cache.outputs.cache-hit == 'true'
    hitcache:
        needs: createcache3
        runs-on: ubuntu-latest
        steps:
        -   uses: actions/cache@v3
            id: cache
            with:
                path: |
                    ./.cache
                key: dummy-key
                restore-keys: |
                  prefix0-
        -   run: 'ls -l ./.cache'
        -   run: exit 1
            if: steps.cache.outputs.cache-hit == 'true'
        -   run: echo We got the expected non exact hit the cache!
            if: steps.cache.outputs.cache-hit != 'true'
        -   run: |
              cat ./.cache/Test
        -   name: Verify that the right entry has been restored
            run: |
              [[ "createcache2" != "$(cat ./.cache/Test )" ]] && exit 1 || exit 0

@cplee
Copy link
Contributor

cplee commented Jan 19, 2024

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Jan 19, 2024

refresh

✅ Pull request refreshed

Copy link
Contributor

mergify bot commented Jan 20, 2024

@fuufen this pull request has failed checks 🛠

@mergify mergify bot added the needs-work Extra attention is needed label Jan 20, 2024
@mergify mergify bot removed the needs-work Extra attention is needed label Jan 20, 2024
@mergify mergify bot merged commit 7f7d84b into nektos:master Jan 20, 2024
10 checks passed
jmikedupont2 pushed a commit to meta-introspector/act that referenced this pull request Mar 10, 2024
* Match cache restore-keys in creation reverse order

* Match full prefix when selecting cache

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

Cache restore-keys not matched in creation reverse order
3 participants