fix(ci): save all Go caches on master#19069
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new generic
go-build-v1-restore key removes theGOARCH/TAGscoping, which may cause incompatible cache reuse across Go versions or other tag changes; consider including at least the Go version (or equivalent) in this fallback key to avoid subtle build issues after upgrades. - With the removal of
with: save: falsein the workflows, the behavior now depends on the default forinputs.saveincache-go-dependencies; double-check that the default matches the intended "save on master only" semantics for all callers to avoid unexpectedly writing caches on non-default branches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new generic `go-build-v1-` restore key removes the `GOARCH`/`TAG` scoping, which may cause incompatible cache reuse across Go versions or other tag changes; consider including at least the Go version (or equivalent) in this fallback key to avoid subtle build issues after upgrades.
- With the removal of `with: save: false` in the workflows, the behavior now depends on the default for `inputs.save` in `cache-go-dependencies`; double-check that the default matches the intended "save on master only" semantics for all callers to avoid unexpectedly writing caches on non-default branches.
## Individual Comments
### Comment 1
<location> `.github/actions/cache-go-dependencies/action.yaml:46` </location>
<code_context>
- restore-keys: go-build-v1-${{ github.job }}-${{ steps.cache-paths.outputs.GOARCH }}-
+ restore-keys: |
+ go-build-v1-${{ github.job }}-${{ steps.cache-paths.outputs.GOARCH }}-
+ go-build-v1-
- name: Cache Go Build (restore)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider whether the generic `go-build-v1-` restore key could lead to suboptimal or incorrect cache reuse across jobs/architectures.
This broader restore key allows caches from other jobs, architectures, or Go versions to be reused, which can both reduce cache effectiveness (due to mismatches) and introduce subtle, hard-to-debug issues from stale artifacts. Consider scoping the fallback key by at least GOARCH (e.g. `go-build-v1-${{ steps.cache-paths.outputs.GOARCH }}-`) and possibly Go version so fallback restores stay within compatible builds.
Suggested implementation:
```
key: go-build-v1-${{ github.job }}-${{ steps.cache-paths.outputs.GOARCH }}-${{ steps.cache-paths.outputs.TAG }}
restore-keys: |
go-build-v1-${{ github.job }}-${{ steps.cache-paths.outputs.GOARCH }}-
go-build-v1-${{ steps.cache-paths.outputs.GOARCH }}-
```
```
key: go-build-v1-${{ github.job }}-${{ steps.cache-paths.outputs.GOARCH }}-${{ steps.cache-paths.outputs.TAG }}
restore-keys: |
go-build-v1-${{ github.job }}-${{ steps.cache-paths.outputs.GOARCH }}-
go-build-v1-${{ steps.cache-paths.outputs.GOARCH }}-
```
If you want to further scope by Go version, you can:
1. Add an output like `GOVERSION` in `cache-paths` (or another prior step), and
2. Extend the fallback key to include it, e.g. `go-build-v1-${{ steps.cache-paths.outputs.GOARCH }}-${{ steps.cache-paths.outputs.GOVERSION }}-`.
Ensure that `GOVERSION` is stable and accurately reflects the toolchain version used for the build.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
With increased cache limits, there's no reason to skip saves for test and style jobs. Re-enable saves on master for all jobs (~19 GB/week, ~76-95 GB over 30 days). Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2a9ce1f to
bbbb67e
Compare
|
@janisz now that we have more gha cache space, we can re-enable the cache saves I had disabled. I still think we should keep the gha caches only saved on the master branch since branch jobs cannot load caches saved by other non-master branches and the keys must be unique (they are not overwritten, and so they aren't replaced by master saves until expired/purged). |
|
Images are ready for the commit at 45ea6ab. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19069 +/- ##
==========================================
- Coverage 49.52% 49.51% -0.01%
==========================================
Files 2670 2670
Lines 201549 201549
==========================================
- Hits 99811 99803 -8
- Misses 94287 94293 +6
- Partials 7451 7453 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
correction: "the uniqueness constraint is key + ref, not just key" |
This reverts commit 74c8e05.
…files PR #19069 removed all save: false, but check-generated-files modifies the working tree (regenerates protos, removes mocks) which breaks hashFiles('**/go.sum') in the GOMODCACHE post-step, causing cache save failures. Reverted in #19108. Re-enable saves for all other jobs (unit-tests, style-check, scanner-db-integration-tests) while keeping save: false only for check-generated-files where the hashFiles failure occurs. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
With increased cache limits, there's no reason to skip saves for test and style jobs. Re-enable saves on master for all jobs (~19 GB/week, ~76-95 GB over 30 days).
Partially generated by AI.
Testing and quality
How I validated my change
change me!