refactor: introduce restore keys for cache - #280
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
📝 WalkthroughWalkthrough
ChangesCache restore fallback
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cache-restore/run.ts`:
- Around line 21-27: Update the cache-hit handling around restoreCache in run so
it reports true only when the returned cache key exactly equals primaryKey;
preserve restoring contents for restoreKeys prefix matches while returning
cache-hit=false. Add a regression test covering a restore-key match and
verifying dependency installation remains reachable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c1c1d484-d0ad-4c1c-94f4-21b1ec789dac
📒 Files selected for processing (1)
src/cache-restore/run.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Standalone Windows self-update (PATH regression)
⚠️ CI failures not shown inline (2)
GitHub Actions: pr-check / 0_check-dist.txt: refactor: introduce restore keys for cache
Conclusion: failure
##[group]Run git diff --exit-code dist/index.js
�[36;1mgit diff --exit-code dist/index.js�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
diff --git a/dist/index.js b/dist/index.js
index 1866127..b11810d 100644
Binary files a/dist/index.js and b/dist/index.js differ
##[error]Process completed with exit code 1.
GitHub Actions: pr-check / check-dist: refactor: introduce restore keys for cache
Conclusion: failure
##[group]Run git diff --exit-code dist/index.js
�[36;1mgit diff --exit-code dist/index.js�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
diff --git a/dist/index.js b/dist/index.js
index 1866127..b11810d 100644
Binary files a/dist/index.js and b/dist/index.js differ
##[error]Process completed with exit code 1.
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-11T12:04:07.383Z
Learnt from: zkochan
Repo: pnpm/action-setup PR: 256
File: src/install-pnpm/run.ts:150-166
Timestamp: 2026-05-11T12:04:07.383Z
Learning: In pnpm/action-setup, when validating or forwarding the version value used for `pnpm self-update` and/or `devEngines.packageManager.version`, do not restrict it to exact versions or tags only—pnpm supports semver ranges for these inputs. Ensure any code that parses/validates `devEngines.packageManager.version` (and the value passed to `pnpm self-update`) allows range syntax such as `^`, `~`, and comparators (e.g. `>=8 <10`) instead of rejecting anything that isn’t a single exact version. Note: the plain `packageManager` field is different, so apply this “allow semver ranges” rule specifically to `devEngines.packageManager.version` / `self-update` handling.
Applied to files:
src/cache-restore/run.ts
BlackHole1
left a comment
There was a problem hiding this comment.
LGTM. setup-node has similar logic: https://github.com/actions/setup-node/blob/e51e5fe84fc33b4c73ebe40526b2694712b5b858/src/cache-restore.ts#L43-L64
|
@zkochan Would you mind reviewing and merging this one, too? I believe there are some pnpm@10 users in the wild that may still benefit from this small change. |
Add
restoreKeysfor restoring the store cache.The idea is that we currently include the
pnpm-lock.yamlcontent hash within the cache primary key. This means even if only one dependency has changed, the entirepnpm-lock.yamlcontent hash changes, and the entire pnpm store cache is now thrown away.But we don't have to invalidate the entire store. Other dependencies' cache may still be utilized. We can still restore the previous store cache, and only the changed dependencies need to be and will be downloaded.
restoreKeysdoesn't change the fact that the cache is still miss (primaryKey), so during the post action we still upload the latest pnpm store to a new cache entry.Summary by CodeRabbit
Summary by CodeRabbit