Skip to content

actions/cache key is not only about exact match #14145

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

Closed
1 task done
Totktonada opened this issue Jan 15, 2022 · 20 comments
Closed
1 task done

actions/cache key is not only about exact match #14145

Totktonada opened this issue Jan 15, 2022 · 20 comments
Labels
actions This issue or pull request should be reviewed by the docs actions team content This issue or pull request belongs to the Docs Content team help wanted Anyone is welcome to open a pull request to fix this issue pumpkin-spice Specifically tracked Hacktoberfest issue - internal purposes

Comments

@Totktonada
Copy link

Totktonada commented Jan 15, 2022

Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows#matching-a-cache-key

What part(s) of the article would you like to see updated?

The documentation provides description of key and restore-keys fields. The actual behaviour is the following:

  1. If key match existing cache entry exactly, the cache is restored and cache-hit output value is set to 'true'.
  2. If key match a prefix of an existing cache entry, the cache is restored and cache-hit output value is set to ''.

The latter is not obvious from the documentation. To be honest, I would even say that the article reads like it works in the opposite way.

The reason why I misunderstood the documentation is the following. The description of the key does not say anything about exact/prefix match. It also say that match is called 'cache hit', so, I guess, it is when cache-hit is set to 'true': exact match.

Next, restore-keys description differentiates 'exact match' and 'partial match' (prefix match in fact). So my guess was that if key would behave this way, it would be described as the common behaviour of both fields. Now it looks like a highlight that restore-keys works differenctly from key, but that's not so.

I would also highlight that cache-hit value is not described. It is quite non-obvious that a cache may be restored, but the output value will give '', because it is not exact match.

Additional information

One can verify the behaviour youself. Let's create a workflow file with the following content and push it to a branch:

name: Test cache

on: [push]

jobs:
  test_cache:
    env:
      CACHE_KEY: my-cache-key-v1-draft
    runs-on: ubuntu-latest
    steps:
      - name: Cache file
        uses: actions/cache@v2
        id: cache
        with:
          path: cached_file
          key: ${{ env.CACHE_KEY }}

      - name: Generate file
        run: |
          [ ! -f cached_file ]  # should never give an error
          printf 'CACHE_KEY: %s\n' "${{ env.CACHE_KEY }}" > cached_file
        if: steps.cache.outputs.cache-hit != 'true'

      - name: Show file
        run: |
          cat cached_file

Next, change the cache key:

diff --git a/.github/workflows/test_cache.yml b/.github/workflows/test_cache.yml
index 5a0f6d4..a3d448e 100644
--- a/.github/workflows/test_cache.yml
+++ b/.github/workflows/test_cache.yml
@@ -5,7 +5,7 @@ on: [push]
 jobs:
   test_cache:
     env:
-      CACHE_KEY: my-cache-key-v1-draft
+      CACHE_KEY: my-cache-key-v1
     runs-on: ubuntu-latest
     steps:
       - name: Cache file

And push it to a branch. What will occur? The cache will be restored from my-cache-key-v1-draft, steps.cache.outputs.cache-hit will be '', so we'll run the 'Generate file' step and get a failure.

In fact, we made two mistakes here:

  1. Assume that key is about exact match.
  2. Assume that if cache restore occurs it means that cache-hit will be 'true'.

Those pitfalls should at least be highlighted in the documentation. However, to be honest, those are good candidates to revisit in a next major version of the action.


Content Plan

Guidance here

@Totktonada Totktonada added the content This issue or pull request belongs to the Docs Content team label Jan 15, 2022
@welcome
Copy link

welcome bot commented Jan 15, 2022

Thanks for opening this issue. A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Jan 15, 2022
@ramyaparimi ramyaparimi added actions This issue or pull request should be reviewed by the docs actions team waiting for review Issue/PR is waiting for a writer's review and removed triage Do not begin working on this issue until triaged by the team labels Jan 17, 2022
vr009 added a commit to tarantool/smtp that referenced this issue Jan 24, 2022
In case server has some settings, which don't let some
types of SMTP request to be handled, SMTP client may receive
5xx, 4xx, 3xx codes. This patch fixes the problem, when
on the client side this SMTP codes are not handled normally.
Instead of runtime error and unusual message that there is
some connection error.

The value of key for cache-step bumped to v4. That is needed
for changing postfix of the key. During testing of changes
there were troubles with cache-reload described and reported
in github/docs#14145.

Fixes #13
vr009 added a commit to tarantool/smtp that referenced this issue Jan 25, 2022
In case server has some settings, which don't let some
types of SMTP request to be handled, SMTP client may receive
5xx, 4xx, 3xx codes. This patch fixes the problem, when
on the client side this SMTP codes are not handled normally.
Instead of runtime error and unusual message that there is
some connection error.

