New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding NODE_CMD option as env variable for specifying run command. #359
base: master
Are you sure you want to change the base?
Conversation
@pacostas I think we should add this for 18 and 18-minimal as well. @pacostas In addition to being able to run with @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. 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.
|
2e3958e
to
cf13d07
Compare
@@ -313,6 +316,47 @@ Below is an example _package.json_ file with the _main_ attribute and _start_ sc | |||
#### Note: | |||
`oc rsync` is only available in versions 3.1+ of OpenShift. | |||
|
|||
## init-wrapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also mention the INIT_WRAPPER env variables in the Environment variables for Source-to-Image
above even if they only refer to this later section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, exaclty, forgot to add it. Added it on commit a59f33d
echo "exec $NODE_CMD" | ||
fi | ||
elif [ "$INIT_WRAPPER" == true ]; then | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blank space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it on commit a59f33d
18/s2i/bin/run
Outdated
if [ -f "server.js" ]; then | ||
start_file="server.js" | ||
elif [ -f "index.js" ]; then | ||
start_file="main.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a typo should be index.js ? Makes me wonder if there is anywhere we could add some testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also are these three files documented in the documentation somewhere else? Otherwise we might want to add that to the documentation as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondered if we could pull what npm start would have used from package.json ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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
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 thanks for helping me understand 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
|
@mhdawson 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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the one small typo I mentioned has been fixed.
@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 |
[test-all] |
1ac826d
to
594094d
Compare
@mhdawson
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 |
@pacostas Can you please have a look why tests for RHEL7-14 are failing? The logs are here: http://artifacts.osci.redhat.com/testing-farm/13d9c6fd-6bff-4a4d-8bd0-67f979a3144e/ |
[test-openshift] |
5d13280
to
3a47b01
Compare
Co-authored-by: Michael Dawson <mdawson@devrus.com>
Co-authored-by: Michael Dawson <mdawson@devrus.com>
3a47b01
to
25bf629
Compare
@phracek On rhel 7 node 14 locally all tests pass. I cant reproduce that error. My only suspicion is that the |
[test-openshift] |
Hi @lholmquist, I see a lot of work has been done on this PR. Is something blocking the merge currently? Could you please re-review, it seems that @pacostas has done some changes in order to fix the issues with tests. I can re-run the tests again: [test-all] |
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:
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: