Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the operator Dockerfile you hard-code default cache paths (
/workspace/pkg/modand/root/.cache/go-build); consider wiring these togo env GOMODCACHE/go env GOCACHE(or at least documenting/aligning with the base image’s defaults) to avoid subtle breakage if the Go toolchain layout changes. - The Docker layer cache key in the workflow uses
hashFiles('**/go.sum'), so any go module change anywhere in the repo will invalidate the operator cache; if you want tighter caching, consider scoping this to the operator-relatedgo.sumfiles (for examplego.sumandoperator/go.sum).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the operator Dockerfile you hard-code default cache paths (`/workspace/pkg/mod` and `/root/.cache/go-build`); consider wiring these to `go env GOMODCACHE`/`go env GOCACHE` (or at least documenting/aligning with the base image’s defaults) to avoid subtle breakage if the Go toolchain layout changes.
- The Docker layer cache key in the workflow uses `hashFiles('**/go.sum')`, so any go module change anywhere in the repo will invalidate the operator cache; if you want tighter caching, consider scoping this to the operator-related `go.sum` files (for example `go.sum` and `operator/go.sum`).
## Individual Comments
### Comment 1
<location path=".github/workflows/build.yaml" line_range="627-628" />
<code_context>
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
+ with:
+ driver-opts: |
+ image=moby/buildkit:latest
+
+ - name: Cache Docker layers
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `moby/buildkit:latest` can introduce non-determinism and unexpected breakage in CI builds.
Pulling `moby/buildkit:latest` makes CI behavior change as the image updates, which can cause flaky, hard-to-reproduce failures. Please pin to a specific BuildKit tag or digest so builds are deterministic and upgrades are controlled.
Suggested implementation:
```
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
with:
driver-opts: |
image=moby/buildkit:v0.15.1
```
If your project has a documented set of pinned tool versions (e.g., in a README or tooling matrix), you may want to:
1. Align the `moby/buildkit` version here (`v0.15.1`) with whatever version is documented there.
2. Optionally further harden determinism by using a digest (`image=moby/buildkit@sha256:...`) instead of a tag, and updating it intentionally when upgrading BuildKit.
</issue_to_address>
### Comment 2
<location path="operator/Makefile" line_range="363-364" />
<code_context>
+ DOCKER_BUILDKIT=1 BUILDKIT_PROGRESS=plain ../scripts/docker-build.sh \
-t ${IMG} \
$(if $(GOARCH),--build-arg TARGET_ARCH=$(GOARCH)) \
+ --build-arg GOMODCACHE_PATH=$(shell go env GOMODCACHE) \
+ --build-arg GOCACHE_PATH=$(shell go env GOCACHE) \
+ $(if $(BUILDKIT_CACHE_FROM),--cache-from $(BUILDKIT_CACHE_FROM)) \
+ $(if $(BUILDKIT_CACHE_TO),--cache-to $(BUILDKIT_CACHE_TO)) \
</code_context>
<issue_to_address>
**issue (performance):** Passing host `go env` paths into the container may misalign cache paths with the container’s Go configuration.
Inside the Docker build, GOPATH is `/workspace`, so the container’s natural `GOMODCACHE`/`GOCACHE` differ from the host’s (e.g. `/home/runner/...`). Overriding Dockerfile defaults with host-derived paths means cache mounts may not match where `go mod download` writes, reducing cache effectiveness or sending writes to unexpected locations. Prefer using the Dockerfile defaults or deriving these paths inside the container from its own `go env` instead of the host’s.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
306d14e to
73138a1
Compare
|
Images are ready for the commit at 30c697d. 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 #19417 +/- ##
=======================================
Coverage 49.28% 49.28%
=======================================
Files 2735 2735
Lines 206215 206215
=======================================
+ Hits 101633 101635 +2
+ Misses 97041 97038 -3
- Partials 7541 7542 +1
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:
|
|
I think it will be easier if operator follow the same build style as main or roxctl instead of using docker alternatively we can use docker with cache but then the whole job should be dockerized. |
e533da3 to
9f7755b
Compare
9f7755b to
b6d2be9
Compare
f2ea7fc to
581ba43
Compare
dee2c85 to
28d5326
Compare
Remove explicit fetch-depth: 1 as it's the default value for actions/checkout@v6. Only jobs that need full history (define-job-matrix and a few others) explicitly set fetch-depth: 0. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fix build failures with shallow clones by using Makefile environment variables instead of git commands. Changes: - define-job-matrix: Compute short commit and export as output - All jobs: Use BUILD_TAG instead of TAG (TAG is cleared for security) - All jobs: Add SHORTCOMMIT env var to avoid git rev-parse failures How it works: - make/env.mk checks BUILD_TAG env var (line 58-59) before git describe - Makefile checks SHORTCOMMIT env var before git rev-parse - This allows builds to work with shallow clones (no git history needed) Benefits: - pre-build-go-binaries can now use shallow clones - Saves ~12s × 2-4 matrix jobs = 24-48s - Total workflow savings: ~72-96s per run Fixes: ROX-33792 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
b29a666 to
0e29168
Compare
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Changed step output key from 'sha' to 'shortcommit' for consistency with the SHORTCOMMIT environment variable name used throughout the workflow. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changed output name from 'main-tag' to 'build-tag' to match the BUILD_TAG environment variable used throughout the workflow. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
Replace manual if/else logic with existing registry_from_branding function from lib.sh. This improves code maintainability by reusing the centralized registry mapping logic. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
Fixes critical issues found in code review: 1. Use docker-build-prebuilt target to leverage prebuilt binaries - Changed from docker-build which rebuilds inside Docker - This enables the intended optimization of building once per arch 2. Add missing 'fi' to close if statement in scan-images workflow - Fixes bash syntax error that would cause CI failures 3. Remove duplicate sourcing of lib.sh - Already sourced at top of script 4. Remove unused 'image' variable - Dead code cleanup AI-assisted code review and fixes.
Fixes test failures where development builds were incorrectly compiled as release builds. Root cause: - make/env.mk checks if BUILD_TAG is set in CI to determine release builds - Our shallow clone optimization now sets BUILD_TAG for ALL builds - This caused ALL builds to be compiled with GOTAGS=release Solution: - Check if BUILD_TAG contains '.x' (development tag marker) - Only set GOTAGS=release for clean version tags without '.x' Examples: - 4.7.x-36-gc2253ef650 → development build (GOTAGS empty) - 4.6.0 → release build (GOTAGS=release) - 4.11.x-nightly-20260325 → development build (GOTAGS empty) This fixes E2E test failures: - TestMetadataIsSetCorrectly expecting buildFlavor=development - TestFeatureFlagSettings expecting development defaults AI-assisted debugging and fix.
|
/konflux-retest scanner-v4-db-on-push |
|
/konflux-retest central-db-on-push |
|
/konflux-retest operator-bundle-on-push |
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>
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>
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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>
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>
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>
|
Closing as it was splitted and merged |
…ecture Build roxctl and roxagent together in a single go build invocation for each Linux architecture, following the pattern from main-build-nodeps. Problem: - CLI builds invoked go build separately for roxctl and roxagent on each architecture - 8 separate build invocations for Linux (4 roxctl + 4 roxagent) - Go toolchain restarted for each binary - Common dependencies recompiled multiple times - No build cache sharing within same architecture Solution: - Created cli_linux-% pattern rule that builds both binaries together - Single go build with multiple package paths: ./roxctl ./compliance/virtualmachines/roxagent - roxctl-linux and roxagent-linux now depend on combined targets - Preserves existing pattern rules for backward compatibility and darwin/windows builds Performance (warm cache, 4 Linux architectures): - Old approach (separate builds): 10m 6s - New approach (sequential): 2m 18s → 4.4x faster - New approach (make -j 2): 1m 6s → 9.2x faster - New approach (make -j 4): 37s → 16.4x faster Similar pattern used in: - main-build-nodeps: builds 9 binaries in one invocation - PR #19417: consolidated operator build Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Issues:
Solutions:
ubuntu-latestinstead of custom containerBefore:

Afeter:

Note: Changes include AI-assisted code generation and optimization.