-
Notifications
You must be signed in to change notification settings - Fork 329
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
Automatically clean up old TRAP caches #2306
Conversation
Since we'll only cleanup when running on the default branch
{ | ||
id: 7, | ||
key: "codeql-trap-1-2.0.0-swift-older", | ||
created_at: "2024-02-21T14:25:00Z", | ||
size_in_bytes: 300 * 1024 * 1024, | ||
}, |
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.
This one wasn't deleted. Is it because the timestamp is identical to id 6? Wait...the code should only keep one cache per language. So, I'm not sure what's going on here.
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.
Ah, I see now that the reasoning for this is a bit opaque without looking at the rest of the file. We're expecting to only clean up TRAP caches for JavaScript and Ruby as these are the only ones that the getTestConfigWithTempDir
config specifies as languages to analyze in this job. I'll add a comment to make this clearer.
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.
Thanks. That makes a lot of sense.
This PR implements a new feature, behind a flag, that keeps only the newest TRAP cache for each language.
We call the Actions cache APIs directly as these have not yet been added to the
@actions/cache
npm package.I have added a unit test exercising most of the functionality, and have also tested this manually using a test repo. I think an integration test is probably not worth it, particularly on this repo where the databases are small enough that TRAP caching skips the upload anyway.
Merge / deployment checklist