Skip to content

fix(ci): GHA cache keys unique vs shared#19580

Merged
davdhacs merged 6 commits intomasterfrom
davdhacs/fix-cache-key-separation
Mar 31, 2026
Merged

fix(ci): GHA cache keys unique vs shared#19580
davdhacs merged 6 commits intomasterfrom
davdhacs/fix-cache-key-separation

Conversation

@davdhacs
Copy link
Copy Markdown
Contributor

@davdhacs davdhacs commented Mar 24, 2026

Fix key issues for GHA cache of GOCACHE:

unit-tests.yaml: add key-suffix for GOTAGS matrix

go and go-postgres have GOTAGS matrix ("" / release) sharing one cache key. Each variant's trim deletes ~3GB of the other's entries every run. Fix: add key-suffix: ${{ matrix.gotags }}.

scanner-build.yaml: set GOARCH on cache step

pre-build-scanner-go-binary cross-compiles for 4 architectures but doesn't set GOARCH on the cache step. go env GOARCH returns the runner's native amd64, so all arches share one cache key with incompatible entries. Fix: add env: GOARCH: ${{ matrix.goarch }}.

build.yaml: only separate race-condition-debug

pre-build-go-binaries: default and prerelease share 93% of GOCACHE entries (only 6 packages use release build tags). Only separate race-condition-debug (-race/CGO_ENABLED=1, 39% unique).

Verification

Local GOCACHE entry overlap measurements:

Variant New entries vs default Unique % Action
prerelease (GOTAGS=release) +691 7% share
race (-race, CGO_ENABLED=1) +6300 39% separate
GOTAGS="" vs release (tests) +691 7% separate (trim causes 3GB/run thrashing)

All CI passed (Build, Unit Tests, Style, Scanner).

Partially generated by AI.

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Mar 24, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • Using matrix.name == 'race-condition-debug' && 'race' || '' relies on GitHub’s truthy/falsey behavior and is a bit opaque; consider moving the condition into the matrix definition (e.g., a race: true flag) or a small reusable expression to make the cache key logic more readable and less brittle to matrix-name changes.
  • For the pre-build-go-binaries job, double-check that passing an empty string as key-suffix is treated the same by .github/actions/cache-go-dependencies as omitting the input entirely (as done in build-and-push-operator), or adjust the action so unset vs empty suffix behaves consistently.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Using `matrix.name == 'race-condition-debug' && 'race' || ''` relies on GitHub’s truthy/falsey behavior and is a bit opaque; consider moving the condition into the matrix definition (e.g., a `race: true` flag) or a small reusable expression to make the cache key logic more readable and less brittle to matrix-name changes.
- For the `pre-build-go-binaries` job, double-check that passing an empty string as `key-suffix` is treated the same by `.github/actions/cache-go-dependencies` as omitting the input entirely (as done in `build-and-push-operator`), or adjust the action so unset vs empty suffix behaves consistently.

## Individual Comments

### Comment 1
<location path=".github/workflows/build.yaml" line_range="213" />
<code_context>
         uses: ./.github/actions/cache-go-dependencies
         with:
-          key-suffix: ${{ matrix.name }}
+          key-suffix: ${{ matrix.name == 'race-condition-debug' && 'race' || '' }}

       - uses: ./.github/actions/handle-tagged-build
</code_context>
<issue_to_address>
**suggestion:** Consider making this conditional expression more explicit to avoid ambiguity in evaluation and future edits.

GitHub Actions’ `&&`/`||` precedence and truthiness can be hard to parse. Adding parentheses like `${{ (matrix.name == 'race-condition-debug' && 'race') || '' }}` makes the grouping clearer. If the action distinguishes `null` from `''`, you could instead use `${{ matrix.name == 'race-condition-debug' && 'race' || null }}` to signal that the suffix is intentionally omitted.

```suggestion
          key-suffix: ${{ (matrix.name == 'race-condition-debug' && 'race') || '' }}
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented Mar 24, 2026

Images are ready for the commit at f5bcf9d.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-504-gf5bcf9dab3.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.65%. Comparing base (44d74f3) to head (f5bcf9d).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19580      +/-   ##
==========================================
- Coverage   49.67%   49.65%   -0.02%     
==========================================
  Files        2747     2747              
  Lines      207296   207296              
==========================================
- Hits       102964   102937      -27     
- Misses      96683    96703      +20     
- Partials     7649     7656       +7     
Flag Coverage Δ
go-unit-tests 49.65% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@davdhacs
Copy link
Copy Markdown
Contributor Author

@sourcery-ai review

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The GitHub Actions expression ${{ (matrix.name == 'race-condition-debug' && 'race') || '' }} is a bit terse; consider using a more explicit pattern (e.g., the if function or a short comment) so future readers immediately understand that only the race build gets a distinct cache key.
  • In scanner-build.yaml, you set GOARCH only on the cache step; if any later Go steps rely on go env GOARCH, it may be safer to set GOARCH at the job level (or at least consistently on all Go-related steps) to avoid subtle cross-arch issues.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The GitHub Actions expression `${{ (matrix.name == 'race-condition-debug' && 'race') || '' }}` is a bit terse; consider using a more explicit pattern (e.g., the `if` function or a short comment) so future readers immediately understand that only the race build gets a distinct cache key.
- In `scanner-build.yaml`, you set `GOARCH` only on the cache step; if any later Go steps rely on `go env GOARCH`, it may be safer to set `GOARCH` at the job level (or at least consistently on all Go-related steps) to avoid subtle cross-arch issues.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@davdhacs davdhacs changed the title perf(ci): stop over-separating GOCACHE for build matrix variants fix(ci): GHA cache keys unique vs shared Mar 25, 2026
@davdhacs davdhacs marked this pull request as ready for review March 25, 2026 04:09
@davdhacs davdhacs requested a review from a team as a code owner March 25, 2026 04:09
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The conditional expression for key-suffix in build.yaml (${{ (matrix.name == 'race-condition-debug' && 'race') || '' }}) is a bit opaque and tightly coupled to the matrix name; consider either using a more explicit mapping or a dedicated matrix field (e.g. cacheKeySuffix) to make future variant changes less error-prone.
  • In unit-tests.yaml, using matrix.gotags directly as the key-suffix assumes only '' and release; if more complex tag sets are introduced later it may be safer to normalize or hash them to avoid unexpected characters in cache keys or collisions between equivalent tag permutations.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The conditional expression for `key-suffix` in `build.yaml` (`${{ (matrix.name == 'race-condition-debug' && 'race') || '' }}`) is a bit opaque and tightly coupled to the matrix name; consider either using a more explicit mapping or a dedicated matrix field (e.g. `cacheKeySuffix`) to make future variant changes less error-prone.
- In `unit-tests.yaml`, using `matrix.gotags` directly as the `key-suffix` assumes only `''` and `release`; if more complex tag sets are introduced later it may be safer to normalize or hash them to avoid unexpected characters in cache keys or collisions between equivalent tag permutations.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

davdhacs and others added 5 commits March 27, 2026 23:00
… entries

pre-build-go-binaries: default and prerelease (GOTAGS=release) share 93%
of cache entries (only 6 packages use release build tags). Separating them
doubles cache storage and causes the trim to delete shared entries. Keep
key-suffix only for race-condition-debug which uses -race/CGO_ENABLED=1
and has 39% unique entries.

build-and-push-operator: branding (RHACS vs STACKROX) is a runtime env
var, not a build tag. Compiled output is identical. Remove key-suffix
entirely.

Verified locally:
  default:    8911 entries (baseline)
  prerelease: +691 new (7% unique, 93% shared)
  race:       +6300 new (39% unique, 61% shared)
  branding:   +0 new (0% unique, 100% shared)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
unit-tests.yaml: go and go-postgres have GOTAGS matrix (""/ release)
sharing one cache key. With the GOCACHE trim, each variant deletes
the other's entries on every run, causing 3GB of churn. Add key-suffix
to give each variant its own cache.

scanner-build.yaml: pre-build-scanner-go-binary cross-compiles for
multiple architectures but doesn't set GOARCH on the cache step,
so all arches share one amd64-labeled cache key. Add env GOARCH.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address sourcery-ai review: make GHA expression grouping explicit.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The operator branding key-suffix is unnecessary for GOCACHE (0% unique
entries), but PR #19417 will unify operator builds entirely. Removing
the key-suffix here would create a merge conflict with that PR.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address sourcery-ai feedback: clarify why only race-condition-debug
gets a separate cache key (39% unique entries from -race/CGO_ENABLED=1)
while default and prerelease share (93% overlap).

GOARCH is intentionally set only on the cache step, not job-level,
to avoid changing the default arch for other steps.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 691161d4-d704-4ed9-948a-725a26eed3f2

📥 Commits

Reviewing files that changed from the base of the PR and between 7072ab4 and b89b929.

📒 Files selected for processing (3)
  • .github/workflows/build.yaml
  • .github/workflows/scanner-build.yaml
  • .github/workflows/unit-tests.yaml

📝 Walkthrough

Summary by CodeRabbit

  • Chores
    • Optimized Go dependency caching in build pipelines to improve build efficiency across different build configurations and test variants.

Walkthrough

Three GitHub Actions workflows were updated to refine Go dependency cache key partitioning: build.yaml now conditionally assigns cache suffixes based on matrix values, scanner-build.yaml explicitly passes the GOARCH environment variable to the cache step, and unit-tests.yaml uses matrix.gotags as a cache key suffix for test variants.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow Cache Configuration
.github/workflows/build.yaml, .github/workflows/scanner-build.yaml, .github/workflows/unit-tests.yaml
Modified Go dependency cache strategies: build.yaml conditionally sets cache suffix to race only for race-condition-debug matrix values; scanner-build.yaml adds GOARCH environment variable to cache step; unit-tests.yaml partitions cache by matrix.gotags values across go and go-postgres jobs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(ci): GHA cache keys unique vs shared' directly and concisely summarizes the main change: fixing GitHub Actions cache key separation strategy across multiple workflow files.
Description check ✅ Passed The PR description is comprehensive and well-structured, covering the specific cache key issues fixed, verification measurements, and CI results, but the template's required sections for user-facing documentation and testing are not formally completed.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch davdhacs/fix-cache-key-separation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@davdhacs davdhacs force-pushed the davdhacs/fix-cache-key-separation branch from b89b929 to f5bcf9d Compare March 31, 2026 15:21
@davdhacs davdhacs merged commit 5b93e61 into master Mar 31, 2026
104 checks passed
@davdhacs davdhacs deleted the davdhacs/fix-cache-key-separation branch March 31, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants