Cache key output #717
Cache key output #717
Conversation
|
Fwiw, jsoref@edb9b28 shows tests pass for this commit |
| @@ -6,7 +6,8 @@ export enum Inputs { | |||
| } | |||
|
|
|||
| export enum Outputs { | |||
| CacheHit = "cache-hit" | |||
| CacheHit = "cache-hit", | |||
| CacheKey = "cache-key" | |||
There was a problem hiding this comment.
Would it be better to name it cache-matched-key to keep it aligned with the corresponding state being set.
Not sure if it is allowed to have state and output with same id
There was a problem hiding this comment.
I will use whatever people request. But, I'd like to avoid updates unless people agree on direction.
| @@ -102,7 +102,8 @@ test("setOutputAndState with exact match to set cache-hit output and state", () | |||
| actionUtils.setOutputAndState(key, cacheKey); | |||
|
|
|||
| expect(setOutputMock).toHaveBeenCalledWith(Outputs.CacheHit, "true"); | |||
| expect(setOutputMock).toHaveBeenCalledTimes(1); | |||
| expect(setOutputMock).toHaveBeenCalledWith(Outputs.CacheKey, key); | |||
There was a problem hiding this comment.
Shouldn't it match to cacheKey?
There was a problem hiding this comment.
Shrug, key and cacheKey are the same.
|
@vsvipul please take a look |
|
I spent some more time on this, it appears that the @actions/cache method doesn't actually provide enough information. This new flavor does yield enough information, although admittedly it's more expensive. -- For fallback cases, it will retrieve the cache item twice, and it makes n additional cache queries. I could skip the doubled cache retrieval by changing the primary call to not include fallback items. It'd be nicer if there was some easy way to figure out from the cache which item was providing the answer. |
|
@lucascosti: I don't suppose you could look into this? I mentioned it in github/docs#18524 If someone is willing to review, I'm happy to update. |
|
Sorry, @jsoref, because these are code changes, the Actions engineers must review this (cc @aparna-ravindra) |
|
@jsoref @lucascosti any chance we can revive this PR? If there's work to be done, I'm willing to take it over, just need to know what you need to get it merged. |
|
I'm happy to update, but only if someone will commit to reviewing it and @vsvipul / @bishal-pdMSFT seem to have just dropped the hot-potato. |
| } else { | ||
| let i: number; | ||
| for (i = 0; i < restoreKeys.length - 1; i++) { | ||
| const fallbackCacheKey = await cache.restoreCache( |
There was a problem hiding this comment.
Why are you calling restoreCache again?
See line 36: it is already called with the restore keys, so how can this succeed?
This addresses #661 (comment)