ROX-33792: build operator binary outside Docker for faster builds#19655
ROX-33792: build operator binary outside Docker for faster builds#19655
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
docker-buildtarget’s CI detection relies onCIbeing exactly"true"; consider relaxing this to a non-empty check (e.g.,[ -n "$$CI" ]) or documenting the expectation, to avoid surprises in other CI environments. - The new prebuilt flow assumes
bin/linux_${GOARCH}/stackrox-operatoris present in the Docker build context and not filtered by.dockerignore/.containerignore; it would be good to double‑check or explicitly comment in those files that this binary must remain included forprebuilt.Dockerfileto work.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `docker-build` target’s CI detection relies on `CI` being exactly `"true"`; consider relaxing this to a non-empty check (e.g., `[ -n "$$CI" ]`) or documenting the expectation, to avoid surprises in other CI environments.
- The new prebuilt flow assumes `bin/linux_${GOARCH}/stackrox-operator` is present in the Docker build context and not filtered by `.dockerignore`/`.containerignore`; it would be good to double‑check or explicitly comment in those files that this binary must remain included for `prebuilt.Dockerfile` to work.
## Individual Comments
### Comment 1
<location path="operator/Makefile" line_range="393-394" />
<code_context>
+ -f $< \
+ ..
+
+.PHONY: docker-build-prebuilt
+docker-build-prebuilt: prebuilt.Dockerfile ## Build docker image with pre-built operator binary.
BUILDKIT_PROGRESS=plain ../scripts/docker-build.sh \
-t ${IMG} \
</code_context>
<issue_to_address>
**🚨 issue (security):** Align docker-build-prebuilt prerequisites with docker-build-full (e.g., smuggled-status-sh) to preserve the same checks.
After the refactor, `docker-build`’s former prerequisite chain (`build/Dockerfile.gen smuggled-status-sh`) only exists on `docker-build-full`, while `docker-build-prebuilt` depends solely on `prebuilt.Dockerfile`. In CI the prebuilt path will run most often, so any checks or side effects in `smuggled-status-sh` (e.g., license/provenance validation) will be skipped. Please add `smuggled-status-sh` (and any other needed prerequisites) to `docker-build-prebuilt`, or extract them into a shared prerequisite so both build modes run the same checks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .PHONY: docker-build-prebuilt | ||
| docker-build-prebuilt: prebuilt.Dockerfile ## Build docker image with pre-built operator binary. |
There was a problem hiding this comment.
🚨 issue (security): Align docker-build-prebuilt prerequisites with docker-build-full (e.g., smuggled-status-sh) to preserve the same checks.
After the refactor, docker-build’s former prerequisite chain (build/Dockerfile.gen smuggled-status-sh) only exists on docker-build-full, while docker-build-prebuilt depends solely on prebuilt.Dockerfile. In CI the prebuilt path will run most often, so any checks or side effects in smuggled-status-sh (e.g., license/provenance validation) will be skipped. Please add smuggled-status-sh (and any other needed prerequisites) to docker-build-prebuilt, or extract them into a shared prerequisite so both build modes run the same checks.
41e8b4b to
65aada2
Compare
|
Images are ready for the commit at 7d9eafc. To use with deploy scripts, first |
65aada2 to
33aae4a
Compare
Build operator binary once in pre-build-go-binaries job alongside other Go binaries, then reuse it when building Docker images. This eliminates duplicate compilation and leverages GitHub Actions' Go module cache. Changes: - Add operator/cmd to main-build-nodeps in root Makefile - Create operator/prebuilt.Dockerfile for CI builds - Add conditional docker-build targets in operator/Makefile that use prebuilt binaries in CI mode, fall back to in-Docker build locally - Allow operator binaries in .dockerignore and .containerignore - Update build-and-push-operator job to download prebuilt binaries and skip proto resolution/duplicate builds Benefits: - ~2-5 min savings from eliminating duplicate Go compilation - Better cache utilization via GitHub Actions' Go module cache - Smaller Docker build context - Fully backwards compatible (local builds unchanged) Related: ROX-33792 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
33aae4a to
7d9eafc
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19655 +/- ##
==========================================
+ Coverage 49.34% 49.37% +0.03%
==========================================
Files 2743 2743
Lines 207019 207020 +1
==========================================
+ Hits 102152 102218 +66
+ Misses 97269 97218 -51
+ Partials 7598 7584 -14
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:
|
|
|
||
| **/node_modules/ | ||
| /bin/ | ||
| !/bin/linux_*/stackrox-operator |
There was a problem hiding this comment.
How about putting these into an operator-specific operator/prebuilt.Dockerfile.dockerignore ?
davdhacs
left a comment
There was a problem hiding this comment.
Looks great. My only request is to remove the stackrox_operator exclude and put it into a .dockerignore that is only used for the prebuilt-operator.
Description
Build operator binary once in pre-build-go-binaries job alongside other Go binaries, then reuse it when building Docker images. This eliminates duplicate compilation and leverages GitHub Actions' Go module cache and Go builder for multiple binaries.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
CI