Skip to content
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

fix: Update the package.json files so that correct Node version gets selected for the Nodejs sample apps #197

Closed
wants to merge 1 commit into from

Conversation

Akash-Nayak
Copy link
Contributor

Signed-off-by: Akash Nayak akash19nayak@gmail.com

…selected for these Nodejs apps

Signed-off-by: Akash Nayak <akash19nayak@gmail.com>
@github-actions
Copy link

Thanks for making a pull request! 😃

@github-actions github-actions bot added the fix label Oct 21, 2022
@Akash-Nayak Akash-Nayak changed the title fix: Update the package.json files so that correct Node version gets selected for these Nodejs apps fix: Update the package.json files so that correct Node version gets selected for the Nodejs sample apps Oct 21, 2022
@Akash-Nayak
Copy link
Contributor Author

If no node version is mentioned in the package.json file, Move2Kube will select the default node version which will be the latest version from the nodeversions.yaml. Our sample nodejs applications aren't compatible with node v16 and above, so I have added the node version constraint to the package.json file.

@HarikrishnanBalagopal
Copy link
Contributor

HarikrishnanBalagopal commented Oct 27, 2022

Our sample nodejs applications aren't compatible with node v16 and above

The NodeJS sample in language-platforms is compatible, we run end-to-end tests which deploy them on a cluster and return 200 OK.

Also even if it is true that they can't run with the latest NodeJS then that is a bug that needs to be fixed. The sample apps are simple static web servers for the most part, there is no reason to have complex version restrictions.

Copy link
Contributor

@HarikrishnanBalagopal HarikrishnanBalagopal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are issues with making these samples work with Node 18 and beyond we should fix those bugs. Adding the version constraint does not make sense for such simple static web server samples.

@Akash-Nayak
Copy link
Contributor Author

Akash-Nayak commented Oct 28, 2022

If there are issues with making these samples work with Node 18 and beyond we should fix those bugs. Adding the version constraint does not make sense for such simple static web server samples.

I deployed the NodeJS sample with Node 16 (registry.access.redhat.com/ubi8/nodejs-16) and it didn't work on the OpenShift cluster. But, then I deployed the same NodeJS sample to Kubernetes cluster and it worked. I think the end-to-end tests deploys it on a Kubernetes cluster and return 200 OK.

Screenshot 2022-10-28 at 9 26 18 AM

Screenshot 2022-10-28 at 9 22 57 AM

@Akash-Nayak
Copy link
Contributor Author

Akash-Nayak commented Oct 28, 2022

With registry.access.redhat.com/ubi8/nodejs-14 and registry.access.redhat.com/ubi8/nodejs-12 image, it works on both OpenShift as well as Kubernetes cluster.

Screenshot 2022-10-28 at 9 35 04 AM

Screenshot 2022-10-28 at 9 37 42 AM

That's why I added the version restriction to the package.json, so that if someone tries to deploy the sample on OpenShift cluster it should not fail to run.

@HarikrishnanBalagopal
Copy link
Contributor

HarikrishnanBalagopal commented Oct 28, 2022

If there are issues with making these samples work with Node 18 and beyond we should fix those bugs. Adding the version constraint does not make sense for such simple static web server samples.

I deployed the NodeJS sample with Node 16 (registry.access.redhat.com/ubi8/nodejs-16) and it didn't work on the OpenShift cluster. But, then I deployed the same NodeJS sample to Kubernetes cluster and it worked. I think the end-to-end tests deploys it on a Kubernetes cluster and return 200 OK.

Screenshot 2022-10-28 at 9 26 18 AM Screenshot 2022-10-28 at 9 22 57 AM

This just confirms that it's not a Node version issue but an Openshift issue (since the same code works on Kubernetes).
Regardless adding the version constraint is not the correct thing to do. If there are bugs that are preventing the same code from running on Openshift we should fix it.

@HarikrishnanBalagopal
Copy link
Contributor

If there are issues with making these samples work with Node 18 and beyond we should fix those bugs. Adding the version constraint does not make sense for such simple static web server samples.

I deployed the NodeJS sample with Node 16 (registry.access.redhat.com/ubi8/nodejs-16) and it didn't work on the OpenShift cluster. But, then I deployed the same NodeJS sample to Kubernetes cluster and it worked. I think the end-to-end tests deploys it on a Kubernetes cluster and return 200 OK.

Screenshot 2022-10-28 at 9 26 18 AM Screenshot 2022-10-28 at 9 22 57 AM

Looking at the error, it's just a common permissions issue that people face:
https://stackoverflow.com/questions/42363105/permission-denied-mkdir-in-container-on-openshift

We can fix it by adding the proper chmod for /opt/app-root/src/.npm.
Another way to fix it is by changing where Node writes the logs to.

@HarikrishnanBalagopal
Copy link
Contributor

Fixed during session over Webex.

@Akash-Nayak
Copy link
Contributor Author

#197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants