NO-JIRA: refactor(Dockerfile.cpu): wrap multiple RUN commands with bash for improved readability and error handling #2645
Conversation
…sh for improved readability and error handling
github-actions
bot
added
the
review-requested
WalkthroughReplaced many Dockerfile multi-command RUN lines with /bin/bash heredoc blocks enabling strict shell options ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
🔇 Additional comments (4)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
codeserver/ubi9-python-3.12/Dockerfile.cpu (1)
58-66: Verify verbose logging side effects fromset -x.The
set -xflag in heredocs will echo every executed command to build logs. This improves debuggability but may produce verbose output. Confirm this is acceptable for your CI/CD pipeline, especially for sensitive operations.If verbose output becomes problematic, consider conditionally enabling it (e.g.,
set -Eeuo pipefailwithout-x), or document this behavior in build logs.Also applies to: 90-94, 98-103
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codeserver/ubi9-python-3.12/Dockerfile.cpu(7 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code static analysis
codeserver/ubi9-python-3.12/Dockerfile.cpu
[error] 95-95: Hadolint reported a syntax issue in Dockerfile.cpu: unexpected '#' at line 95 (expected a new instruction). Process exited with code 123.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (codeserver-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (codeserver-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: Red Hat Konflux / odh-workbench-codeserver-datascience-cpu-py312-ubi9-on-pull-request
🔇 Additional comments (3)
codeserver/ubi9-python-3.12/Dockerfile.cpu (3)
58-66: Cache mount syntax and stage references look correct.All
--mount=type=cachedeclarations are properly formatted and correctly reference build stages where applicable. Therwflags enable write access for cache population as intended.Also applies to: 106-111, 174-184, 291-309
79-127: USER context preserved correctly across refactored RUN commands.Each heredoc executes in the context of the preceding USER directive. Root operations remain under USER 0, and user operations under USER 1001. No regressions detected.
121-128: Fix comment placement after oc client heredoc.Line 128's comment must not follow the heredoc EOF. Apply the same fix: move the comment before the RUN instruction or integrate it into a preceding section comment.
-EOF -# Install the oc client end +EOF + +# Install the oc client endPlace the "end" comment as a section delimiter before any subsequent RUN or other instruction.
Likely an incorrect or invalid review comment.
| RUN /bin/bash <<'EOF' | ||
| set -Eeuxo pipefail | ||
| dnf -y upgrade --refresh --nobest --skip-broken --nodocs --noplugins --setopt=install_weak_deps=0 --setopt=keepcache=0 | ||
| dnf clean all -y | ||
| EOF | ||
| # upgrade first to avoid fixable vulnerabilities end |
There was a problem hiding this comment.
CRITICAL: Fix Hadolint syntax error—comments cannot follow heredoc EOF.
The comment at line 95 appears immediately after the heredoc EOF, which violates Dockerfile syntax. Hadolint requires a Dockerfile instruction keyword (not a comment) after RUN ends. This same issue occurs at lines 127-128 and 296-297.
Move section-end comments before the corresponding RUN instruction instead.
Apply this diff to relocate the comment:
-# upgrade first to avoid fixable vulnerabilities begin
-# Problem: The operation would result in removing the following protected packages: systemd
-# (try to add '--allowerasing' to command line to replace conflicting packages or '--skip-broken' to skip uninstallable packages)
-# Solution: --best --skip-broken does not work either, so use --nobest
RUN /bin/bash <<'EOF'
set -Eeuxo pipefail
dnf -y upgrade --refresh --nobest --skip-broken --nodocs --noplugins --setopt=install_weak_deps=0 --setopt=keepcache=0
dnf clean all -y
EOF
-# upgrade first to avoid fixable vulnerabilities end
+
+# upgrade first to avoid fixable vulnerabilities endRelocate the "end" comment immediately BEFORE the next RUN instruction (line 96) as a section delimiter, or consolidate with the section-start comment above the RUN block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN /bin/bash <<'EOF' | |
| set -Eeuxo pipefail | |
| dnf -y upgrade --refresh --nobest --skip-broken --nodocs --noplugins --setopt=install_weak_deps=0 --setopt=keepcache=0 | |
| dnf clean all -y | |
| EOF | |
| # upgrade first to avoid fixable vulnerabilities end | |
| # upgrade first to avoid fixable vulnerabilities end | |
| RUN /bin/bash <<'EOF' | |
| set -Eeuxo pipefail | |
| dnf -y upgrade --refresh --nobest --skip-broken --nodocs --noplugins --setopt=install_weak_deps=0 --setopt=keepcache=0 | |
| dnf clean all -y | |
| EOF |
🧰 Tools
🪛 GitHub Actions: Code static analysis
[error] 95-95: Hadolint reported a syntax issue in Dockerfile.cpu: unexpected '#' at line 95 (expected a new instruction). Process exited with code 123.
🤖 Prompt for AI Agents
In codeserver/ubi9-python-3.12/Dockerfile.cpu around lines 90-95, the heredoc
RUN block is immediately followed by a comment ("# upgrade first to avoid
fixable vulnerabilities end") which causes a Hadolint/Dockerfile syntax error
because comments cannot appear immediately after an EOF terminator; relocate
that section-end comment to before the corresponding RUN instruction (or merge
it with the section-start comment above the RUN) so the EOF line is directly
followed by a new Dockerfile instruction, and apply the same fix for the other
occurrences at ~lines 127-128 and 296-297.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the specific areas mentioned in the original issue to verify the fix
# Lines 95, 127-128, and 296-297 were mentioned
# Display the file with line numbers around the areas of concern
echo "=== Checking around line 95 (first occurrence) ==="
sed -n '88,100p' codeserver/ubi9-python-3.12/Dockerfile.cpu | cat -n
echo -e "\n=== Checking around line 127-128 (second occurrence) ==="
sed -n '120,135p' codeserver/ubi9-python-3.12/Dockerfile.cpu | cat -n
echo -e "\n=== Checking around line 296-297 (third occurrence) ==="
sed -n '290,305p' codeserver/ubi9-python-3.12/Dockerfile.cpu | cat -n
echo -e "\n=== Searching for all patterns where EOF is followed by a comment ==="
# Look for EOF followed immediately by # (no blank line)
grep -n "^EOF$" codeserver/ubi9-python-3.12/Dockerfile.cpu | while read line; do
linenum=$(echo "$line" | cut -d: -f1)
nextline=$((linenum + 1))
echo "EOF at line $linenum:"
sed -n "${linenum},${nextline}p" codeserver/ubi9-python-3.12/Dockerfile.cpu | cat -A
done
Length of output: 2714
@jiridanek Perfect! ✅ I've verified the fix and can confirm that all three occurrences have been corrected:
- Line 95: EOF now has a blank line before the
# upgrade first to avoid fixable vulnerabilities endcomment - Line 128: EOF now has a blank line before the
# Install the oc client endcomment - Line 298: EOF now has a blank line before the next comment
All 14 heredoc blocks in the file now properly have blank lines after their EOF markers, which resolves the Hadolint syntax error. The Dockerfile syntax is now correct! 🎉
jiridanek
added
the
tide/merge-method-squash
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ide-developer 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 |
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu (1)
76-94: CRITICAL: Single-quoted heredoc delimiter prevents variable expansion.The heredoc uses
<<'EOF'(single quotes), which prevents shell variable expansion. The condition at line 78 references$TARGETARCH, but it will remain a literal string"$TARGETARCH"rather than being expanded to its value. This causes the Rust/Cargo setup to never execute, even on s390x.Additionally,
TARGETARCHis defined as anARG(line 45), not anENV, so it is not automatically available in the shell context.Fix: Use an unquoted heredoc delimiter to enable variable expansion:
-RUN /bin/bash <<'EOF' +RUN /bin/bash <<EOF set -Eeuxo pipefail if [ "$TARGETARCH" = "s390x" ]; then # Install Rust and set up environmentThis same issue affects multiple other heredocs in this file (lines 97–104, 211–227, 236–247, 326–333, 336–343, 345–353).
rstudio/rhel9-python-3.12/Dockerfile.cuda (1)
163-165: Remove duplicate NGINX_VERSION declaration.Line 164 contains a duplicate
NGINX_VERSION=1.24that was already declared on line 163.ENV NGINX_VERSION=1.24 \ - NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \
🧹 Nitpick comments (3)
runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm (1)
82-89: Consider refactoring this multi-command RUN block for consistency.Lines 82–89 still use the older multi-line RUN style with
&&continuation. Given the PR objective to wrap multiple RUN commands in heredocs for improved readability and error handling, consider applying the same refactor here to ensure consistency across the Dockerfile.If refactoring is desired, apply this diff:
-RUN echo "Installing softwares and packages" && \ - # This may have to download and compile some dependencies, and as we don't lock requirements from `build-system.requires`, - # we often don't know the correct hashes and `--require-hashes` would therefore fail on non amd64, where building is common. - # Not using --build-constraints=./requirements.txt because error: Unnamed requirements are not allowed as constraints (found: `https://repo.radeon.com/rocm/manylinux/rocm-rel-6.4/ - uv pip install --strict --no-deps --no-cache --no-config --no-progress --verify-hashes --compile-bytecode --index-strategy=unsafe-best-match --requirements=./pylock.toml && \ - # Fix permissions to support pip in Openshift environments \ - chmod -R g+w /opt/app-root/lib/python3.12/site-packages && \ - fix-permissions /opt/app-root -P +RUN /bin/bash <<'EOF' +set -Eeuxo pipefail +echo "Installing softwares and packages" +# This may have to download and compile some dependencies, and as we don't lock requirements from `build-system.requires`, +# we often don't know the correct hashes and `--require-hashes` would therefore fail on non amd64, where building is common. +# Not using --build-constraints=./requirements.txt because error: Unnamed requirements are not allowed as constraints (found: `https://repo.radeon.com/rocm/manylinux/rocm-rel-6.4/ +uv pip install --strict --no-deps --no-cache --no-config --no-progress --verify-hashes --compile-bytecode --index-strategy=unsafe-best-match --requirements=./pylock.toml +# Fix permissions to support pip in Openshift environments +chmod -R g+w /opt/app-root/lib/python3.12/site-packages +fix-permissions /opt/app-root -P +EOFPlease verify whether this refactor should be applied or if it was intentionally deferred.
codeserver/ubi9-python-3.12/Dockerfile.cpu (1)
245-275: Consider breaking up this large heredoc block.While the heredoc refactoring is correct, this 30-line block handles many distinct concerns (NGINX directories, httpd directories, code-server extensions). Consider splitting it into smaller, focused blocks (e.g., one for NGINX setup, one for httpd setup, one for code-server directories) to improve maintainability and make failures easier to debug.
scripts/dockerfile_fragments.py (1)
140-145: Test is correct, but consider clarifying the name.The test correctly verifies that a double trailing newline in the input (
"key=value\n\n") results in a blank line before the end marker in the output. However, the nametest_lastnewline_removalis slightly misleading—the test actually demonstrates that a trailing newline is preserved (as the blank line before# end), rather than removed.Consider renaming to something like
test_trailing_newline_preservationortest_heredoc_blank_line_before_end_markerto better reflect what's being tested.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
codeserver/ubi9-python-3.12/Dockerfile.cpu(7 hunks)jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu(2 hunks)jupyter/minimal/ubi9-python-3.12/Dockerfile.cpu(3 hunks)jupyter/minimal/ubi9-python-3.12/Dockerfile.cuda(3 hunks)jupyter/minimal/ubi9-python-3.12/Dockerfile.rocm(3 hunks)jupyter/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda(3 hunks)jupyter/pytorch/ubi9-python-3.12/Dockerfile.cuda(3 hunks)jupyter/rocm/pytorch/ubi9-python-3.12/Dockerfile.rocm(3 hunks)jupyter/rocm/tensorflow/ubi9-python-3.12/Dockerfile.rocm(3 hunks)jupyter/tensorflow/ubi9-python-3.12/Dockerfile.cuda(3 hunks)jupyter/trustyai/ubi9-python-3.12/Dockerfile.cpu(3 hunks)rstudio/c9s-python-3.12/Dockerfile.cpu(1 hunks)rstudio/c9s-python-3.12/Dockerfile.cuda(1 hunks)rstudio/rhel9-python-3.12/Dockerfile.cpu(1 hunks)rstudio/rhel9-python-3.12/Dockerfile.cuda(1 hunks)runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu(2 hunks)runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu(2 hunks)runtimes/pytorch+llmcompressor/ubi9-python-3.12/Dockerfile.cuda(2 hunks)runtimes/pytorch/ubi9-python-3.12/Dockerfile.cuda(2 hunks)runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm(2 hunks)runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm(2 hunks)runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda(2 hunks)scripts/dockerfile_fragments.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- rstudio/c9s-python-3.12/Dockerfile.cpu
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (51)
- GitHub Check: build (rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: build (cuda-rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-minimal-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-codeserver-datascience-cpu-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cpu-py312-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-datascience-cpu-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-tensorflow-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-trustyai-cpu-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-minimal-rocm-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py312-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-pytorch-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-pytorch-llmcompressor-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-pytorch-rocm-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-minimal-cpu-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-tensorflow-cuda-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-datascience-cpu-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-tensorflow-rocm-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-jupyter-tensorflow-rocm-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-minimal-cpu-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-rocm-py312-ubi9-on-pull-request
- GitHub Check: Red Hat Konflux / odh-pipeline-runtime-pytorch-llmcompressor-cuda-py312-ubi9-on-pull-request
- GitHub Check: build (codeserver-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
- GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-cuda-pytorch-llmcompressor-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (codeserver-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (rocm-jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-datascience-ubi9-python-3.12, 3.12, linux/ppc64le, false) / build
- GitHub Check: build (cuda-rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-jupyter-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-minimal-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (jupyter-datascience-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-pytorch-llmcompressor-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-cuda-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/s390x, false) / build
- GitHub Check: build (cuda-jupyter-tensorflow-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-runtime-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-runtime-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-jupyter-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-trustyai-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/ppc64le, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
🔇 Additional comments (24)
runtimes/rocm-tensorflow/ubi9-python-3.12/Dockerfile.rocm (2)
28-32: LGTM! Heredoc refactor improves readability and error handling.The refactored RUN block properly uses
/bin/bash <<'EOF'with quoted delimiter to prevent variable expansion, andset -Eeuxo pipefailenforces strict error handling (exit on error, undefined variables, pipe failures) and provides command tracing. The dnf commands are preserved correctly.
47-53: LGTM! Heredoc refactor with proper error handling.The oc client installation is now properly wrapped in a heredoc with strict bash options. The tar extraction syntax (
tar -xzvf ... oc) correctly extracts only the oc binary from the archive. The use of quoted EOF prevents unintended variable expansion.jupyter/trustyai/ubi9-python-3.12/Dockerfile.cpu (3)
64-68: Improved error handling with bash heredoc wrapper.Converting the inline RUN to a /bin/bash heredoc with strict options (
set -Eeuxo pipefail) strengthens error detection for the package upgrade sequence. The single-quoted EOF correctly prevents variable expansion.Verify that container builds successfully with this change, especially on non-amd64 architectures (given the
--nobest --skip-brokenflags to handle edge cases).
83-89: Consistent heredoc pattern for OC client installation.The /bin/bash heredoc with
set -Eeuxo pipefailimproves error handling and debuggability for the multi-step download, extract, and cleanup sequence. The line continuation in curl and proper cleanup logic are correctly implemented.Confirm that
uname -mcorrectly detects architecture on all supported platforms (amd64, s390x, ppc64le) and that the tarball is accessible and unpacks without errors.
111-111: Minor formatting improvement.The blank line improves visual separation of the PATH environment configuration. No functional impact.
rstudio/rhel9-python-3.12/Dockerfile.cpu (1)
30-34: Approved: Clean heredoc refactoring with improved error handling.The refactoring correctly converts the inline package upgrade into a /bin/bash heredoc with strict error handling (
set -Eeuxo pipefail). The commands and flags remain identical—same dnf upgrade options and cleanup—so no functional change. The new form is more readable and provides better error visibility (explicit-eexit-on-error,-ufor undefined variables,-xfor command tracing). EOF is properly quoted to prevent unintended expansion.jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu (1)
56-60: ✓ Heredoc for dnf upgrade looks good.The
set -Eeuxo pipefailenables proper error handling, and the single-quoted EOF delimiter is appropriate since this block doesn't require variable expansion.runtimes/rocm-pytorch/ubi9-python-3.12/Dockerfile.rocm (2)
28-32: Heredoc refactoring looks good; verify runtime behavior.The dnf upgrade and oc client download operations are correctly wrapped in bash heredocs with strict error handling. Commands and options are preserved, and
$(uname -m)command substitution should execute correctly at runtime within the<<'EOF'block.Also applies to: 47-53
37-37: New package installation step added.Line 37 introduces a new OS package installation (perl, mesa-libGL, skopeo, libxcrypt-compat). Verify this aligns with PR objectives and is intentional (not accidentally added during refactoring).
runtimes/datascience/ubi9-python-3.12/Dockerfile.cpu (3)
32-36: Upgrade heredoc is correct.dnf upgrade wrapped properly with strict error handling. All original options preserved.
57-69: Nested heredoc syntax is valid but complex.The nested here-document (outer
EOF, innerPROFILE_EOF) uses correct bash syntax and should parse correctly at runtime. The inner here-document with single quotes (<<'PROFILE_EOF') correctly preserves$VARliterals for later expansion when the profile script is sourced.Verify that this nested heredoc pattern works correctly in your Docker/Buildah/Podman build environment. If issues arise, consider extracting the profile generation into a separate script file.
72-90: Architecture-specific heredocs are well-structured.The s390x Rust setup, python alternatives, and oc client downloads are correctly wrapped with preserved commands and proper conditional logic within heredocs.
Also applies to: 93-100, 110-116
runtimes/tensorflow/ubi9-python-3.12/Dockerfile.cuda (1)
32-36: Heredoc refactoring is correct.Both dnf upgrade and oc client download blocks are properly wrapped with strict error handling. Commands and options are preserved correctly.
Also applies to: 51-57
jupyter/rocm/pytorch/ubi9-python-3.12/Dockerfile.rocm (2)
41-45: Heredoc refactoring follows established pattern.dnf upgrade and oc client heredocs are correctly formatted with preserved commands and strict error handling.
Also applies to: 60-66
88-88: Blank line added for formatting.Minor formatting change (blank line after ENV PATH) for improved readability. Cosmetic only, no functional impact.
jupyter/minimal/ubi9-python-3.12/Dockerfile.rocm (1)
28-32: Heredoc refactoring is correct.Both dnf upgrade and oc client blocks properly wrapped with strict error handling. All commands and options preserved.
Also applies to: 47-53
jupyter/tensorflow/ubi9-python-3.12/Dockerfile.cuda (1)
43-47: Heredoc refactoring is correct.dnf upgrade and oc client blocks properly formatted with strict error handling. Commands preserved correctly.
Also applies to: 62-68
jupyter/pytorch/ubi9-python-3.12/Dockerfile.cuda (1)
43-47: Heredoc refactoring is correct and consistent.Both dnf upgrade and oc client heredocs properly formatted with strict error handling. All commands preserved.
Also applies to: 62-68
jupyter/rocm/tensorflow/ubi9-python-3.12/Dockerfile.rocm (1)
41-45: Heredoc refactoring is correct.dnf upgrade and oc client blocks properly wrapped with strict error handling. Commands and options preserved correctly.
Also applies to: 60-66
codeserver/ubi9-python-3.12/Dockerfile.cpu (4)
90-96: LGTM! Hadolint syntax issue resolved.The blank line at line 95 between the EOF terminator and the section-end comment correctly addresses the critical Hadolint syntax error flagged in previous reviews. The heredoc format with
set -Eeuxo pipefailalso improves error handling.
58-66: LGTM! Consistent heredoc refactoring.The whl-cache stage correctly wraps the build commands in a bash heredoc with strict error handling. The pattern is consistent with the rest of the file.
122-129: LGTM! EOF blank line fix applied consistently.The OC client installation block correctly follows the pattern with a blank line after EOF (line 129) before the section-end comment. This matches the fix applied to other blocks.
293-299: LGTM! Third Hadolint syntax fix confirmed.The blank line at line 299 correctly separates the EOF from the next comment, completing the fix for all three occurrences flagged in previous reviews.
scripts/dockerfile_fragments.py (1)
30-35: LGTM! HEREDOC format aligns with Dockerfile changes.The heredoc format in the generated fragment correctly matches the refactoring pattern applied throughout the Dockerfiles. The blank line after
EOFat line 35 ensures Hadolint compatibility, addressing the critical issues from previous reviews.
| RUN /bin/bash <<'EOF' | ||
| set -Eeuxo pipefail | ||
| curl -L https://mirror.openshift.com/pub/openshift-v4/$(uname -m)/clients/ocp/stable/openshift-client-linux.tar.gz \ | ||
| -o /tmp/openshift-client-linux.tar.gz | ||
| tar -xzvf /tmp/openshift-client-linux.tar.gz oc | ||
| rm -f /tmp/openshift-client-linux.tar.gz | ||
| EOF |
There was a problem hiding this comment.
CRITICAL: Single-quoted heredoc delimiter prevents command substitution.
Line 116 uses $(uname -m) within the heredoc, but the single-quoted delimiter <<'EOF' prevents command substitution. The curl command will receive a literal string "$(uname -m)" instead of the actual machine architecture, causing the download URL to be malformed and the command to fail.
Fix: Use an unquoted heredoc delimiter to enable command substitution:
-RUN /bin/bash <<'EOF'
+RUN /bin/bash <<EOF
set -Eeuxo pipefail
curl -L https://mirror.openshift.com/pub/openshift-v4/$(uname -m)/clients/ocp/stable/openshift-client-linux.tar.gz \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN /bin/bash <<'EOF' | |
| set -Eeuxo pipefail | |
| curl -L https://mirror.openshift.com/pub/openshift-v4/$(uname -m)/clients/ocp/stable/openshift-client-linux.tar.gz \ | |
| -o /tmp/openshift-client-linux.tar.gz | |
| tar -xzvf /tmp/openshift-client-linux.tar.gz oc | |
| rm -f /tmp/openshift-client-linux.tar.gz | |
| EOF | |
| RUN /bin/bash <<EOF | |
| set -Eeuxo pipefail | |
| curl -L https://mirror.openshift.com/pub/openshift-v4/$(uname -m)/clients/ocp/stable/openshift-client-linux.tar.gz \ | |
| -o /tmp/openshift-client-linux.tar.gz | |
| tar -xzvf /tmp/openshift-client-linux.tar.gz oc | |
| rm -f /tmp/openshift-client-linux.tar.gz | |
| EOF |
🤖 Prompt for AI Agents
In jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu around lines 114 to 120,
the heredoc is quoted (<<'EOF') which prevents command substitution so $(uname
-m) is not expanded; change the heredoc delimiter to an unquoted form (<<EOF) so
shell expansions like $(uname -m) are performed, leaving the rest of the block
intact and keeping existing set -Eeuxo pipefail behavior.
There was a problem hiding this comment.
@coderabbitai no, this is not how it works, bash will evaluate $(uname -m) just fine, this is heredoc in Dockerfile not heredoc in bash script
There was a problem hiding this comment.
@jiridanek, you're absolutely right, and I apologize for the confusion!
In a Dockerfile heredoc, the single-quoted delimiter <<'EOF' only prevents the Dockerfile parser from expanding build-time variables like ${ARG} or ${ENV}. The heredoc content is still passed to /bin/bash as-is, and bash will perform its own command substitution when it executes $(uname -m).
This is different from a bash script heredoc, where <<'EOF' would prevent the shell from performing expansions. Thank you for the correction!
(\_(\
(='.'=)
(")_(")
*learning noted*
✏️ Learnings added
Learnt from: jiridanek
Repo: opendatahub-io/notebooks PR: 2645
File: jupyter/datascience/ubi9-python-3.12/Dockerfile.cpu:114-120
Timestamp: 2025-11-06T10:42:31.884Z
Learning: In Dockerfile heredocs (RUN /bin/bash <<'EOF'), the single-quoted delimiter <<'EOF' only prevents Dockerfile parser from expanding build-time variables like ${ARG} or ${ENV}. The heredoc content is still passed to the specified shell (bash) which will perform its own command substitution for expressions like $(uname -m). This is different from bash script heredocs where <<'EOF' prevents shell expansion.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai konflux failure looks flaky https://konflux-ui.apps.stone-prd-rh01.pg1f.p1.openshiftapps.com/ns/open-data-hub-tenant/applications/opendatahub-release/pipelineruns/odh-pipeline-runtime-pytorch-rocm-py312-ubi9-on-pull-reque4wtvl/logs?task=build-images |
|
Tip For best results, initiate chat on the files or code changes. @jiridanek, thank you for flagging this! I agree that the failure appears to be flaky and unrelated to the changes in this PR. The error indicates an I/O operation failure during extraction of the PyTorch ROCm wheel file, which is typically a transient network or infrastructure issue. Since this PR only refactors the Dockerfile syntax by wrapping RUN commands in bash heredocs (without changing the actual package installation logic or versions), it shouldn't affect the PyTorch download/extraction process. This looks like a good candidate for a retry once the infrastructure issue resolves. |
|
@jiridanek: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. I understand the commands that are listed here. |
2 participants
Description
Reformat inline bash commands into HEREDOC blocks, so they can be later extracted to separate script files.
How Has This Been Tested?
Self checklist (all need to be checked):
make test(gmakeon macOS) before asking for reviewDockerfile.konfluxfiles should be done inodh/notebooksand automatically synced torhds/notebooks. For Konflux-specific changes, modifyDockerfile.konfluxfiles directly inrhds/notebooksas these require special attention in the downstream repository and flow to the upcoming RHOAI release.Merge criteria:
Summary by CodeRabbit