-
-
Notifications
You must be signed in to change notification settings - Fork 21.8k
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
CI: Ensure default branch cache persists #100457
Conversation
Looks good overall, but how does this deal with stable branches like We don't want all of those to use caches from |
Commenting something I've come across for earlier PRs: It may be useful to use |
At a glance this looks promising! Was considering something like it in my testing, will give it a review in the next few days when I find time! |
In the short-term, it doesn't. Those branches already have their own Action syntax/names, which don't overlap with this PR's new cache names at all. If anything, they'll behave better, as now there's zero risk of falling back to As for if this gets cherry-picked and/or when 4.4 becomes a stable branch... That's a little trickier. It might be possible to exclude
It's extremely important to note that only the default branch of a repo can have its cache accessed by other branches. The idea that all caches could be shared across any PR was a HUGE misconception of the earlier implementation; they're deliberately isolated. That's why I removed the last fallback key case, it never came into play. If we wanted something that could reference any PR from any PR, the only things I can think of are:
I've not looked into either option thoroughly, nor am I certain if they're even feasible, but it's a start. I'm absolutely gonna pursue them further in other PRs, but they're almost certainly outside the scope of this one. |
This solution looks good, can't extensively test it as such but it should work based on what I can see |
65babb9
to
865ef15
Compare
865ef15
to
5265503
Compare
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.
Can't confirm the behavior itself but the change makes sense and I think we can give this a try and see if it helps
Needs rebase to resolve merge conflicts. |
5265503
to
0b98715
Compare
There is now a warning when running:
Unsure if this inhibits the fetch or not, will have to test by re-running once CI has finished |
Would have been good to have been able to verify this before the PR was merged, but will see if a fix is needed once CI is done on |
I've been brainstorming solutions to our cache system & this is what I've come up with: a peristent cache for the default branch. Presently, after a merge batch happens on main, we're lucky if that cache lasts the entire day; when traffic is high, we're lucky if it lasts mere hours. This comes down to a double-whammy of caches being immutable (can't override existing versions) & fallback keys not counting as "accessing" the cache. While the former is something we just have to live with, this PR works around the latter with this new system:
I've cleaned out my cache to demonstrate how integrating this affects a cache list. When quota is reached, the oldest cache accessed are what will go away first.