The value of key for cache-step bumped to v4. That is needed
for changing postfix of the key. During testing of changes
there were troubles with cache-reload described and reported
in github/docs#14145.

Fixes #13
vr009 added a commit to tarantool/smtp that referenced this issue Jan 31, 2022
In case server has some settings, which don't let some
types of SMTP request to be handled, SMTP client may receive
5xx, 4xx, 3xx codes. This patch fixes the problem, when
on the client side this SMTP codes are not handled normally.
Instead of runtime error and unusual message that there is
some connection error.

The value of key for cache-step bumped to v4. That is needed
for changing postfix of the key. During testing of changes
there were troubles with cache-reload described and reported
in github/docs#14145.

Fixes #13
vr009 added a commit to tarantool/smtp that referenced this issue Jan 31, 2022
In case server has some settings, which don't let some
types of SMTP request to be handled, SMTP client may receive
5xx, 4xx, 3xx codes. This patch fixes the problem, when
on the client side this SMTP codes are not handled normally.
Instead of runtime error and unusual message that there is
some connection error.

The value of key for cache-step bumped to v4. That is needed
for changing postfix of the key. During testing of changes
there were troubles with cache-reload described and reported
in github/docs#14145.

Fixes #13
vr009 added a commit to tarantool/smtp that referenced this issue Jan 31, 2022
In case server has some settings, which don't let some
types of SMTP request to be handled, SMTP client may receive
5xx, 4xx, 3xx codes. This patch fixes the problem, when
on the client side this SMTP codes are not handled normally.
Instead of runtime error and unusual message that there is
some connection error.

The value of key for cache-step bumped to v4. That is needed
for changing value of the key. During testing of changes
there were troubles with cache-reload described and reported
in github/docs#14145.

Fixes #13
Totktonada pushed a commit to tarantool/smtp that referenced this issue Jan 31, 2022
In case server has some settings, which don't let some
types of SMTP request to be handled, SMTP client may receive
5xx, 4xx, 3xx codes. This patch fixes the problem, when
on the client side this SMTP codes are not handled normally.
Instead of runtime error and unusual message that there is
some connection error.

The value of key for cache-step bumped to v4. That is needed
for changing value of the key. During testing of changes
there were troubles with cache-reload described and reported
in github/docs#14145.

Fixes #13
@ethomson
Copy link
Contributor

ethomson commented Feb 2, 2022

Hi @Totktonada ! Thanks for opening this issue. At the moment, the documentation states:

cache-hit: A boolean value to indicate an exact match was found for the key.

Can you suggest how we could clarify this? Feel free to open a PR and we can iterate on the language there, if you'd like!

@hosein1arti

This comment was marked as spam.

@Totktonada
Copy link
Author

The cache-hit output parameter description is technically correct, however I would specifically highlight the 'exact' word or explicity mention that a partial match sets the parameter to ''.

The main problem is not here. Let's look at the following paragraph:

You can provide a list of restore keys to use when there is a cache miss on key. You can create multiple restore keys ordered from the most specific to least specific. The cache action searches for restore-keys in sequential order. 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.

The whole paragraph is about restore-keys. It highlights that the cache action searches for the keys prefixed with given ones. Is it about restore-keys or both key and restore-keys? I read it as the former, but actual behaviour is the latter.

Next, the list below:

  1. If you provide restore-keys, the cache action sequentially searches for any caches that match the list of restore-keys.
    • When there is an exact match, the action restores the files in the cache to the path directory.
    • If there are no exact matches, the action searches for partial matches of the restore keys. When the action finds a partial match, the most recent cache is restored to the path directory.
  2. <...>
  3. <...>

The same problem. The logic about prefixing is described for restore-keys, so I read it as if key would have different logic.


I would prefer to leave a feedback and go ahead, but if the problem still not clear enough, I'll open a pull request.

@ramyaparimi ramyaparimi added the needs SME This proposal needs review from a subject matter expert label Mar 25, 2022
@github-actions
Copy link
Contributor

This is a gentle bump for the docs team that this issue is waiting for technical review.

@github-actions github-actions bot added the SME stale The request for an SME has staled label Apr 12, 2022
@Kieudung717

This comment was marked as spam.

@Kieudung717

This comment was marked as spam.

@ramyaparimi
Copy link
Contributor

@Totktonada Thanks so much for your patience. I spoke with the team and we are not going to accept this contribution 💛. We really appreciate your time and interest in improving GitHub docs 💖 .

@Totktonada
Copy link
Author

Totktonada commented Apr 13, 2022

@ramyaparimi Can you reveal a reason, please?

@ramyaparimi
Copy link
Contributor

