Adding NODE_CMD option as env variable for specifying run command. #359

pacostas · 2022-09-29T16:10:20Z

Two new env variables have been added for specifying on how to start the Node.js application.

NODE_CMD
INIT_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 · 2022-09-29T16:10:45Z

#120

pacostas · 2022-09-29T16:10:54Z

#275

mhdawson · 2022-10-14T20:04:47Z

@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.

pacostas · 2022-10-17T11:25:49Z

The commit 72896d2 is not very descriptive, what it does is adding the init-wrapper on node-18. It is also tested.

pacostas · 2022-10-18T11:32:45Z

@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. 2e3958e

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.

pacostas · 2022-11-22T18:09:57Z

@mhdawson 06f9b3d d404db7 cf13d07

I've added these commits, with docs about the init-wrapper and with some examples.

18/README.md

18-minimal/README.md

18/s2i/bin/run

mhdawson · 2022-11-24T19:06:48Z

18/s2i/bin/run

+    if [ -f "server.js" ]; then
+      start_file="server.js"
+    elif [ -f "index.js" ]; then
+      start_file="main.js"


looks like a typo should be index.js ? Makes me wonder if there is anywhere we could add some testing?

Also are these three files documented in the documentation somewhere else? Otherwise we might want to add that to the documentation as well.

I also wondered if we could pull what npm start would have used from package.json ?

looks like a typo should be index.js ? Makes me wonder if there is anywhere we could add some testing?

Fixed it on commit a59f33d

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.

Also are these three files documented in the documentation somewhere else? Otherwise we might want to add that to the documentation as well.

We only mention the presence of these files when we describe the init-wrapper functionality. I dont think we should mention something more because is specific to the scope of how init-wrapper works - looks for the files.

18/README.md

mhdawson · 2022-11-24T19:14:04Z

@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 · 2022-11-25T16:19:40Z

@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

mhdawson · 2022-11-28T22:33:56Z

@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 · 2022-11-29T10:28:43Z

@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.

mhdawson · 2022-11-29T23:09:20Z

@pacostas thanks for helping me understand that.

mhdawson · 2022-11-29T23:10:06Z

@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?

pacostas · 2022-11-30T08:55:04Z

@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

pacostas · 2022-11-30T11:19:08Z

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

mhdawson · 2022-12-02T19:50:30Z

@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?

14-minimal/README.md

mhdawson

LGTM once the one small typo I mentioned has been fixed.

mhdawson · 2022-12-19T19:28:06Z

@pacostas just one small typo I noticed during my final review otherwise looks good !

mhdawson · 2022-12-19T19:30:32Z

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

phracek · 2023-01-02T09:32:18Z

[test-all]

pacostas · 2023-01-03T15:57:47Z

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.

Co-authored-by: Michael Dawson <[email protected]>

…regex

…of init-wrapper

…iable

pacostas · 2024-09-20T08:12:08Z

@lholmquist can you do a [test-all] ?

lholmquist · 2024-09-20T16:35:45Z

[test-all]

pacostas · 2024-09-24T13:34:40Z

@lholmquist Looks like the errors are not related to PR changes. Can you do once more the [test-all]

lholmquist · 2024-09-24T13:54:23Z

[test-all]

github-actions · 2024-09-25T14:19:16Z

Pull Request validation

Success

🟢 CI - All checks have passed
🟢 Review - Reviewed by a member
🟢 Approval - Changes were approved

Auto Merge

Success

🟢 Pull Request is not marked as draft and it's not blocked by dont-merge label
🟢 Pull Request meet requirements, title has correct form
🟢 Pull Request meet requirements, mergeable is true
🟢 Pull Request meet requirements, mergeable_state is clean
🟢 Pull Request has correct target branch master
🟢 Pull Request was merged

pacostas force-pushed the command-option-on-14-and-16 branch from 2e3958e to cf13d07 Compare November 22, 2022 15:57