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

Refs #24780 - use native npm #804

Merged
merged 1 commit into from
Sep 26, 2018
Merged

Refs #24780 - use native npm #804

merged 1 commit into from
Sep 26, 2018

Conversation

mmoll
Copy link
Contributor

@mmoll mmoll commented Sep 1, 2018

No description provided.

@mmoll
Copy link
Contributor Author

mmoll commented Sep 1, 2018

With npm 3.10.10 on NodeJS 6.14.4, at least Foreman core tests work fine for me.

@ohadlevy
Copy link
Member

ohadlevy commented Sep 2, 2018

3.10 is fairly old, and i am worried this is a major deviation from what developers are actually using, i would prefer to reduce (and even limit) the amount of npm/node combinations, in fact, I would be in favor of creating a simple docker image that builds assets, it can be reused across all the various distros, and its output can be simply added to the resulting tar/package...?

@mmoll
Copy link
Contributor Author

mmoll commented Sep 2, 2018

NPM 3.10 and NodeJS 6 are the versions coming from EPEL and thus are used to build the RPMs. The "npm install npm" dance was only a workaround and I had hoped that EPEL would somewhen provide a newer version for performance reasons, however the current npm package set seems to install and work just fine with npm 3 again.

Also, this combination is still "supported" until 04/2019.

I would be in favor of creating a simple docker image

I would be strongly against that (at this moment). If we need newer node/npm versions, we should just tap the nodesource.org RPM repos, like we do for the DEB build, as this fits in much better in all the build processes we use currently.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

What @mmoll said. We use the system npm when building RPMs so we should be doing PR testing with it as well. If we require a newer version, we see it earlier. It also results in a faster build. I'm going to merge this and see if we run into issues.

@ekohl ekohl merged commit 31b52c5 into theforeman:master Sep 26, 2018
@mmoll mmoll deleted the npm_install branch September 26, 2018 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants