Do not include a cache mount's ID in the ExecOp's cachemap #4585
Conversation
cpuguy83
force-pushed
the
exclude_cachemount_id_from_cachemap
branch
from
4e7c61b to
e2b73b6
Compare
cpuguy83
force-pushed
the
exclude_cachemount_id_from_cachemap
branch
from
e2b73b6 to
867839d
Compare
A cache ID should not have any impact on whether or not a step should be re-run any more than the content of that cache does (or rather, doesn't). Signed-off-by: Brian Goff <[email protected]>
cpuguy83
force-pushed
the
exclude_cachemount_id_from_cachemap
branch
from
867839d to
584ec40
Compare
| // This is a dockerfile default cache mount. | ||
| // We are treating this as a special case so we don't cause a cache miss unintentionally. | ||
| if m.CacheOpt.ID == m.Dest && m.CacheOpt.Sharing == 0 { | ||
| return false | ||
| } | ||
|
|
||
| // Check the case where a dockerfile cache-namespace may be used. | ||
| // This would be `<namespace>/<dest>` | ||
| _, trimmed, ok := strings.Cut(m.CacheOpt.ID, "/") | ||
| if ok && trimmed == m.Dest && m.CacheOpt.Sharing == 0 { | ||
| return false | ||
| } |
There was a problem hiding this comment.
nit: can we condense these cases together? We can get the trimmed ID, and then compare and check the sharing, given they're essentially the same case.
I'm also still not sure I understand where this special case can actually happen. Is it just to prevent dockerfile cache misses when upgrading buildkit?
There was a problem hiding this comment.
can we condense these cases together? We can get the trimmed ID, and then compare and check the sharing, given they're essentially the same case.
Seems like this would make the code more confusing, and certainly more expensive in the case where there is nothing to trim.
There was a problem hiding this comment.
Is it just to prevent dockerfile cache misses when upgrading buildkit?
Yes, I believe so.
See the discussion thread here: https://dockercommunity.slack.com/archives/C7S7A40MP/p1705105919498119
A cache ID should not have any impact on whether or not a step should be re-run any more than the content of that cache does (or rather, doesn't).