NO-JIRA: refactor(rstudio/*/Dockerfile*): wrap multiple RUN commands with bash for improved readability, consistency, and error handling #2657
Conversation
github-actions
bot
added
the
review-requested
WalkthroughReplaced grouped multi-step Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 (4)
⏰ 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). (1)
🔇 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 |
|
[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 |
jiridanek
force-pushed
the
jd_heredoc5
branch
from
411093b to
ff3b999
Compare
|
New changes are detected. LGTM label has been removed. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rstudio/c9s-python-3.12/Dockerfile.cuda (1)
139-144: Wrap NGINX installation in heredoc for consistency.This section still uses old-style backslash-chained commands instead of the heredoc pattern applied throughout the rest of this file. Refactor it to match the established pattern and enable
set -Eeuxo pipefailerror handling.Apply this diff:
-# Modules does not exist -RUN dnf -y module enable nginx:$NGINX_VERSION && \ - INSTALL_PKGS="nss_wrapper bind-utils gettext hostname nginx nginx-mod-stream nginx-mod-http-perl httpd" && \ - dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \ - rpm -V $INSTALL_PKGS && \ - nginx -v 2>&1 | grep -qe "nginx/$NGINX_VERSION\." && echo "Found VERSION $NGINX_VERSION" && \ - dnf -y clean all --enablerepo='*' +# Modules does not exist +RUN /bin/bash <<'EOF' +set -Eeuxo pipefail +dnf -y module enable nginx:$NGINX_VERSION +INSTALL_PKGS="nss_wrapper bind-utils gettext hostname nginx nginx-mod-stream nginx-mod-http-perl httpd" +dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS +rpm -V $INSTALL_PKGS +nginx -v 2>&1 | grep -qe "nginx/$NGINX_VERSION\." && echo "Found VERSION $NGINX_VERSION" +dnf -y clean all --enablerepo='*' +EOFrstudio/rhel9-python-3.12/Dockerfile.cuda (1)
181-183: Remove duplicateNGINX_VERSIONdefinition.The environment variable
NGINX_VERSIONis defined twice consecutively on lines 181-182, which is redundant and suggests incomplete editing during the refactoring.Apply this diff:
ENV NGINX_VERSION=1.24 \ - NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rstudio/c9s-python-3.12/Dockerfile.cpu(5 hunks)rstudio/c9s-python-3.12/Dockerfile.cuda(3 hunks)rstudio/rhel9-python-3.12/Dockerfile.cpu(4 hunks)rstudio/rhel9-python-3.12/Dockerfile.cuda(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Code static analysis
rstudio/rhel9-python-3.12/Dockerfile.cuda
[error] 115-115: Hadolint: unexpected '#' - parsing error in Dockerfile.cuda at line 115. Likely syntax issue or malformed instruction.
rstudio/rhel9-python-3.12/Dockerfile.cpu
[error] 126-126: Hadolint: unexpected 'm' - parsing error in Dockerfile.cpu at line 126. Likely syntax issue or malformed instruction.
⏰ 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). (6)
- 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-workbench-rstudio-minimal-cpu-py312-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py312-c9s-on-pull-request
- GitHub Check: build (cuda-rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
🔇 Additional comments (1)
rstudio/c9s-python-3.12/Dockerfile.cpu (1)
36-41: Well-structured heredoc refactoring with proper error handling.The wrapping of OS package installation into a heredoc block with
set -Eeuxo pipefailis consistent with the PR's refactoring goals. The cleanup commands (dnf clean) are now clearly grouped.
jiridanek
force-pushed
the
jd_heredoc5
branch
from
ff3b999 to
ec51d6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rstudio/c9s-python-3.12/Dockerfile.cuda (1)
139-144: Refactor NGINX installation block into heredoc for consistency with PR goals.This NGINX module enablement and package installation block uses traditional backslash-chained RUN syntax instead of the heredoc-wrapped approach applied throughout this PR. All comparable installation blocks (R, RStudio, Python packages) have been consolidated into heredocs with
set -Eeuxo pipefail. This inconsistency undermines the refactoring goal of improved error handling and readability.Apply this diff to wrap the block in a heredoc:
-# Modules does not exist -RUN dnf -y module enable nginx:$NGINX_VERSION && \ - INSTALL_PKGS="nss_wrapper bind-utils gettext hostname nginx nginx-mod-stream nginx-mod-http-perl httpd" && \ - dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \ - rpm -V $INSTALL_PKGS && \ - nginx -v 2>&1 | grep -qe "nginx/$NGINX_VERSION\." && echo "Found VERSION $NGINX_VERSION" && \ - dnf -y clean all --enablerepo='*' +# Modules does not exist +RUN /bin/bash <<'EOF' +set -Eeuxo pipefail +dnf -y module enable nginx:$NGINX_VERSION +INSTALL_PKGS="nss_wrapper bind-utils gettext hostname nginx nginx-mod-stream nginx-mod-http-perl httpd" +dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS +rpm -V $INSTALL_PKGS +nginx -v 2>&1 | grep -qe "nginx/$NGINX_VERSION\." && echo "Found VERSION $NGINX_VERSION" +dnf -y clean all --enablerepo='*' +EOFrstudio/rhel9-python-3.12/Dockerfile.cuda (1)
182-191: Remove duplicateNGINX_VERSIONenvironment variable declaration.Line 183 duplicates the
NGINX_VERSION=1.24definition from line 182, which is a copy-paste error. Environment variable declarations should not be duplicated.Apply this diff to remove the duplicate:
ENV NGINX_VERSION=1.24 \ - NGINX_VERSION=1.24 \ NGINX_SHORT_VER=124 \
♻️ Duplicate comments (1)
rstudio/c9s-python-3.12/Dockerfile.cuda (1)
85-90: Add missingset -Eeuxo pipefailfor consistency.This R library setup heredoc lacks the strict error handling flags present in all other heredocs throughout this file and the PR. The
set -Eeuxo pipefailshould be included immediately after the opening<<'EOF'to align with the refactoring goal and match the pattern used in the R installation block (lines 68-79) and other Dockerfiles.Apply this diff:
RUN /bin/bash <<'EOF' +set -Eeuxo pipefail chmod -R a+w ${LIBLOC} # create User R Library path mkdir -p ${R_LIBS_USER} chmod -R a+w ${R_LIBS_USER} EOF
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rstudio/c9s-python-3.12/Dockerfile.cpu(5 hunks)rstudio/c9s-python-3.12/Dockerfile.cuda(3 hunks)rstudio/rhel9-python-3.12/Dockerfile.cpu(4 hunks)rstudio/rhel9-python-3.12/Dockerfile.cuda(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rstudio/c9s-python-3.12/Dockerfile.cpu
🧰 Additional context used
🪛 GitHub Actions: Code static analysis
rstudio/rhel9-python-3.12/Dockerfile.cpu
[error] 254-254: Hadolint: unexpected '#' expecting a new line followed by the next instruction.
⏰ 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). (6)
- GitHub Check: build (cuda-rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: build (rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: build (cuda-rstudio-c9s-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: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py312-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cpu-py312-c9s-on-pull-request
🔇 Additional comments (2)
rstudio/rhel9-python-3.12/Dockerfile.cpu (1)
85-100: ✓ Heredoc blocks properly standardized with consistent error handling.All refactored RUN commands correctly wrap bash instructions in heredocs with
set -Eeuxo pipefailfor strict error handling. The consolidation of multi-step operations (subscriptions, R installation, RStudio, NGINX, Python packages) into atomic blocks improves failure visibility and aligns with the PR's consistency goals.Also applies to: 107-118, 124-130, 137-152, 159-163, 179-187, 212-238, 260-269
rstudio/rhel9-python-3.12/Dockerfile.cuda (1)
85-89: ✓ Comprehensive heredoc refactoring with consistent error handling throughout.All RUN commands are properly wrapped in
/bin/bash <<'EOF'blocks withset -Eeuxo pipefailfor strict error handling. CUDA toolkit, subscription management, R/RStudio installation, NGINX/httpd configuration, and Python packaging are now consolidated into atomic, traceable steps. The refactoring maintains functional equivalence while significantly improving failure visibility and readability.Also applies to: 99-114, 121-132, 138-144, 151-166, 173-177, 194-202, 227-253, 263-268, 276-285
jiridanek
force-pushed
the
jd_heredoc5
branch
from
ec51d6b to
837ebb1
Compare
jiridanek
force-pushed
the
jd_heredoc5
branch
from
837ebb1 to
22e5039
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rstudio/c9s-python-3.12/Dockerfile.cuda (1)
140-145: Convert NGINX installation to heredoc for consistency.This NGINX module installation block remains as an inline, chained RUN command, inconsistent with the PR's refactoring goal. The parallel files—
rstudio/rhel9-python-3.12/Dockerfile.cuda(lines 194-202) andrstudio/c9s-python-3.12/Dockerfile.cpu(lines 145-153)—both wrap this same logical section in heredoc blocks withset -Eeuxo pipefail.For consistency and to align with the PR's intent to improve error handling and atomicity, wrap this block in a heredoc.
Apply this diff to standardize with other files:
-# Modules does not exist -RUN dnf -y module enable nginx:$NGINX_VERSION && \ - INSTALL_PKGS="nss_wrapper bind-utils gettext hostname nginx nginx-mod-stream nginx-mod-http-perl httpd" && \ - dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \ - rpm -V $INSTALL_PKGS && \ - nginx -v 2>&1 | grep -qe "nginx/$NGINX_VERSION\." && echo "Found VERSION $NGINX_VERSION" && \ - dnf -y clean all --enablerepo='*' +# Modules does not exist +RUN /bin/bash <<'EOF' +set -Eeuxo pipefail +dnf -y module enable nginx:$NGINX_VERSION +INSTALL_PKGS="nss_wrapper bind-utils gettext hostname nginx nginx-mod-stream nginx-mod-http-perl httpd" +dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS +rpm -V $INSTALL_PKGS +nginx -v 2>&1 | grep -qe "nginx/$NGINX_VERSION\." && echo "Found VERSION $NGINX_VERSION" +dnf -y clean all --enablerepo='*' +EOF
🧹 Nitpick comments (1)
rstudio/rhel9-python-3.12/Dockerfile.cuda (1)
40-40: Consider standardizing mesa-libGL installation pattern.Line 40 in this file keeps mesa-libGL + perl installation inline, while
rstudio/c9s-python-3.12/Dockerfile.cpu(lines 36-41) wraps a similar mesa-libGL installation in a heredoc withset -Eeuxo pipefail. For consistency with the PR's refactoring goal, consider wrapping line 40 in a heredoc as well. Additionally, note that this file installs bothperlandmesa-libGL, while the CPU variant installs onlymesa-libGL—verify this difference is intentional.Also applies to: 36-41
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
rstudio/c9s-python-3.12/Dockerfile.cpu(5 hunks)rstudio/c9s-python-3.12/Dockerfile.cuda(3 hunks)rstudio/rhel9-python-3.12/Dockerfile.cpu(4 hunks)rstudio/rhel9-python-3.12/Dockerfile.cuda(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- rstudio/rhel9-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). (6)
- GitHub Check: build (cuda-rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: build (rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cpu-py312-c9s-on-pull-request
- GitHub Check: Red Hat Konflux / odh-workbench-rstudio-minimal-cuda-py312-c9s-on-pull-request
- GitHub Check: build (cuda-rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
🔇 Additional comments (3)
rstudio/rhel9-python-3.12/Dockerfile.cuda (1)
85-89: Consistent heredoc pattern with proper error handling.All converted RUN blocks consistently use
/bin/bash <<'EOF'withset -Eeuxo pipefailfor improved error visibility and atomicity. The refactoring preserves command semantics while standardizing execution contexts.Also applies to: 99-114, 121-132, 138-144, 151-166, 173-177, 194-202, 227-253, 263-268, 276-285
rstudio/c9s-python-3.12/Dockerfile.cuda (1)
68-79: Remaining heredoc conversions are consistent.All other converted RUN blocks in this file properly use heredoc syntax with
set -Eeuxo pipefail. These changes align well with the PR's refactoring goal. Once the NGINX block (lines 140-145) is converted, this file will be fully consistent with its parallel variants.Also applies to: 85-91, 98-113, 120-124, 170-196, 208-217
rstudio/c9s-python-3.12/Dockerfile.cpu (1)
36-41: Consistent heredoc pattern throughout.All RUN blocks in this file are consistently converted to heredocs with
set -Eeuxo pipefail. The conversion pattern is clean and uniform, with proper command preservation. This file demonstrates the intended refactoring approach and provides a good template for consistency across other Dockerfiles in the PR.Also applies to: 73-84, 90-96, 103-118, 125-129, 145-153, 178-204, 216-225
…with bash for improved readability, consistency, and error handling
jiridanek
force-pushed
the
jd_heredoc5
branch
from
22e5039 to
86485e5
Compare
|
@jiridanek: The following test 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. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
2 participants
Carrying on with
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