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

Using specific Node version #262

Merged
merged 2 commits into from Nov 6, 2018
Merged

Using specific Node version #262

merged 2 commits into from Nov 6, 2018

Conversation

didierofrivia
Copy link
Member

@didierofrivia didierofrivia commented Oct 30, 2018

What this PR does / why we need it:

Since we still can't productise Yarn, we need to commit our webpack packs and having different Node version resolves to different packs committed.

Which issue(s) this PR fixes

This will avoid having different packs every time webpack builds them. It also fixed a webpacker - node-sass issue.

Verification steps

  • Install node version 8
  • rm -rf node_modules
  • npm install
  • rake webpack:production
  • It shouldn't change anything

INSTALL.md Outdated

```bash
nvm install 10.13.0
nvm use 10.13.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference between Node 10.x versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, minor TBH, we try to stick with latest LTS

Copy link
Contributor

Choose a reason for hiding this comment

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

so, brew install node@10 should be good too no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I could add as a macos alternative

@mikz
Copy link
Contributor

mikz commented Oct 30, 2018

BTW, any reason why not to just use npm instead of yarn? I don't see clear path how yarn gets productized, so just going back to npm does not sound as that big a deal.

@didierofrivia
Copy link
Member Author

@mikz My idea was after #57 is merged, ditch yarn and use just npm. But @hallelujah mentioned the alternative of somehow productize yarn/ocaml since it's default for Rails 5. Still we haven't explored much the latest alternative.

@mikz
Copy link
Contributor

mikz commented Oct 30, 2018

@didierofrivia I don't see yarn productization as something feasible or our task in the first place. Yarn is just a package manager. It is irrelevant for rails what package manager we use to install dependencies. We only need to change relevant rake tasks to run npm instead.

@didierofrivia
Copy link
Member Author

@mikz I wholeheartedly agree with you 👍

@hallelujah
Copy link
Contributor

hallelujah commented Oct 30, 2018

Please do not forget about https://github.com/3scale/porta/blob/master/openshift/system/contrib/scl_enable#L4

And build an image to try this out before CR1 if you want to have it in CR1 (optional IMO)

josemigallas
josemigallas previously approved these changes Oct 30, 2018
damianpm
damianpm previously approved these changes Oct 30, 2018
@codecov
Copy link

codecov bot commented Oct 30, 2018

Codecov Report

Merging #262 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   92.58%   92.58%   +<.01%     
==========================================
  Files        2359     2359              
  Lines       75471    75471              
==========================================
+ Hits        69874    69875       +1     
+ Misses       5597     5596       -1
Impacted Files Coverage Δ
app/models/payment_transaction.rb 81.72% <0%> (+1.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2946da3...cef0f0b. Read the comment docs.

@didierofrivia
Copy link
Member Author

didierofrivia commented Oct 31, 2018

UPDATE: Latest stable RHEL Node version is 8.3.0, so updating this PR

josemigallas
josemigallas previously approved these changes Oct 31, 2018
INSTALL.md Outdated

The project supports **Version: 8.X.X**.

You might want to use [nvm](https://github.com/creationix/nvm/) to install and work with a specific Node versions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, it should be specific Node version

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I wanted with specific node versions without the a hehe, thanks!

@didierofrivia didierofrivia merged commit 929d4a6 into master Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants