-
Notifications
You must be signed in to change notification settings - Fork 421
Skip uploading dependency caches if we know they exist #3296
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
Skip uploading dependency caches if we know they exist #3296
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes the dependency caching mechanism by skipping cache uploads when the cache key was previously restored, avoiding unnecessary expensive operations like calculating cache sizes and API calls.
Key changes:
- Tracks restored cache keys during cache restoration and stores them in the config
- Checks if a cache with the same key was restored before attempting to upload, skipping the upload if it was
- Adds comprehensive unit test coverage for
uploadDependencyCacheswhich was previously untested
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config-utils.ts | Adds dependencyCachingRestoredKeys field to the Config interface to track restored cache keys |
| src/testing-utils.ts | Updates createTestConfig helper to include the new dependencyCachingRestoredKeys field |
| src/init-action.ts | Captures restored cache keys from downloadDependencyCaches and stores them in config |
| src/dependency-caching.ts | Adds DownloadDependencyCachesResult interface; modifies downloadDependencyCaches to return restored keys; optimizes uploadDependencyCaches to skip upload for known cache keys |
| src/dependency-caching.test.ts | Adds comprehensive unit tests for uploadDependencyCaches covering various scenarios (no config, no hash, duplicate, empty, stored, error handling) |
| src/config-utils.test.ts | Refactors tests to use createTestConfig helper instead of manually constructing config objects |
| lib/init-action.js | Auto-generated JavaScript from TypeScript source |
| lib/analyze-action.js | Auto-generated JavaScript from TypeScript source |
755b23c to
b665de6
Compare
b665de6 to
1ed85b4
Compare
redsun82
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR modifies
uploadDependencyCachesto avoid attempting to create and upload a dependency cache if a cache with the same key was previously restored, because that means we know that it already exists. Cache keys are intended to uniquely identify the contents of a cache and, based on that principle, it does not make sense to upload a cache with a key that already exists. Note that this is also not a change in behaviour here: ordinarily, the Cache API would refuse to accept a cache upload if a cache with the same key already exists. This change simply optimisesuploadDependencyCachesso that we don't perform expensive tasks when we can avoid them before the API tells us that the cache already exists.This is accomplished in the following way:
initaction, we now keep track of the cache keys that we restored.This PR also adds general unit test coverage for
uploadDependencyCacheswhich was missing so far.We need to continue to check for
ReserveCacheErrors when uploading caches, because there are two scenarios where we may still try to upload a cache that already exists:Some other notes:
initto match the cache key we compute inuploadDependencyCaches, because our analysis should not modify any project files.uploadDependencyCachesto ensure that it really corresponds to what we are about to put into the cache. This hasn't changed in this PR.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning).analysis-kinds: code-quality).How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Merge / deployment checklist