sclorg
Adding NODE_CMD option as env variable for specifying run command.
Two new env variables have been added for specifying on how to start the Node.js application.
NODE_CMDINIT_WRAPPER
These two env variables can be used as follow, assuming you already have a build image created with s2i:
docker run -e NODE_CMD="node server.js" -e INIT_WRAPPER=true node-16-rhel9
By using NODE_CMD env variable:
- We avoid running npm, which results in having several pros
In addition to NODE_CMD env variable, you can combine it with the INIT_WRAPPER env variable which wraps/runs the value of NODE_CMD variable with this script. That way we achieve:
- reaping zombie processes
- Properly forwarding signals such as (SIGINT, SIGTERM) to the node.js application inside the container.
@pacostas I think we should add this for 18 and 18-minimal as well.
@pacostas In addition to being able to run with docker run -e NODE_CMD="node server.js". We'd also want to be able to use when building a container with S2i or docker. We should probably find the right place in the documentation to add some explanation of how to use it those three cases. Just let me know if you have an questions about that.
@hhorak thanks for the pointer to the init-wrapper.
The commit 72896d2 is not very descriptive, what it does is adding the init-wrapper on node-18. It is also tested.
@mhdawson Thanks for indicating on adding NODE_CMD in the documentation. I added it on the readme page of each node version under the NPM_RUN variable. https://github.com/sclorg/s2i-nodejs-container/pull/359/commits/2e3958e89619685745d98a75948d6d62ab406805
Below is the text I've added for documenting the NODE_CMD variable, feel free to review it, by adding a comment here and I'll update it in all the places it is being used.
NODE_CMD
When specified (e.g.Specify NODE_CMD="node server.js") the NODE_CMD is executed by the init-wrapper script, which handles reaping zombie processes and signal forwarding (SIGINT, SIGTERM) to Node.js application.
@mhdawson https://github.com/sclorg/s2i-nodejs-container/pull/359/commits/06f9b3d4ecfe7780b48cd36b989a697ce4f9b589 https://github.com/sclorg/s2i-nodejs-container/pull/359/commits/d404db7f20ade18b07e957e00e20b234e28f5f89 https://github.com/sclorg/s2i-nodejs-container/pull/359/commits/cf13d07597009bea4c5182b0f341e989a6311919
I've added these commits, with docs about the init-wrapper and with some examples.
@pacostas I'm looking for the part which enables INIT_WRAPPER to be used in the build part of S2i. I assume that somehow INIT_WRAPPER being set in env during built must result in INIT_WRAPPER being set in the built in environment variables for the resulting container but I can't find the part that would make that happen?
@pacostas I'm looking for the part which enables INIT_WRAPPER to be used in the build part of S2i. I assume that somehow INIT_WRAPPER being set in env during built must result in INIT_WRAPPER being set in the built in environment variables for the resulting container but I can't find the part that would make that happen?
@mhdawson I think you mean this one? https://github.com/pacostas/s2i-nodejs-container/blob/a59f33d729564d7509b56680cfd84a7406f9a5ca/18/README.md?plain=1#L263-L275
@pacostas what I meant were the changes that allow
s2i -e INIT_WRAPPER=true build . buildImage node-app docker run node-app
to work? Or were there none needed specifically for that?
@pacostas what I meant were the changes that allow
s2i -e INIT_WRAPPER=true build . buildImage node-app docker run node-app
to work? Or were there none needed specifically for that?
@mhdawson Exactly, the s2i propagates by itself the env variables that have been set on the build process to the output image. So none changes had to be done for that.
@pacostas did you built a containers to test/try out in the build process? If is it something you can give me access to in order to experiment?
@mhdawson sure
cd ./s2i-nodejs-container
git remote add pacostas https://github.com/pacostas/s2i-nodejs-container.git
git fetch --all
git checkout command-option-on-14-and-16
# the name of the branch is a bit wrong, it should also include the number 18
cd 16
# Create the builder image
docker build -f Dockerfile.fedora . -t fedora16build
# Create the app image
s2i -e INIT_WRAPPER=true build https://github.com/pacostas/hello-node-16.git fedora16build myapp16fedora
# run the the app image
docker run myapp16fedora
I also wondered if we could pull what npm start would have used from package.json ?
Yes we can do that with bash code, working on it.
@mhdawson by looking at the npm start documentation https://docs.npmjs.com/cli/v9/commands/npm-start?v=true I tried to reproduce the start command and I ended up with below implementation which will replace the current one of this pr https://github.com/pacostas/s2i-nodejs-container/blob/a59f33d729564d7509b56680cfd84a7406f9a5ca/18/s2i/bin/run#L31-L45
elif [ "$INIT_WRAPPER" == true ]; then
package_json_start=$(cat package.json | grep "start" | sed -r 's/(.*)"start"([^:]*[^"]*")(.*)",/\3/')
package_json_main=$(cat package.json | grep "main" | sed -r 's/(.*)"main"([^:]*[^"]*")(.*)",/\3/')
if [ -n "$package_json_start" ]; then
start_command=$package_json_start
elif [ -n $package_json_main ]; then
start_command="node ."
elif [ -f "server.js" ]; then
start_command="node server.js"
else
echo "Failed to find file for starting the Node.js application"
exit 1
fi
echo "launching via init wrapper..."
exec ${STI_SCRIPTS_PATH}/init-wrapper $start_command
else
@pacostas what you have for start looks reasonable to me.
@lholmquist since I'm out next week could you do the final review once @pacostas integrates the latest change?
@pacostas just one small typo I noticed during my final review otherwise looks good !
One other thing to check @pacostas should we add a test for this feature in - https://github.com/sclorg/s2i-nodejs-container/blob/master/test/run
One other thing to check @pacostas should we add a test for this feature in - https://github.com/sclorg/s2i-nodejs-container/blob/master/test/run
@mhdawson Thank you for the URL, although I'm not sure how tests work and how to write them on this codebase. But theoretically, to test this implementation I would go with 2 tests:
- unit testing the run_node function https://github.com/pacostas/s2i-nodejs-container/blob/0bc965615e69639e0cb76ebd63e5f03990f15166/18/s2i/bin/run#L18 where in that case this would require refactoring it to the point where the run_node would return a string (and later on another function executing that string). That way we ensure that if statements work properly on each PR.
- running the https://github.com/pacostas/s2i-nodejs-container/blob/0bc965615e69639e0cb76ebd63e5f03990f15166/18/s2i/bin/run#L48 command as we do with https://github.com/pacostas/s2i-nodejs-container/blob/0bc965615e69639e0cb76ebd63e5f03990f15166/18/s2i/bin/run#L51 for all the tests.
I wouldn't go with integration tests because we would need more tests to have the same coverage, as with above 2 tests.
Also, we can avoid unit tests if there is no place for that type of tests in the project. IMO the logic inside run_node function is very simple and small, to sacrifice time and effort for adding unit tests on the project just for testing one simple function.
bottom line: IMO just running all the tests with exec ${STI_SCRIPTS_PATH}/init-wrapper $start_command as we do with exec npm run -d $NPM_RUN , we are good.
@phracek I'm trying to add some tests and I cant find a way to run them locally while development. Is there any option for running them locally in a Fedora OS?
@pacostas I think we figured that out and both myself and @lholmquist have run the tests locally. If you need any help just sync with one of us two. It does remind me that we should PR in the steps to the s2i repo.
@phracek I've managed to add tests for the NODE_CMD and INIT_WRAPPER env variables on this commit https://github.com/sclorg/s2i-nodejs-container/pull/359/commits/92e17b9bd0854c18758b63270838fd5c80d9af68
I was able to reproduce it locally the issue for the 18-minimal on rhel8 and rhel9. Just pushed with the fixes.
Also I had to force push, as I rebased the branch on main for adding the init-wrapper script and NODE_CMD option on node 20 version
@phracek The service for seeing the logs of the failed tests is not reachable server IP address could not be found. Could also be this reason why the tests appear as failed, as in the past if I can recall correctly the majority of them werent failing.