add distgen support#295
Open
ndavidova wants to merge 6 commits into
Open
Conversation
Contributor
Author
|
[test] |
Pull Request validationFailed🔴 Failed or pending statuses:
🔴 Approval - missing or changes were requested Success🟢 Review - Reviewed by a member Triggered by Workflow Run |
Testing Farm results
|
Contributor
Author
|
[test-all] |
ndavidova
marked this pull request as ready for review
May 28, 2026 15:05
ndavidova
force-pushed
the
ndavidov/distgen
branch
from
June 1, 2026 10:48
22e5d69 to
b4de737
Compare
phracek
requested changes
Jun 1, 2026
phracek
left a comment
phracek
left a comment
Member
There was a problem hiding this comment.
It looks good. I hit only some issues.
Testing Farm and quay.io have some outtages.
So please wait till they are back and online again.
ndavidova
force-pushed
the
ndavidov/distgen
branch
from
June 1, 2026 12:22
b4de737 to
364a3ff
Compare
github-actions
Bot
added
pr/changes-requested
and removed
pr/failing-ci
pr/missing-review
labels
Member
|
[test] |
ndavidova
force-pushed
the
ndavidov/distgen
branch
2 times, most recently
from
June 2, 2026 15:12
7000fba to
404bf36
Compare
ndavidova
force-pushed
the
ndavidov/distgen
branch
from
June 2, 2026 15:15
404bf36 to
ca90220
Compare
ndavidova
force-pushed
the
ndavidov/distgen
branch
from
June 2, 2026 15:37
ca90220 to
5570e91
Compare
Member
|
[test] |
phracek
requested changes
Jun 8, 2026
Member
There was a problem hiding this comment.
@ndavidova Please add also manifest file for '-micro' image.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (115)
💤 Files with no reviewable changes (67)
📝 WalkthroughWalkthroughThe change introduces specification-driven generation for regular and micro HTTPD images, consolidates runtime and S2I assets under shared source paths, updates Dockerfile variants, and adds documentation and test asset links. ChangesHTTPD image generation and runtime
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Generator
participant Manifest
participant Dockerfile
participant Image
Generator->>Manifest: Select version-specific manifest
Manifest->>Dockerfile: Apply distro and symlink rules
Dockerfile->>Image: Install packages and copy shared assets
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
ndavidova
force-pushed
the
ndavidov/distgen
branch
from
June 9, 2026 13:44
f49b5f5 to
1d56ad6
Compare
phracek
requested changes
Jun 11, 2026
phracek
left a comment
phracek
left a comment
Member
There was a problem hiding this comment.
It looks like micro is a symlink from 2.4-micro. It should be real file.
| HTTPD_SHORT_VERSION={{ spec.short }} \ | ||
| NAME=httpd | ||
|
|
||
| ENV SUMMARY="Platform for running Apache httpd ${HTTPD_VERSION} or building httpd-based application" \ |
Member
There was a problem hiding this comment.
This duplicity can be removed. Let's use once.
Please remove 'simple' and 'combined' and we can have only 'combined'.
Only:
ENV HTTPD_VERSION={{ spec.version }} \
HTTPD_SHORT_VERSION={{ spec.short }} \
NAME=httpd
is fine.
| Virtual hosting allows one Apache installation to serve many different Web sites." | ||
|
|
||
| {% if spec.var_style == "dollar" %} | ||
| LABEL summary="$SUMMARY" \ |
Member
There was a problem hiding this comment.
Label should be done once.
Let's use only $DESCRIPTION. We do not need both variants.
| io.openshift.expose-services="8080:http,8443:https" \ | ||
| io.openshift.tags="builder,httpd,httpd-{{ spec.short }}" \ | ||
| name="{{ spec.img_name }}" \ | ||
| {% if spec.version_label %} |
| {% if spec.extra_install %} | ||
| RUN {{ spec.extra_install }} | ||
| INSTALL_PKGS="{{ spec.pkgs }}" && \ | ||
| {{ spec.pkg_manager }} install -y --setopt=tsflags=nodocs ${INSTALL_PKGS} && \ |
Member
There was a problem hiding this comment.
Let's use only 'dnf'. It is available since RHEL8.
| HTTPD_DATA_ORIG_PATH=/var/www \ | ||
| HTTPD_LOG_PATH=/var/log/httpd | ||
|
|
||
| {% if spec.copy_version %} |
Member
There was a problem hiding this comment.
This condition is not needed. Let's unifi it.
| COPY ${HTTPD_VERSION}/root / | ||
| {% endif %} | ||
|
|
||
| {% if spec.fedora_specific %} |
Member
There was a problem hiding this comment.
This is not needed at all. Please remove it.
| # Reset permissions of filesystem to default values | ||
| {% else %} | ||
| # Reset permissions of filesystem to default values | ||
| {% endif %} |
Member
|
[test] |
frenzymadness
left a comment
frenzymadness
left a comment
Member
There was a problem hiding this comment.
Just two comments from me.
|
|
||
| generate: | ||
| for version in ${VERSIONS} ; do \ | ||
| if [[ "$$version" == *-micro ]]; then \ |
Member
There was a problem hiding this comment.
[[ ]] are bash-specific and if we run this on a system where /bin/sh is not bash, it will fail. We can either require bash here in the Makefile or switch this to a POSIX-compatible variant.
Comment on lines
+30
to
+31
| - src: . | ||
| dest: "2.4" |
Member
There was a problem hiding this comment.
This creates a following symlink inside the 2.4 directory: 2.4 -> .
Contributor
Author
|
[test] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
specs/multispec.yml (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional refactor: Remove unused
pkg_managerconfiguration.The PR objective notes that
spec.pkg_managerwas replaced directly withdnfin the templates. If the generator template no longer interpolatespkg_manager, this key (and its occurrences across other distributions in this file) is dead configuration and can be safely removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/multispec.yml` at line 15, Remove the unused pkg_manager configuration key from the multispec definitions, including its occurrences for other distributions, after confirming the generator templates no longer interpolate it. Leave the template behavior and all unrelated distribution settings unchanged.test/utils.sh (1)
36-46: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueQuote the
$cmdvariable passed toeval.Using
eval $cmdwithout quotes causes word splitting beforeevalprocesses the arguments, which can break test commands that contain complex quoting or spaces. Useeval "$cmd"to preserve the exact command string.♻️ Proposed fix
function run_command { local cmd="$1" local expected="${2:-0}" local msg="${3:-Running command '$cmd'}" print_result working "$msg" - eval $cmd + eval "$cmd" local res="$?" - status=`get_status "$res" "$expected"` + status=$(get_status "$res" "$expected") print_result "$status" "$msg" return "$res" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/utils.sh` around lines 36 - 46, Update the eval invocation in run_command to pass cmd as a quoted argument, preserving embedded spaces and quoting while leaving the surrounding command execution and status handling unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/multispec.yml`:
- Line 125: Correct the registry hostname from quya.io to quay.io in
specs/multispec.yml:125-125, then regenerate the Dockerfile or directly update
2.4/Dockerfile.fedora:31-31 so its usage label uses quay.io as well.
- Line 57: Replace the empty version_label in specs/multispec.yml with a valid
non-empty value consistent with the other variants, or omit the field when no
version label is intended. Regenerate or update 2.4/Dockerfile.rhel10 so its
LABEL metadata no longer contains version=""; ensure both sites reflect the
chosen valid-or-omitted behavior.
In `@src/root/usr/bin/run-httpd`:
- Line 18: Update the argument expansion in the httpd exec command to use quoted
"$@" so each argument passed to the script retains its original boundaries,
including spaces.
In `@src/root/usr/libexec/httpd-prepare`:
- Around line 8-10: Replace the recursive a+rwx permissions in the httpd
preparation flow with group-scoped access limited to the users that assemble
configuration, and remove world-write access from the main, drop-in, and modules
configuration paths. Ensure TLS private-key files and storage receive
restrictive permissions while preserving necessary read/write access for the
intended service group.
In `@src/root/usr/share/container-scripts/httpd/common.sh`:
- Around line 193-198: Update process_ssl_certs to quote the dir-based path
expansions in both directory checks and the mv command, preventing word
splitting and ensuring paths containing spaces are handled safely.
- Around line 173-190: Quote all dir-based path expansions in
process_config_files, including the httpd-cfg and deprecated cfg directory
checks, ls glob paths, cp sources, and rm targets, so paths containing spaces or
shell metacharacters are handled safely without changing the existing
configuration-copy behavior.
In `@src/s2i/bin/run`:
- Around line 5-7: Update the exec invocation in the run script to pass
positional arguments as a quoted expansion, preserving spaces, wildcard
characters, and original argument boundaries.
In `@src/s2i/bin/usage`:
- Around line 1-5: Make the shell used by the usage script compatible with the
conditional in the DISTRO branch: either change the shebang to /bin/bash to
support the existing [[ $DISTRO =~ rhel* ]] expression, or rewrite that
condition with POSIX [ ] syntax while preserving correct RHEL detection.
In `@test/pre-init-test-app/index.html`:
- Line 1: Prepend the HTML5 doctype declaration to the static fixture content in
index.html, before the existing sample text, so browsers render it in standards
mode.
---
Nitpick comments:
In `@specs/multispec.yml`:
- Line 15: Remove the unused pkg_manager configuration key from the multispec
definitions, including its occurrences for other distributions, after confirming
the generator templates no longer interpolate it. Leave the template behavior
and all unrelated distribution settings unchanged.
In `@test/utils.sh`:
- Around line 36-46: Update the eval invocation in run_command to pass cmd as a
quoted argument, preserving embedded spaces and quoting while leaving the
surrounding command execution and status handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c741363-df1c-4ad8-a5d2-0b9735b23e5b
📒 Files selected for processing (115)
2.4-micro/.build-args-c9s2.4-micro/.exclude-c8s2.4-micro/2.4-micro2.4-micro/Dockerfile.c10s2.4-micro/Dockerfile.c9s2.4-micro/Dockerfile.fedora2.4-micro/core-scripts2.4-micro/root2.4-micro/root/usr/share/container-scripts/httpd/common.sh2.4-micro/s2i2.4-micro/test2.4-micro/test/__init__.py2.4-micro/test/check_imagestreams.py2.4-micro/test/conftest.py2.4-micro/test/examples2.4-micro/test/imagestreams2.4-micro/test/run2.4-micro/test/run-openshift-pytest2.4-micro/test/run-openshift-remote-cluster2.4-micro/test/run-pytest2.4-micro/test/sample-test-app2.4-micro/test/self-signed-ssl2.4-micro/test/settings.py2.4-micro/test/test-lib-httpd.sh2.4-micro/test/test-lib-openshift.sh2.4-micro/test/test-lib-remote-openshift.sh2.4-micro/test/test-lib.sh2.4-micro/test/test_container_httpd.py2.4-micro/test/test_container_httpd_s2i.py2.4-micro/test/test_ocp_ex_template.py2.4-micro/test/test_ocp_imagestream_s2i.py2.4-micro/test/test_ocp_imagestreams.py2.4-micro/test/test_ocp_integration.py2.4-micro/test/test_ocp_shared_helm_imagestreams.py2.4-micro/test/test_ocp_shared_helm_template.py2.4/2.42.4/Dockerfile.c10s2.4/Dockerfile.c9s2.4/Dockerfile.fedora2.4/Dockerfile.rhel102.4/Dockerfile.rhel82.4/Dockerfile.rhel92.4/root2.4/root/usr/bin/run-httpd2.4/root/usr/libexec/httpd-prepare2.4/root/usr/share/container-scripts/httpd/passwd.template2.4/root/usr/share/container-scripts/httpd/post-assemble/20-copy-config.sh2.4/root/usr/share/container-scripts/httpd/post-assemble/40-ssl-certs.sh2.4/root/usr/share/container-scripts/httpd/pre-init/10-set-mpm.sh2.4/root/usr/share/container-scripts/httpd/pre-init/20-copy-config.sh2.4/root/usr/share/container-scripts/httpd/pre-init/40-ssl-certs.sh2.4/s2i2.4/s2i/bin/assemble2.4/s2i/bin/run2.4/s2i/bin/usage2.4/test2.4/test/__init__.py2.4/test/check_imagestreams.py2.4/test/conftest.py2.4/test/examples2.4/test/imagestreams2.4/test/pre-init-test-app/httpd-pre-init/modify_index.sh2.4/test/pre-init-test-app/index.html2.4/test/run2.4/test/run-openshift-pytest2.4/test/run-openshift-remote-cluster2.4/test/run-pytest2.4/test/sample-test-app2.4/test/self-signed-ssl2.4/test/settings.py2.4/test/test-lib-httpd.sh2.4/test/test-lib-openshift.sh2.4/test/test-lib-remote-openshift.sh2.4/test/test-lib.sh2.4/test/test-openshift.yaml2.4/test/test_container_httpd.py2.4/test/test_container_httpd_s2i.py2.4/test/test_ocp_ex_template.py2.4/test/test_ocp_imagestream_s2i.py2.4/test/test_ocp_imagestreams.py2.4/test/test_ocp_integration.py2.4/test/test_ocp_shared_helm_imagestreams.py2.4/test/test_ocp_shared_helm_template.py2.4/test/utils.shMakefilemanifest-micro.ymlmanifest.ymlspecs/multispec.ymlsrc/Dockerfilesrc/Dockerfile.microsrc/README.mdsrc/root/README.mdsrc/root/usr/bin/run-httpdsrc/root/usr/libexec/httpd-preparesrc/root/usr/share/container-scripts/httpd/README.mdsrc/root/usr/share/container-scripts/httpd/common.shsrc/root/usr/share/container-scripts/httpd/passwd.templatesrc/root/usr/share/container-scripts/httpd/post-assemble/20-copy-config.shsrc/root/usr/share/container-scripts/httpd/post-assemble/40-ssl-certs.shsrc/root/usr/share/container-scripts/httpd/pre-init/10-set-mpm.shsrc/root/usr/share/container-scripts/httpd/pre-init/20-copy-config.shsrc/root/usr/share/container-scripts/httpd/pre-init/40-ssl-certs.shsrc/s2i/bin/assemblesrc/s2i/bin/runsrc/s2i/bin/usagetest/examplestest/pre-init-test-app/httpd-pre-init/modify_index.shtest/pre-init-test-app/index.htmltest/run-openshift-remote-clustertest/self-signed-ssltest/test-lib-httpd.shtest/test-lib-openshift.shtest/test-lib-remote-openshift.shtest/test-openshift.yamltest/utils.sh
💤 Files with no reviewable changes (67)
- 2.4-micro/test/imagestreams
- 2.4-micro/test/sample-test-app
- 2.4/test/test_ocp_imagestreams.py
- 2.4/2.4
- 2.4-micro/test/test_ocp_shared_helm_template.py
- 2.4/test/run-openshift-pytest
- 2.4-micro/test/test_ocp_integration.py
- 2.4/test/test_ocp_shared_helm_template.py
- 2.4/test/pre-init-test-app/httpd-pre-init/modify_index.sh
- 2.4/test/pre-init-test-app/index.html
- 2.4/test/test_container_httpd.py
- 2.4-micro/test/settings.py
- 2.4/test/test-lib-openshift.sh
- 2.4/test/run
- 2.4-micro/2.4-micro
- 2.4-micro/test/test-lib-openshift.sh
- 2.4/root/usr/share/container-scripts/httpd/pre-init/20-copy-config.sh
- 2.4/test/test-lib-httpd.sh
- 2.4-micro/test/test_ocp_ex_template.py
- 2.4/root/usr/share/container-scripts/httpd/post-assemble/20-copy-config.sh
- 2.4/test/run-openshift-remote-cluster
- 2.4-micro/test/run
- 2.4/test/settings.py
- 2.4-micro/test/run-openshift-pytest
- 2.4/root/usr/share/container-scripts/httpd/post-assemble/40-ssl-certs.sh
- 2.4/s2i/bin/run
- 2.4/test/test_container_httpd_s2i.py
- 2.4/root/usr/share/container-scripts/httpd/pre-init/40-ssl-certs.sh
- 2.4/root/usr/bin/run-httpd
- 2.4/test/test_ocp_integration.py
- 2.4/test/examples
- 2.4/test/sample-test-app
- 2.4-micro/test/check_imagestreams.py
- 2.4-micro/test/test-lib.sh
- 2.4-micro/test/test-lib-remote-openshift.sh
- 2.4/s2i/bin/usage
- 2.4-micro/test/test_container_httpd.py
- 2.4-micro/.build-args-c9s
- 2.4-micro/test/test_ocp_imagestreams.py
- 2.4/test/self-signed-ssl
- 2.4/test/test_ocp_ex_template.py
- 2.4/test/test_ocp_imagestream_s2i.py
- 2.4-micro/test/self-signed-ssl
- 2.4-micro/test/test_container_httpd_s2i.py
- 2.4-micro/test/run-openshift-remote-cluster
- 2.4-micro/test/examples
- 2.4/s2i/bin/assemble
- 2.4/test/run-pytest
- 2.4/root/usr/share/container-scripts/httpd/passwd.template
- 2.4/test/conftest.py
- 2.4-micro/test/conftest.py
- 2.4-micro/test/test_ocp_shared_helm_imagestreams.py
- 2.4-micro/test/test_ocp_imagestream_s2i.py
- 2.4/test/imagestreams
- 2.4/test/check_imagestreams.py
- 2.4-micro/test/init.py
- 2.4/root/usr/libexec/httpd-prepare
- 2.4/test/init.py
- 2.4/test/test-openshift.yaml
- 2.4/test/test_ocp_shared_helm_imagestreams.py
- 2.4/test/test-lib-remote-openshift.sh
- 2.4-micro/test/run-pytest
- 2.4-micro/test/test-lib-httpd.sh
- 2.4/root/usr/share/container-scripts/httpd/pre-init/10-set-mpm.sh
- 2.4/test/test-lib.sh
- 2.4-micro/root/usr/share/container-scripts/httpd/common.sh
- 2.4/test/utils.sh
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
🧹 Nitpick comments (2)
specs/multispec.yml (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional refactor: Remove unused
pkg_managerconfiguration.The PR objective notes that
spec.pkg_managerwas replaced directly withdnfin the templates. If the generator template no longer interpolatespkg_manager, this key (and its occurrences across other distributions in this file) is dead configuration and can be safely removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/multispec.yml` at line 15, Remove the unused pkg_manager configuration key from the multispec definitions, including its occurrences for other distributions, after confirming the generator templates no longer interpolate it. Leave the template behavior and all unrelated distribution settings unchanged.test/utils.sh (1)
36-46: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueQuote the
$cmdvariable passed toeval.Using
eval $cmdwithout quotes causes word splitting beforeevalprocesses the arguments, which can break test commands that contain complex quoting or spaces. Useeval "$cmd"to preserve the exact command string.♻️ Proposed fix
function run_command { local cmd="$1" local expected="${2:-0}" local msg="${3:-Running command '$cmd'}" print_result working "$msg" - eval $cmd + eval "$cmd" local res="$?" - status=`get_status "$res" "$expected"` + status=$(get_status "$res" "$expected") print_result "$status" "$msg" return "$res" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/utils.sh` around lines 36 - 46, Update the eval invocation in run_command to pass cmd as a quoted argument, preserving embedded spaces and quoting while leaving the surrounding command execution and status handling unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/multispec.yml`:
- Line 125: Correct the registry hostname from quya.io to quay.io in
specs/multispec.yml:125-125, then regenerate the Dockerfile or directly update
2.4/Dockerfile.fedora:31-31 so its usage label uses quay.io as well.
- Line 57: Replace the empty version_label in specs/multispec.yml with a valid
non-empty value consistent with the other variants, or omit the field when no
version label is intended. Regenerate or update 2.4/Dockerfile.rhel10 so its
LABEL metadata no longer contains version=""; ensure both sites reflect the
chosen valid-or-omitted behavior.
In `@src/root/usr/bin/run-httpd`:
- Line 18: Update the argument expansion in the httpd exec command to use quoted
"$@" so each argument passed to the script retains its original boundaries,
including spaces.
In `@src/root/usr/libexec/httpd-prepare`:
- Around line 8-10: Replace the recursive a+rwx permissions in the httpd
preparation flow with group-scoped access limited to the users that assemble
configuration, and remove world-write access from the main, drop-in, and modules
configuration paths. Ensure TLS private-key files and storage receive
restrictive permissions while preserving necessary read/write access for the
intended service group.
In `@src/root/usr/share/container-scripts/httpd/common.sh`:
- Around line 193-198: Update process_ssl_certs to quote the dir-based path
expansions in both directory checks and the mv command, preventing word
splitting and ensuring paths containing spaces are handled safely.
- Around line 173-190: Quote all dir-based path expansions in
process_config_files, including the httpd-cfg and deprecated cfg directory
checks, ls glob paths, cp sources, and rm targets, so paths containing spaces or
shell metacharacters are handled safely without changing the existing
configuration-copy behavior.
In `@src/s2i/bin/run`:
- Around line 5-7: Update the exec invocation in the run script to pass
positional arguments as a quoted expansion, preserving spaces, wildcard
characters, and original argument boundaries.
In `@src/s2i/bin/usage`:
- Around line 1-5: Make the shell used by the usage script compatible with the
conditional in the DISTRO branch: either change the shebang to /bin/bash to
support the existing [[ $DISTRO =~ rhel* ]] expression, or rewrite that
condition with POSIX [ ] syntax while preserving correct RHEL detection.
In `@test/pre-init-test-app/index.html`:
- Line 1: Prepend the HTML5 doctype declaration to the static fixture content in
index.html, before the existing sample text, so browsers render it in standards
mode.
---
Nitpick comments:
In `@specs/multispec.yml`:
- Line 15: Remove the unused pkg_manager configuration key from the multispec
definitions, including its occurrences for other distributions, after confirming
the generator templates no longer interpolate it. Leave the template behavior
and all unrelated distribution settings unchanged.
In `@test/utils.sh`:
- Around line 36-46: Update the eval invocation in run_command to pass cmd as a
quoted argument, preserving embedded spaces and quoting while leaving the
surrounding command execution and status handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c741363-df1c-4ad8-a5d2-0b9735b23e5b
📒 Files selected for processing (115)
2.4-micro/.build-args-c9s2.4-micro/.exclude-c8s2.4-micro/2.4-micro2.4-micro/Dockerfile.c10s2.4-micro/Dockerfile.c9s2.4-micro/Dockerfile.fedora2.4-micro/core-scripts2.4-micro/root2.4-micro/root/usr/share/container-scripts/httpd/common.sh2.4-micro/s2i2.4-micro/test2.4-micro/test/__init__.py2.4-micro/test/check_imagestreams.py2.4-micro/test/conftest.py2.4-micro/test/examples2.4-micro/test/imagestreams2.4-micro/test/run2.4-micro/test/run-openshift-pytest2.4-micro/test/run-openshift-remote-cluster2.4-micro/test/run-pytest2.4-micro/test/sample-test-app2.4-micro/test/self-signed-ssl2.4-micro/test/settings.py2.4-micro/test/test-lib-httpd.sh2.4-micro/test/test-lib-openshift.sh2.4-micro/test/test-lib-remote-openshift.sh2.4-micro/test/test-lib.sh2.4-micro/test/test_container_httpd.py2.4-micro/test/test_container_httpd_s2i.py2.4-micro/test/test_ocp_ex_template.py2.4-micro/test/test_ocp_imagestream_s2i.py2.4-micro/test/test_ocp_imagestreams.py2.4-micro/test/test_ocp_integration.py2.4-micro/test/test_ocp_shared_helm_imagestreams.py2.4-micro/test/test_ocp_shared_helm_template.py2.4/2.42.4/Dockerfile.c10s2.4/Dockerfile.c9s2.4/Dockerfile.fedora2.4/Dockerfile.rhel102.4/Dockerfile.rhel82.4/Dockerfile.rhel92.4/root2.4/root/usr/bin/run-httpd2.4/root/usr/libexec/httpd-prepare2.4/root/usr/share/container-scripts/httpd/passwd.template2.4/root/usr/share/container-scripts/httpd/post-assemble/20-copy-config.sh2.4/root/usr/share/container-scripts/httpd/post-assemble/40-ssl-certs.sh2.4/root/usr/share/container-scripts/httpd/pre-init/10-set-mpm.sh2.4/root/usr/share/container-scripts/httpd/pre-init/20-copy-config.sh2.4/root/usr/share/container-scripts/httpd/pre-init/40-ssl-certs.sh2.4/s2i2.4/s2i/bin/assemble2.4/s2i/bin/run2.4/s2i/bin/usage2.4/test2.4/test/__init__.py2.4/test/check_imagestreams.py2.4/test/conftest.py2.4/test/examples2.4/test/imagestreams2.4/test/pre-init-test-app/httpd-pre-init/modify_index.sh2.4/test/pre-init-test-app/index.html2.4/test/run2.4/test/run-openshift-pytest2.4/test/run-openshift-remote-cluster2.4/test/run-pytest2.4/test/sample-test-app2.4/test/self-signed-ssl2.4/test/settings.py2.4/test/test-lib-httpd.sh2.4/test/test-lib-openshift.sh2.4/test/test-lib-remote-openshift.sh2.4/test/test-lib.sh2.4/test/test-openshift.yaml2.4/test/test_container_httpd.py2.4/test/test_container_httpd_s2i.py2.4/test/test_ocp_ex_template.py2.4/test/test_ocp_imagestream_s2i.py2.4/test/test_ocp_imagestreams.py2.4/test/test_ocp_integration.py2.4/test/test_ocp_shared_helm_imagestreams.py2.4/test/test_ocp_shared_helm_template.py2.4/test/utils.shMakefilemanifest-micro.ymlmanifest.ymlspecs/multispec.ymlsrc/Dockerfilesrc/Dockerfile.microsrc/README.mdsrc/root/README.mdsrc/root/usr/bin/run-httpdsrc/root/usr/libexec/httpd-preparesrc/root/usr/share/container-scripts/httpd/README.mdsrc/root/usr/share/container-scripts/httpd/common.shsrc/root/usr/share/container-scripts/httpd/passwd.templatesrc/root/usr/share/container-scripts/httpd/post-assemble/20-copy-config.shsrc/root/usr/share/container-scripts/httpd/post-assemble/40-ssl-certs.shsrc/root/usr/share/container-scripts/httpd/pre-init/10-set-mpm.shsrc/root/usr/share/container-scripts/httpd/pre-init/20-copy-config.shsrc/root/usr/share/container-scripts/httpd/pre-init/40-ssl-certs.shsrc/s2i/bin/assemblesrc/s2i/bin/runsrc/s2i/bin/usagetest/examplestest/pre-init-test-app/httpd-pre-init/modify_index.shtest/pre-init-test-app/index.htmltest/run-openshift-remote-clustertest/self-signed-ssltest/test-lib-httpd.shtest/test-lib-openshift.shtest/test-lib-remote-openshift.shtest/test-openshift.yamltest/utils.sh
💤 Files with no reviewable changes (67)
- 2.4-micro/test/imagestreams
- 2.4-micro/test/sample-test-app
- 2.4/test/test_ocp_imagestreams.py
- 2.4/2.4
- 2.4-micro/test/test_ocp_shared_helm_template.py
- 2.4/test/run-openshift-pytest
- 2.4-micro/test/test_ocp_integration.py
- 2.4/test/test_ocp_shared_helm_template.py
- 2.4/test/pre-init-test-app/httpd-pre-init/modify_index.sh
- 2.4/test/pre-init-test-app/index.html
- 2.4/test/test_container_httpd.py
- 2.4-micro/test/settings.py
- 2.4/test/test-lib-openshift.sh
- 2.4/test/run
- 2.4-micro/2.4-micro
- 2.4-micro/test/test-lib-openshift.sh
- 2.4/root/usr/share/container-scripts/httpd/pre-init/20-copy-config.sh
- 2.4/test/test-lib-httpd.sh
- 2.4-micro/test/test_ocp_ex_template.py
- 2.4/root/usr/share/container-scripts/httpd/post-assemble/20-copy-config.sh
- 2.4/test/run-openshift-remote-cluster
- 2.4-micro/test/run
- 2.4/test/settings.py
- 2.4-micro/test/run-openshift-pytest
- 2.4/root/usr/share/container-scripts/httpd/post-assemble/40-ssl-certs.sh
- 2.4/s2i/bin/run
- 2.4/test/test_container_httpd_s2i.py
- 2.4/root/usr/share/container-scripts/httpd/pre-init/40-ssl-certs.sh
- 2.4/root/usr/bin/run-httpd
- 2.4/test/test_ocp_integration.py
- 2.4/test/examples
- 2.4/test/sample-test-app
- 2.4-micro/test/check_imagestreams.py
- 2.4-micro/test/test-lib.sh
- 2.4-micro/test/test-lib-remote-openshift.sh
- 2.4/s2i/bin/usage
- 2.4-micro/test/test_container_httpd.py
- 2.4-micro/.build-args-c9s
- 2.4-micro/test/test_ocp_imagestreams.py
- 2.4/test/self-signed-ssl
- 2.4/test/test_ocp_ex_template.py
- 2.4/test/test_ocp_imagestream_s2i.py
- 2.4-micro/test/self-signed-ssl
- 2.4-micro/test/test_container_httpd_s2i.py
- 2.4-micro/test/run-openshift-remote-cluster
- 2.4-micro/test/examples
- 2.4/s2i/bin/assemble
- 2.4/test/run-pytest
- 2.4/root/usr/share/container-scripts/httpd/passwd.template
- 2.4/test/conftest.py
- 2.4-micro/test/conftest.py
- 2.4-micro/test/test_ocp_shared_helm_imagestreams.py
- 2.4-micro/test/test_ocp_imagestream_s2i.py
- 2.4/test/imagestreams
- 2.4/test/check_imagestreams.py
- 2.4-micro/test/init.py
- 2.4/root/usr/libexec/httpd-prepare
- 2.4/test/init.py
- 2.4/test/test-openshift.yaml
- 2.4/test/test_ocp_shared_helm_imagestreams.py
- 2.4/test/test-lib-remote-openshift.sh
- 2.4-micro/test/run-pytest
- 2.4-micro/test/test-lib-httpd.sh
- 2.4/root/usr/share/container-scripts/httpd/pre-init/10-set-mpm.sh
- 2.4/test/test-lib.sh
- 2.4-micro/root/usr/share/container-scripts/httpd/common.sh
- 2.4/test/utils.sh
🛑 Comments failed to post (9)
specs/multispec.yml (2)
57-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Provide a non-empty version label or omit it in the template.
Defining
version_labelas an empty string generates aversion=""label in the Dockerfile. Empty label values for standard metadata (likeversion) cause validation failures in downstream build pipelines such as Konflux and Red Hat Container Certification.
specs/multispec.yml#L57-L57: Provide a valid value (e.g.,"1", matching other variants) or implement a way to completely omit the label if intended.2.4/Dockerfile.rhel10#L29-L29: Remove or populate the emptyversion=""label (this file will likely be regenerated).📍 Affects 2 files
specs/multispec.yml#L57-L57(this comment)2.4/Dockerfile.rhel10#L29-L29🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/multispec.yml` at line 57, Replace the empty version_label in specs/multispec.yml with a valid non-empty value consistent with the other variants, or omit the field when no version label is intended. Regenerate or update 2.4/Dockerfile.rhel10 so its LABEL metadata no longer contains version=""; ensure both sites reflect the chosen valid-or-omitted behavior.
125-125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the typo in the registry URL.
There is a typo in the registry URL (
quya.ioinstead ofquay.io), which propagates to the generated Dockerfile'susagelabel.
specs/multispec.yml#L125-L125: Changequya.iotoquay.io.2.4/Dockerfile.fedora#L31-L31: Changequya.iotoquay.io(or simply regenerate the Dockerfile after fixing the spec).📍 Affects 2 files
specs/multispec.yml#L125-L125(this comment)2.4/Dockerfile.fedora#L31-L31🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/multispec.yml` at line 125, Correct the registry hostname from quya.io to quay.io in specs/multispec.yml:125-125, then regenerate the Dockerfile or directly update 2.4/Dockerfile.fedora:31-31 so its usage label uses quay.io as well.src/root/usr/bin/run-httpd (1)
18-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote
$@to preserve argument boundaries.Using unquoted
$@causes arguments containing spaces to be split into multiple separate arguments. Use"$@"to ensure arguments are passed exactly as provided.🐛 Proposed fix
-exec httpd -D FOREGROUND $@ +exec httpd -D FOREGROUND "$@"📝 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.exec httpd -D FOREGROUND "$@"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/root/usr/bin/run-httpd` at line 18, Update the argument expansion in the httpd exec command to use quoted "$@" so each argument passed to the script retains its original boundaries, including spaces.src/root/usr/libexec/httpd-prepare (1)
8-10: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not make HTTPD configuration and TLS storage world-writable.
a+rwxgrants every container user write access tohttpd.conf/ssl.confand the TLS directory, allowing an unprivileged application to alter server behavior or replace certificate/key material. Use group-scoped permissions where assembly requires writes, then apply restrictive permissions to private keys.Also applies to: 22-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/root/usr/libexec/httpd-prepare` around lines 8 - 10, Replace the recursive a+rwx permissions in the httpd preparation flow with group-scoped access limited to the users that assemble configuration, and remove world-write access from the main, drop-in, and modules configuration paths. Ensure TLS private-key files and storage receive restrictive permissions while preserving necessary read/write access for the intended service group.src/root/usr/share/container-scripts/httpd/common.sh (2)
173-190: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Quote variables in path expansions to prevent word splitting.
Unquoted variables like
${dir}incpandrmcommands can cause unintended word splitting or globbing if the directory path contains spaces or special characters, potentially leading to incorrect file operations or unintended deletions.🛡️ Proposed fixes for quoting
process_config_files() { local dir=${1:-.} - if [ -d ${dir}/httpd-cfg ]; then + if [ -d "${dir}/httpd-cfg" ]; then echo "---> Copying httpd configuration files..." - if [ "$(ls -A ${dir}/httpd-cfg/*.conf)" ]; then - cp -v ${dir}/httpd-cfg/*.conf "${HTTPD_CONFIGURATION_PATH}" - rm -rf ${dir}/httpd-cfg + if [ "$(ls -A "${dir}"/httpd-cfg/*.conf 2>/dev/null)" ]; then + cp -v "${dir}"/httpd-cfg/*.conf "${HTTPD_CONFIGURATION_PATH}" + rm -rf "${dir}/httpd-cfg" fi else - if [ -d ${dir}/cfg ]; then + if [ -d "${dir}/cfg" ]; then echo "---> Copying httpd configuration files from deprecated './cfg' directory, use './httpd-cfg' instead..." - if [ "$(ls -A ${dir}/cfg/*.conf)" ]; then - cp -v ${dir}/cfg/*.conf "${HTTPD_CONFIGURATION_PATH}" - rm -rf ${dir}/cfg + if [ "$(ls -A "${dir}"/cfg/*.conf 2>/dev/null)" ]; then + cp -v "${dir}"/cfg/*.conf "${HTTPD_CONFIGURATION_PATH}" + rm -rf "${dir}/cfg" fi fi fi📝 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.process_config_files() { local dir=${1:-.} if [ -d "${dir}/httpd-cfg" ]; then echo "---> Copying httpd configuration files..." if [ "$(ls -A "${dir}"/httpd-cfg/*.conf 2>/dev/null)" ]; then cp -v "${dir}"/httpd-cfg/*.conf "${HTTPD_CONFIGURATION_PATH}" rm -rf "${dir}/httpd-cfg" fi else if [ -d "${dir}/cfg" ]; then echo "---> Copying httpd configuration files from deprecated './cfg' directory, use './httpd-cfg' instead..." if [ "$(ls -A "${dir}"/cfg/*.conf 2>/dev/null)" ]; then cp -v "${dir}"/cfg/*.conf "${HTTPD_CONFIGURATION_PATH}" rm -rf "${dir}/cfg" fi fi fi }🧰 Tools
🪛 ast-grep (0.44.1)
[error] 178-178: Recursive
rmis given an unquoted variable expansion as a target. If the variable is empty, contains whitespace, or holds glob characters, word splitting and pathname expansion can makermdelete unintended paths (e.g.rm -rf $dirwithdir=""ordir="/ tmp"). Always double-quote the expansion and guard against empty values:rm -rf -- "${dir:?dir is unset}".
Context: ${dir}
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(rm-rf-unquoted-variable-bash)
[error] 185-185: Recursive
rmis given an unquoted variable expansion as a target. If the variable is empty, contains whitespace, or holds glob characters, word splitting and pathname expansion can makermdelete unintended paths (e.g.rm -rf $dirwithdir=""ordir="/ tmp"). Always double-quote the expansion and guard against empty values:rm -rf -- "${dir:?dir is unset}".
Context: ${dir}
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(rm-rf-unquoted-variable-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/root/usr/share/container-scripts/httpd/common.sh` around lines 173 - 190, Quote all dir-based path expansions in process_config_files, including the httpd-cfg and deprecated cfg directory checks, ls glob paths, cp sources, and rm targets, so paths containing spaces or shell metacharacters are handled safely without changing the existing configuration-copy behavior.Source: Linters/SAST tools
193-198: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Quote variables in path expansions to prevent word splitting.
Similar to the config copying function, the
${dir}variable should be quoted to prevent word splitting when moving the SSL directory, which mitigates the risk of command injection or clobbering unintended files.🛡️ Proposed fixes for quoting
process_ssl_certs() { local dir=${1:-.} - if [ -d ${dir}/httpd-ssl/private ] && [ -d ${dir}/httpd-ssl/certs ]; then + if [ -d "${dir}/httpd-ssl/private" ] && [ -d "${dir}/httpd-ssl/certs" ]; then echo "---> Moving the httpd-ssl directory included in the source to a directory that isn't exposed by httpd..." - mv ${dir}/httpd-ssl ${HTTPD_APP_ROOT} + mv "${dir}/httpd-ssl" "${HTTPD_APP_ROOT}" fi📝 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.process_ssl_certs() { local dir=${1:-.} if [ -d "${dir}/httpd-ssl/private" ] && [ -d "${dir}/httpd-ssl/certs" ]; then echo "---> Moving the httpd-ssl directory included in the source to a directory that isn't exposed by httpd..." mv "${dir}/httpd-ssl" "${HTTPD_APP_ROOT}" fi🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 196-196:
mv/cpis invoked with an unquoted variable argument (e.g.mv $src $dst). Unquoted expansions undergo word splitting and pathname (glob) expansion, so a value containing spaces,*/?/[, or leading-can split into extra arguments, expand to unintended files, or be parsed as an option — leading to files being moved/overwritten/clobbered at attacker-controlled paths. Always double-quote the expansion and terminate options with--:mv -- "$src" "$dst".
Context: mv ${dir}/httpd-ssl ${HTTPD_APP_ROOT}
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(mv-cp-unquoted-variable-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/root/usr/share/container-scripts/httpd/common.sh` around lines 193 - 198, Update process_ssl_certs to quote the dir-based path expansions in both directory checks and the mv command, preventing word splitting and ensuring paths containing spaces are handled safely.Source: Linters/SAST tools
src/s2i/bin/run (1)
5-7: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the original argument boundaries.
exec run-httpd $@splits arguments and expands globs, so valid runtime arguments containing spaces or wildcard characters are not forwarded intact. Useexec run-httpd "$@".Proposed fix
export HTTPD_RUN_BY_S2I=1 -exec run-httpd $@ +exec run-httpd "$@"📝 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.export HTTPD_RUN_BY_S2I=1 exec run-httpd "$@"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/s2i/bin/run` around lines 5 - 7, Update the exec invocation in the run script to pass positional arguments as a quoted expansion, preserving spaces, wildcard characters, and original argument boundaries.src/s2i/bin/usage (1)
1-5: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a compatible shell for the conditional.
The script declares
/bin/shbut uses Bash-only[[ ... =~ ... ]]. On POSIX shells this can fail or evaluate incorrectly, causing RHEL images to display the non-RHEL sample image. Either change the shebang to/bin/bashor rewrite the condition using POSIX[ ... ].Proposed fix
-#!/bin/sh +#!/bin/bash📝 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.#!/bin/bash DISTRO=`cat /etc/*-release | grep ^ID= | grep -Po '".*?"' | tr -d '"'` NAMESPACE=sclorg if [[ $DISTRO =~ rhel* ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/s2i/bin/usage` around lines 1 - 5, Make the shell used by the usage script compatible with the conditional in the DISTRO branch: either change the shebang to /bin/bash to support the existing [[ $DISTRO =~ rhel* ]] expression, or rewrite that condition with POSIX [ ] syntax while preserving correct RHEL detection.test/pre-init-test-app/index.html (1)
1-1: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an HTML doctype to the fixture.
Because this file is served as
index.html, prepend<!doctype html>so browsers do not parse it in quirks mode.🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pre-init-test-app/index.html` at line 1, Prepend the HTML5 doctype declaration to the static fixture content in index.html, before the existing sample text, so browsers render it in standards mode.Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores