Dist gen generation#541
Open
tjuhaszrh wants to merge 9 commits into
Open
Conversation
Contributor
Author
|
[test] |
Testing Farm results
|
Contributor
Author
|
Tests seem to mostly pass, the failures look like infra issues. |
tjuhaszrh
force-pushed
the
dist-git2
branch
4 times, most recently
from
a5b6c0f to
6852710
Compare
Contributor
Author
|
[test] |
Contributor
Author
|
@phracek Could you do a review please? |
phracek
requested changes
Jun 4, 2026
phracek
left a comment
phracek
left a comment
Member
There was a problem hiding this comment.
Just a few comments. C8S is not supported at all.
Member
|
@tjuhaszrh Please update |
Contributor
Author
|
[test] |
Contributor
Author
|
I don't think the README check targets changes from this PR. |
Contributor
Author
Removed all mentions of c8s and regenerated files, so c8s dockerfiles should be gone. |
Member
|
@tjuhaszrh Please by |
Contributor
Author
|
[test] |
Member
|
[test] |
Member
|
[test-pytest] |
Contributor
Author
|
[test] |
phracek
requested changes
Jun 15, 2026
phracek
left a comment
phracek
left a comment
Member
There was a problem hiding this comment.
See my comments. Let's make the src/Dockerfile more readable. Let's remove a lot of duplicty.
But thanks for your good work.
| help="For more information visit https://github.com/sclorg/s2i-nodejs-container" \ | ||
| usage="s2i build <SOURCE-REPOSITORY> {% if spec.prod in ["c8s", "c9s", "c10s"] %}quay.io/{% endif %}{{ spec.img_name }}{% if spec.prod != "rhel10" %}:latest{% endif %} <APP-NAME>" | ||
|
|
||
| {% if spec.prod in ["c9s", "rhel10", "c10s"] %} |
| {% if spec.prod in ["c9s", "rhel10", "c10s"] %} | ||
| # Package libatomic_ops was removed | ||
| {% endif %}{% if spec.prod in ["rhel10", "c10s"] and spec.version == "22" %} | ||
| RUN INSTALL_PKGS="make gcc gcc-c++ git openssl-devel nodejs nodejs-nodemon nodejs-npm nss_wrapper-libs which" && \ |
Member
There was a problem hiding this comment.
what about to add none npm package to mamultispec.yml file and add only nodejs version or unversioned packages here.
| dnf -y clean all --enablerepo='*' | ||
| {% elif spec.environment_setup %} | ||
| {% if spec.prod == "rhel8" and spec.version == "20" -%} | ||
| RUN yum -y module enable nodejs:$NODEJS_VERSION && \ |
Member
There was a problem hiding this comment.
This is repeating. It would be nice to collect them together. See e.g. sclorg/mariadb-container#331
| INSTALL_PKGS="$MODULE_DEPS {{ spec.pkgs }}" && \ | ||
| ln -s /usr/lib/node_modules/nodemon/bin/nodemon.js /usr/bin/nodemon && \ | ||
| ln -s /usr/libexec/platform-python /usr/bin/python3 && \ | ||
| yum install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \ |
Member
There was a problem hiding this comment.
This code is duplicated several times.
Add scripts which will be copied to version specific fodlers upon generation. Scripts are distro-agnostic as to be usable for all resulted versions.
Readme files also have to be genereted from as agnostic baseline as possible.
Manifest files dictate source baseline dockerfile and resulting path of genereted files, also sets access rights.
Add multispec.yaml file which defines values later used in conditions of DockerFiles in src and to fill in the version specific data.
Move scripts to version agnostic pathway which allows copying during generation.
Remove c8s files and configurations from the project.
Regenerate README.md with updated version table.
📝 WalkthroughWalkthroughIntroduces a ChangesSource template and manifest infrastructure
Corrections to existing version-specific artifacts
Sequence Diagram(s)sequenceDiagram
participant S2I as S2I Build System
participant assemble as assemble script
participant run as run / run-minimal
participant init_wrapper as init-wrapper
participant node as Node.js Process
S2I->>assemble: invoke (source in /tmp/src)
assemble->>assemble: restore node_modules from /tmp/artifacts
assemble->>assemble: configure npm proxy / registry / auth token
assemble->>assemble: npm install (dev or prod path)
assemble->>assemble: npm run build + prune (prod only)
assemble->>assemble: fix-permissions
S2I->>run: container entrypoint
run->>run: source generate_container_user (if present)
run->>run: resolve DEV_MODE / NODE_ENV defaults
alt DEV_MODE=true
run->>node: nodemon --inspect
else INIT_WRAPPER=true and NODE_CMD set
run->>init_wrapper: exec NODE_CMD in background
init_wrapper-->>node: start process, trap SIGTERM/SIGINT
init_wrapper-->>run: exit with child status
else NODE_CMD set
run->>node: exec NODE_CMD directly
else package.json / server.js derived
run->>node: exec derived start command
else
run->>node: npm run -d $NPM_RUN
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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)
src/README.md (1)
5-5: ⚡ Quick winFix spelling and grammar errors in both README templates.
Multiple spelling errors and hyphenation issues reduce clarity and professional appearance. Consistent errors across both files:
src/README.md#L206, src/README.minimal.md#L278: "souce" → "source"src/README.md#L264, src/README.minimal.md#L336: "avalable" → "available"src/README.md#L149, src/README.minimal.md#L222: "environemnt variale" → "environment variable"src/README.md#L301, src/README.minimal.md#L372: "another versions" → "other versions"src/README.md#L5,L7, src/README.minimal.md#L7: "Fedora based" → "Fedora-based"src/README.md#L166, src/README.minimal.md#L238: "key value pairs" → "key-value pairs"🔧 Proposed fixes for spelling and grammar in src/README.md
-Users can choose between RHEL, CentOS and Fedora based images. +Users can choose between RHEL, CentOS and Fedora-based images. -and the Fedora images are available in [Quay.io](https://quay.io/organization/fedora). +and the Fedora-based images are available in [Quay.io](https://quay.io/organization/fedora). -In case of `NODE_CMD` environemnt variale is specified, then `init-wrapper` script will use the value of `NODE_CMD` to start your application. +In case of `NODE_CMD` environment variable is specified, then `init-wrapper` script will use the value of `NODE_CMD` to start your application. -One way to define a set of environment variables is to include them as key value pairs in your repo's `.s2i/environment` file. +One way to define a set of environment variables is to include them as key-value pairs in your repo's `.s2i/environment` file. -As part of development mode, this image supports hot deploy. If development mode is enabled, any souce code that is changed in the running container will be immediately reflected in the running nodejs application. +As part of development mode, this image supports hot deploy. If development mode is enabled, any source code that is changed in the running container will be immediately reflected in the running nodejs application. -A detailed explanation on how the init-wrapper script works is avalable in +A detailed explanation on how the init-wrapper script works is available in -In that repository you also can find another versions of Node.js environment Dockerfiles. +In that repository you also can find other versions of Node.js environment Dockerfiles.🔧 Proposed fixes for spelling and grammar in src/README.minimal.md
-Users can choose between RHEL, CentOS and Fedora based images. +Users can choose between RHEL, CentOS and Fedora-based images. -In case of `NODE_CMD` environemnt variale is specified, then `init-wrapper` script will use the value of `NODE_CMD` to start your application. +In case of `NODE_CMD` environment variable is specified, then `init-wrapper` script will use the value of `NODE_CMD` to start your application. -One way to define a set of environment variables is to include them as key value pairs in your repo's `.s2i/environment` file. +One way to define a set of environment variables is to include them as key-value pairs in your repo's `.s2i/environment` file. -As part of development mode, this image supports hot deploy. If development mode is enabled, any souce code that is changed in the running container will be immediately reflected in the running nodejs application. +As part of development mode, this image supports hot deploy. If development mode is enabled, any source code that is changed in the running container will be immediately reflected in the running nodejs application. -A detailed explanation on how the init-wrapper script works is avalable in +A detailed explanation on how the init-wrapper script works is available in -In that repository you also can find another versions of Node.js environment Dockerfiles. +In that repository you also can find other versions of Node.js environment Dockerfiles.🤖 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/README.md` at line 5, Fix all spelling and grammar errors across src/README.md and src/README.minimal.md. In src/README.md at lines 5-7, change "Fedora based" to "Fedora-based". Additionally, in src/README.md, correct "souce" to "source" at line 206, "avalable" to "available" at line 264, "environemnt variale" to "environment variable" at line 149, "another versions" to "other versions" at line 301, and "key value pairs" to "key-value pairs" at line 166. In src/README.minimal.md, apply the same corrections at the corresponding locations: line 7 for "Fedora-based", line 278 for "source", line 336 for "available", line 222 for "environment variable", line 372 for "other versions", and line 238 for "key-value pairs".
🤖 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 `@src/Dockerfile`:
- Around line 71-73: The conditional guard on line 71 checks for
`config.post_install` but the RUN directive on line 73 renders
`spec.post_install`, causing a namespace mismatch. Since the post_install field
is defined in the spec namespace in specs/multispec.yml, the condition will
always evaluate to false and skip the post-install hook. Change the conditional
on line 71 from `{% if config.post_install %}` to `{% if spec.post_install %}`
to match the namespace being used in the RUN statement.
In `@src/s2i/bin/run`:
- Around line 31-37: The startup command detection has two issues: First, on
line 36, the condition `[ -n $package_json_main ]` uses an unquoted variable,
which when empty expands to `[ -n ]` and evaluates to true incorrectly—add
quotes around the variable to properly check if it is non-empty. Second, lines
31-32 execute sed commands on package.json without verifying the file exists
first, which will cause the script to exit under set -e if the file is
missing—add a file existence check before the sed commands that read the
package.json file to ensure the script handles missing package.json gracefully.
---
Nitpick comments:
In `@src/README.md`:
- Line 5: Fix all spelling and grammar errors across src/README.md and
src/README.minimal.md. In src/README.md at lines 5-7, change "Fedora based" to
"Fedora-based". Additionally, in src/README.md, correct "souce" to "source" at
line 206, "avalable" to "available" at line 264, "environemnt variale" to
"environment variable" at line 149, "another versions" to "other versions" at
line 301, and "key value pairs" to "key-value pairs" at line 166. In
src/README.minimal.md, apply the same corrections at the corresponding
locations: line 7 for "Fedora-based", line 278 for "source", line 336 for
"available", line 222 for "environment variable", line 372 for "other versions",
and line 238 for "key-value pairs".
🪄 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: 48becdda-8e9b-440a-9d42-410371d84779
📒 Files selected for processing (45)
22-minimal/.exclude-c8s22-minimal/.exclude-c9s22-minimal/.exclude-fedora22-minimal/Dockerfile.c8s22-minimal/s2i/bin/init-wrapper22-minimal/s2i/bin/run22/.exclude-c8s22/.exclude-c9s22/.exclude-fedora22/Dockerfile.c8s22/Dockerfile.rhel822/Dockerfile.rhel922/s2i/bin/init-wrapper22/s2i/bin/run22/test24-minimal/.exclude-c8s24-minimal/.exclude-rhel824-minimal/Dockerfile.c8s24-minimal/s2i/bin/init-wrapper24-minimal/s2i/bin/run24/.exclude-c8s24/.exclude-rhel824/Dockerfile.c8s24/Dockerfile.rhel824/Dockerfile.rhel924/s2i/bin/init-wrapper24/s2i/bin/run24/testREADME.mdmanifest-minimal.ymlmanifest.ymlspecs/multispec.ymlsrc/Dockerfilesrc/README.mdsrc/README.minimal.mdsrc/root/opt/app-root/etc/generate_container_usersrc/root/opt/app-root/etc/npm_global_module_listsrc/root/opt/app-root/etc/scl_enablesrc/root/usr/bin/fix-permissionssrc/s2i/bin/assemblesrc/s2i/bin/init-wrappersrc/s2i/bin/runsrc/s2i/bin/run-minimalsrc/s2i/bin/save-artifactssrc/s2i/bin/usage
💤 Files with no reviewable changes (4)
- 24-minimal/Dockerfile.c8s
- 24/Dockerfile.c8s
- 22-minimal/Dockerfile.c8s
- 22/Dockerfile.c8s
Comment on lines
+71
to
+73
| {% if spec.prod == "c8s" or (spec.prod == "rhel8" and spec.version == "20") or (spec.prod == "c9s" and spec.version == "20") or (spec.prod == "rhel9" and spec.version == "20") %}yum{% else %}dnf{% endif %} -y clean all --enablerepo='*'{% if config.post_install %} | ||
|
|
||
| RUN {{ spec.post_install }}{% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm post_install field source and template namespace usage.
rg -n -C2 'post_install|config\.post_install|spec\.post_install' src/Dockerfile specs/multispec.yml manifest.yml manifest-minimal.ymlRepository: sclorg/s2i-nodejs-container
Length of output: 1325
🏁 Script executed:
rg -n 'config\.' src/DockerfileRepository: sclorg/s2i-nodejs-container
Length of output: 342
🏁 Script executed:
rg -n 'spec\.' src/Dockerfile | head -30Repository: sclorg/s2i-nodejs-container
Length of output: 2958
Change post-install guard condition to use correct variable namespace.
Line 71 checks {% if config.post_install %} but line 73 renders {{ spec.post_install }}. The post_install field is defined in specs/multispec.yml under the spec namespace, so the guard condition will always be false and skip the post-install hook. Change the condition on line 71 to {% if spec.post_install %}.
🤖 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/Dockerfile` around lines 71 - 73, The conditional guard on line 71 checks
for `config.post_install` but the RUN directive on line 73 renders
`spec.post_install`, causing a namespace mismatch. Since the post_install field
is defined in the spec namespace in specs/multispec.yml, the condition will
always evaluate to false and skip the post-install hook. Change the conditional
on line 71 from `{% if config.post_install %}` to `{% if spec.post_install %}`
to match the namespace being used in the RUN statement.
Comment on lines
+31
to
+37
| package_json_start=$(sed -n 's/\s*"start"\s*:\s*"\(.*\)".*/\1/p' package.json) | ||
| package_json_main=$(sed -n 's/\s*"main"\s*:\s*"\(.*\)".*/\1/p' package.json) | ||
|
|
||
| if [ -n "$package_json_start" ]; then | ||
| start_command=$package_json_start | ||
| elif [ -n $package_json_main ]; then | ||
| start_command="node ." |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
unset package_json_main
if [ -n $package_json_main ]; then
echo "unquoted check => true (bug)"
else
echo "unquoted check => false"
fi
if [ -n "$package_json_main" ]; then
echo "quoted check => true"
else
echo "quoted check => false (expected)"
fi
bash -c 'set -e; sed -n "s/.*/x/p" package.json; echo "still running"' 2>/dev/null || \
echo "missing package.json causes non-zero path under set -e"Repository: sclorg/s2i-nodejs-container
Length of output: 126
🏁 Script executed:
cat -n src/s2i/bin/run | head -50Repository: sclorg/s2i-nodejs-container
Length of output: 2147
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Testing unquoted [ -n \$var ] without set -u ==="
unset package_json_main
if [ -n $package_json_main ]; then
echo "unquoted empty var => true"
else
echo "unquoted empty var => false (correct)"
fi
echo ""
echo "=== Testing sed on missing file under set -e ==="
bash -c 'set -e; sed -n "s/.*/x/p" nonexistent.json; echo "still running"' 2>&1 || \
echo "FAILED: script exited due to sed failure (as expected)"
echo ""
echo "=== Verifying lines 34-37 logic ==="
# Simulate the scenario
package_json_start=""
package_json_main=""
echo "package_json_start='$package_json_start'"
echo "package_json_main='$package_json_main'"
if [ -n "$package_json_start" ]; then
echo "Result: uses package_json_start"
elif [ -n $package_json_main ]; then
echo "Result: uses package_json_main (node .)"
else
echo "Result: falls through to other checks"
fiRepository: sclorg/s2i-nodejs-container
Length of output: 435
Startup command detection fails when package_json_main is empty, and script exits if package.json is missing.
Line 36 uses [ -n $package_json_main ] unquoted. When the variable is empty, this expands to [ -n ], which evaluates to true because it tests whether the string "-n" itself is non-empty—incorrectly forcing start_command="node .". Additionally, lines 31–32 execute sed on package.json without checking file existence first, so a missing file will cause the script to exit under set -e.
Proposed fix
- package_json_start=$(sed -n 's/\s*"start"\s*:\s*"\(.*\)".*/\1/p' package.json)
- package_json_main=$(sed -n 's/\s*"main"\s*:\s*"\(.*\)".*/\1/p' package.json)
+ package_json_start=""
+ package_json_main=""
+ if [ -f "package.json" ]; then
+ package_json_start=$(sed -n 's/[[:space:]]*"start"[[:space:]]*:[[:space:]]*"\(.*\)".*/\1/p' package.json)
+ package_json_main=$(sed -n 's/[[:space:]]*"main"[[:space:]]*:[[:space:]]*"\(.*\)".*/\1/p' package.json)
+ fi
@@
- elif [ -n $package_json_main ]; then
+ elif [ -n "$package_json_main" ]; then
start_command="node ."📝 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.
Suggested change
| package_json_start=$(sed -n 's/\s*"start"\s*:\s*"\(.*\)".*/\1/p' package.json) | |
| package_json_main=$(sed -n 's/\s*"main"\s*:\s*"\(.*\)".*/\1/p' package.json) | |
| if [ -n "$package_json_start" ]; then | |
| start_command=$package_json_start | |
| elif [ -n $package_json_main ]; then | |
| start_command="node ." | |
| package_json_start="" | |
| package_json_main="" | |
| if [ -f "package.json" ]; then | |
| package_json_start=$(sed -n 's/[[:space:]]*"start"[[:space:]]*:[[:space:]]*"\(.*\)".*/\1/p' package.json) | |
| package_json_main=$(sed -n 's/[[:space:]]*"main"[[:space:]]*:[[:space:]]*"\(.*\)".*/\1/p' package.json) | |
| fi | |
| if [ -n "$package_json_start" ]; then | |
| start_command=$package_json_start | |
| elif [ -n "$package_json_main" ]; then | |
| start_command="node ." |
🤖 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 31 - 37, The startup command detection has two
issues: First, on line 36, the condition `[ -n $package_json_main ]` uses an
unquoted variable, which when empty expands to `[ -n ]` and evaluates to true
incorrectly—add quotes around the variable to properly check if it is non-empty.
Second, lines 31-32 execute sed commands on package.json without verifying the
file exists first, which will cause the script to exit under set -e if the file
is missing—add a file existence check before the sed commands that read the
package.json file to ensure the script handles missing package.json gracefully.
phracek
requested changes
Jun 17, 2026
phracek
left a comment
phracek
left a comment
Member
There was a problem hiding this comment.
Last round. Now it looks really awesome.
| com.redhat.dev-mode.port="DEBUG_PORT:5858" \ | ||
| com.redhat.component="{{ spec.redhat_component }}" \ | ||
| name="{{ spec.img_name }}" \ | ||
| {% if spec.prod != "rhel10" %} version="1" \ |
Member
There was a problem hiding this comment.
This can be removed at all. We do not needed.
| {% endif %} com.redhat.license_terms="https://www.redhat.com/en/about/red-hat-end-user-license-agreements#UBI" \ | ||
| maintainer="SoftwareCollections.org <sclorg@redhat.com>" \ | ||
| help="For more information visit https://github.com/sclorg/s2i-nodejs-container" \ | ||
| usage="s2i build <SOURCE-REPOSITORY> {% if spec.prod in ["c8s", "c9s", "c10s"] %}quay.io/{% endif %}{{ spec.img_name }}{% if spec.prod != "rhel10" %}:latest{% endif %} <APP-NAME>" |
Member
There was a problem hiding this comment.
maybe directly specified for each manifest version and then use something like {{ spec.full_image_name }}
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
2 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.
Cleaning up the draft PR.
Summary by CodeRabbit
New Features
Documentation
Chores