UPSTREAM: <carry>: Add support for optional prebuilt Go binaries in Dockerfiles #183
Conversation
WalkthroughConditional logic was added to several backend Dockerfiles to allow the use of a prebuilt binary via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Docker Build
participant Source as Source Code
participant Prebuilt as Prebuilt Binary (optional)
participant Image as Docker Image
Builder->>Builder: Check PREBUILT_BINARY argument
alt PREBUILT_BINARY is unset
Builder->>Source: Build binary from source
Source->>Image: Provide built binary
else PREBUILT_BINARY is set
Builder->>Prebuilt: Copy prebuilt binary
Prebuilt->>Image: Provide prebuilt binary
end
Image->>Builder: Finalize image with binary
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Change to PR detected. A new PR build was completed. |
hbelmiro
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
backend/Dockerfile.scheduledworkflow (1)
44-45: Duplicate: default sentinel value for PREBUILT_BINARY
See the note inbackend/Dockerfile.launcherabout usingARG PREBUILT_BINARY=""and checking-n "$PREBUILT_BINARY"rather than"unset".backend/Dockerfile (1)
34-36: Duplicate: default sentinel value for PREBUILT_BINARY
Align with other components by usingARG PREBUILT_BINARY=""and testing-n "$PREBUILT_BINARY"to avoid literal collisions with"unset".backend/Dockerfile.driver (1)
37-38: Duplicate: default sentinel value for PREBUILT_BINARY
As above, default to""and use-n "$PREBUILT_BINARY"instead of the literal"unset".backend/Dockerfile.persistenceagent (1)
39-40: Duplicate: default sentinel value for PREBUILT_BINARY
Consistent with other Dockerfiles, switch toARG PREBUILT_BINARY=""and check-n "$PREBUILT_BINARY"to avoid sentinel collisions.
🧹 Nitpick comments (1)
backend/Dockerfile.launcher (1)
39-41: Consider using an empty default for PREBUILT_BINARY
Using the literal"unset"risks colliding with a binary actually namedunset. It’s more robust to default to""and detect with-n, e.g.:ARG PREBUILT_BINARY="" … RUN if [ -n "$PREBUILT_BINARY" ]; then …; else …; fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/Dockerfile(1 hunks)backend/Dockerfile.driver(1 hunks)backend/Dockerfile.launcher(1 hunks)backend/Dockerfile.persistenceagent(1 hunks)backend/Dockerfile.scheduledworkflow(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: API integration tests v2 - K8s with kubernetes v1.29.2
- GitHub Check: API integration tests v2 - K8s with database v1.29.2
- GitHub Check: Basic Sample Tests - K8s v1.29.2
- GitHub Check: API integration tests v2 - K8s with kubernetes v1.31.0
- GitHub Check: Basic Sample Tests - K8s v1.31.0
- GitHub Check: API integration tests v2 - K8s with database v1.31.0
- GitHub Check: API integration tests v2 with proxy - K8s v1.31.0
- GitHub Check: check_ci_status
| RUN if [ "$PREBUILT_BINARY" = "unset" ]; then \ | ||
| GO111MODULE=on CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -tags netgo -ldflags '-extldflags "-static"' -o /bin/launcher-v2 ./backend/src/v2/cmd/launcher-v2/*.go && \ | ||
| GO111MODULE=on CGO_ENABLED=1 GOOS=linux GOARCH=amd64 GOEXPERIMENT=strictfipsruntime go build -tags 'netgo strictfipsruntime' -o /bin/launcher-v2-fips ./backend/src/v2/cmd/launcher-v2/*.go ;\ | ||
| else \ | ||
| cp "${PREBUILT_BINARY}" /bin/launcher-v2 && \ | ||
| cp "${PREBUILT_BINARY}" /bin/launcher-v2-fips ;\ | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit existence check and clear error for the prebuilt binary
Relying on cp alone yields a generic failure if the path is wrong. To improve diagnostics, wrap the copy in a file‐test, for example:
if [ -f "$PREBUILT_BINARY" ]; then \
cp "$PREBUILT_BINARY" /bin/launcher-v2 && cp "$PREBUILT_BINARY" /bin/launcher-v2-fips; \
else \
echo "Error: PREBUILT_BINARY '$PREBUILT_BINARY' not found" >&2; exit 1; \
fi🤖 Prompt for AI Agents
In backend/Dockerfile.launcher around lines 42 to 48, the current script copies
the prebuilt binary without checking if the file exists, causing unclear errors
if the path is wrong. Modify the else block to first check if the file at
PREBUILT_BINARY exists using a file test. If it exists, proceed with copying to
/bin/launcher-v2 and /bin/launcher-v2-fips; if not, output a clear error message
to stderr and exit with status 1 to improve diagnostics.
| RUN if [ "$PREBUILT_BINARY" = "unset" ]; then \ | ||
| GO111MODULE=on CGO_ENABLED=1 GOEXPERIMENT=strictfipsruntime go build -tags strictfipsruntime -o /bin/controller backend/src/crd/controller/scheduledworkflow/*.go; \ | ||
| else \ | ||
| cp ${PREBUILT_BINARY} /bin/controller; \ | ||
| fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen the prebuilt-binary copy step
- Quote the path to handle spaces:
cp "${PREBUILT_BINARY}" /bin/controller - Add a
[ -f ]test beforecpto surface a clear “file not found” error instead of a crypticcpfailure.
🤖 Prompt for AI Agents
In backend/Dockerfile.scheduledworkflow around lines 47 to 52, the prebuilt
binary copy step should be improved by quoting the PREBUILT_BINARY variable to
handle paths with spaces and adding a file existence check before copying.
Modify the else branch to first test if the file exists using [ -f
"${PREBUILT_BINARY}" ] and if not, output a clear error message and exit; if it
exists, then copy it with cp "${PREBUILT_BINARY}" /bin/controller.
| # If PREBUILT_BINARY is set, use it; otherwise, build from source | ||
| RUN if [ "$PREBUILT_BINARY" = "unset" ]; then \ | ||
| GO111MODULE=on CGO_ENABLED=1 GOOS=linux GOARCH=amd64 GOEXPERIMENT=strictfipsruntime go build -tags 'netgo strictfipsruntime' -o /bin/driver ./backend/src/v2/cmd/driver/*.go; \ | ||
| else \ | ||
| cp ${PREBUILT_BINARY} /bin/driver; \ | ||
| fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve prebuilt-binary copy logic
- Wrap the path in quotes:
cp "${PREBUILT_BINARY}" /bin/driver - Pre-check with
[ -f "$PREBUILT_BINARY" ]to fail fast with a clear error if missing.
🤖 Prompt for AI Agents
In backend/Dockerfile.driver around lines 39 to 45, the copy command for the
prebuilt binary should be improved by wrapping the PREBUILT_BINARY variable in
quotes to handle paths with spaces, and adding a pre-check using [ -f
"$PREBUILT_BINARY" ] to verify the file exists before copying. If the file is
missing, the build should fail immediately with a clear error message. Update
the RUN command to include these checks and quoted variable usage.
| # If PREBUILT_BINARY is set, use it; otherwise, build from source | ||
| RUN if [ "$PREBUILT_BINARY" = "unset" ]; then \ | ||
| GO111MODULE=on CGO_ENABLED=1 GOEXPERIMENT=strictfipsruntime go build -tags strictfipsruntime -o /bin/persistence_agent backend/src/agent/persistence/*.go; \ | ||
| else \ | ||
| cp ${PREBUILT_BINARY} /bin/persistence_agent; \ | ||
| fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Robustify the prebuilt-binary copy step
- Quote the argument:
cp "${PREBUILT_BINARY}" /bin/persistence_agent - Add an existence check to emit a clear “file not found” error before attempting the copy.
🤖 Prompt for AI Agents
In backend/Dockerfile.persistenceagent around lines 41 to 47, the cp command
copying the prebuilt binary should quote the variable to handle paths with
spaces by using cp "${PREBUILT_BINARY}" /bin/persistence_agent. Additionally,
add a check before the copy to verify the file exists, and if not, output a
clear error message and exit to prevent silent failures.
…ockerfiles Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>
hbelmiro
changed the title
hbelmiro
changed the title
|
Change to PR detected. A new PR build was completed. |
Signed-off-by: Helber Belmiro <helber.belmiro@gmail.com>
|
Change to PR detected. A new PR build was completed. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alyssacgoins The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description of your changes:
This commit adds support for optional prebuilt Go binaries in Dockerfiles using a
PREBUILT_BINARYargument. It allows developers to skip source builds and use prebuilt binaries, serving as a workaround for issues like cross-platform builds. The default behavior remains unchanged for compatibility.For example, one can build the binary with:
CC=x86_64-unknown-linux-gnu-gcc GO111MODULE=on CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -o controller backend/src/crd/controller/scheduledworkflow/*.goThen build the Docker image using the prebuilt binary:
Checklist:
Summary by CodeRabbit