@Totktonada I am extremely sorry for closing this issue. I didn't notice the comment with an SME review earlier and closed it assuming it was not reviewed by an SME. I apologize for this and I am going to reopen this issue and get this triaged for a writer's review 👀.

Thanks so much for your patience and understanding 💖.

@ramyaparimi ramyaparimi reopened this Apr 13, 2022
@ramyaparimi ramyaparimi removed the SME stale The request for an SME has staled label Apr 13, 2022
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Apr 13, 2022
@ramyaparimi ramyaparimi removed the needs SME This proposal needs review from a subject matter expert label Apr 13, 2022
@ramyaparimi ramyaparimi added SME reviewed An SME has reviewed this issue/PR and removed triage Do not begin working on this issue until triaged by the team labels Apr 13, 2022
@ramyaparimi ramyaparimi added needs SME This proposal needs review from a subject matter expert and removed SME reviewed An SME has reviewed this issue/PR labels May 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2022

Thanks for opening an issue! We've triaged this issue for technical review by a subject matter expert 👀

@ramyaparimi
Copy link
Contributor

@Totktonada I have triaged this for a subject matter expert to review the approach in this comment. Thanks so much for your patience 💖 .

@github-actions
Copy link
Contributor

This is a gentle bump for the docs team that this issue is waiting for technical review.

@github-actions github-actions bot added the SME stale The request for an SME has staled label May 12, 2022
@janiceilene
Copy link
Contributor

👋 @Totktonada Thanks so much for your patience as the team reviewed your suggestions!

You (or anyone else!) are welcome to open a PR making the changes described in #14145 (comment) 💖

@janiceilene janiceilene added help wanted Anyone is welcome to open a pull request to fix this issue and removed waiting for review Issue/PR is waiting for a writer's review needs SME This proposal needs review from a subject matter expert SME stale The request for an SME has staled labels May 16, 2022
@cmwilson21 cmwilson21 added the pumpkin-spice Specifically tracked Hacktoberfest issue - internal purposes label Sep 30, 2022
@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Aug 10, 2023
@cmwilson21 cmwilson21 removed the stale There is no recent activity on this issue or pull request label Aug 11, 2023
@felicitymay
Copy link
Contributor

felicitymay commented Sep 19, 2023

Hi @Totktonada 👋🏻

We're reviewing all open help wanted issues as part of our preparation for Hacktoberfest, which is coming up shortly.

It looks as if this article and the information about cache-hit was updated in November 2022 to improve the clarity.

We'll plan to close this issue at the end of the week unless you have further feedback on this article.

@Totktonada
Copy link
Author

@felicitymay, hi!

I guess you're about 9569184. As I see, the information about key, restore-keys and cache-hit is still incomplete.

In my understanding (if nothing was changed in the cache action implementation), the documentation should make clear the following points.

  • key allows a partial match.
  • cache-hit is set to '' (not 'true') at the partial match.

@felicitymay
Copy link
Contributor

Thank you for getting back to us so quickly with clear feedback. I'm not an actions expert, so this is really helpful. 💖

I'll update the issue summary soon and this will be ready for someone to work on. We usually get a lot of contributions during Hacktoberfest.

@felicitymay felicitymay self-assigned this Sep 20, 2023
@felicitymay
Copy link
Contributor

Before updating the content plan for fixing the documentation, I spent some time looking at the documentation for the cache action. I'm now wondering if the behavior you observed is the intended behavior.

I'm going to check with the action maintainers because the partial matching behavior on key you see doesn't seem to be covered in the docs or tests for the cache action.

@Totktonada
Copy link
Author

Thank you!

I would highlight one related point.

I don't remember precisely now, but even if the key behavior will be changed and/or documented, restore-keys likely also set cache-hit to '' on the partial match and it may be not obvious behavior.

We can't lean on ${{ steps.mycachestep.outputs.cache-hit != 'true' }} as a criteria for a cache miss. I think that the documentation should provide three criterias: cache hit with exact match (== 'true'), cache hit with exact or partial match, cache miss.

@felicitymay
Copy link
Contributor

Hi @Totktonada. Thank you for your patience with this discussion and for all your thoughtful feedback. ✨

I've just had a discussion with a member of the team that is taking over support of this action. They agree that the behavior you've uncovered might well be unintended. We're going to look into the behavior internally and update this article once we've corrected any problems with the action itself. I'm going to close this issue as we will track both the action and documentation updates internally.

Thank you for all your help identifying this inconsistency 🙇🏻‍♀️

@felicitymay felicitymay removed their assignment Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actions This issue or pull request should be reviewed by the docs actions team content This issue or pull request belongs to the Docs Content team help wanted Anyone is welcome to open a pull request to fix this issue pumpkin-spice Specifically tracked Hacktoberfest issue - internal purposes
Projects
None yet
Development

No branches or pull requests

11 participants