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

Setup script fails for new devs #7521

Closed
Matt-Yorkley opened this issue Apr 30, 2021 · 5 comments · Fixed by #7578
Closed

Setup script fails for new devs #7521

Matt-Yorkley opened this issue Apr 30, 2021 · 5 comments · Fixed by #7578

Comments

@Matt-Yorkley
Copy link
Contributor

Matt-Yorkley commented Apr 30, 2021

Description

A couple of potential contributors have hit errors when running the setup script under script/setup (as directed to in the wiki). the script checks for a really outdated version of Node (5.12.0). We actually use 14.x.x in production and in CI, so this check is a bit ridiculous. The setup script fails if that exact version is not present.

We use yarn now, which should work fine with any modern version of Node.

We should just do a basic check that node is installed, and continue if it is. Also the wiki might need an update.

Expected Behavior

New devs can run the setup script as per the wiki.

Actual Behaviour

Setup script fails.

Steps to Reproduce

Animated Gif/Screenshot

Workaround

Severity

Your Environment

  • Version used:
  • Browser name and version:
  • Operating System and version (desktop or mobile):

Possible Fix

@psychoslave
Copy link
Contributor

psychoslave commented May 7, 2021

Related data

  • version in .node-version: 5.12.0
  • last LTS node version: 14.16.1
  • last node version: 16.1.0
  • node version used in production: 14.x.x
  • additional version reported as functional : 10.19.0 (@Matt-Yorkley), 15.14.0 (Jean-Baptiste Bellet)

Involved lines in ./script/setup:

 21 NODE_VERSION=$(cat .node-version)  
[…]
 29 # Check node version                                                            
 30 if ! node --version | grep $NODE_VERSION > /dev/null; then                      
 31   printf "${RED}Open Food Network requires node ${NODE_VERSION}${NO_COLOR}. Have a look at: https://github.com/nodenv/nodenv\n"
 32   exit 1                                                                        
 33 fi    

Meanwhile Circumvention

Edit the .node-version file or the script itself to disable the test or make the version an available version on the system.

Drawbacks: this might bring some additional inconvenience regarding git commits.

Required Actions

  • determine if keeping this test is relevant since "Essentially; as long as you can do yarn install from inside the repo directory and it runs without error, that's the only real requirement." (Matt-Yorkley)
    • if kept, determine which version target
      • patch .node-version
    • otherwise, possibly add an alternative test making sure that yarn is functional

Notes and References

@Matt-Yorkley
Copy link
Contributor Author

I would:

  • Update the docs to say node must be installed. You can check by doing node -v and it'll probably be installed already, otherwise install it with apt install nodejs. The references to nodenv and npm can be removed.
  • Remove this check from the setup script entirely
  • Note that yarn needs to be installed
  • Note that after the bundler gem is installed, the user can do bundle install && yarn install to install all gems and Javascipt dependencies

@psychoslave
Copy link
Contributor

psychoslave commented May 7, 2021

I'll remove this check from the setup script entirely later, but regarding the wiki documentation on Ubuntu it's done.

@psychoslave
Copy link
Contributor

#7578

@viktorsmari
Copy link
Contributor

FYI: On a Ubuntu 20.04 machine I was able to start the project by following the instructions, however I did not run the script/setup (because of the issues listed above) but used rails db:setup instead.

If you are interested, this is my bundle exec rspec output

Finished in 49 minutes 55 seconds (files took 9.99 seconds to load)
4209 examples, 16 failures, 19 pending

And the output of the command used in Github Actions (CI) test:
bundle exec rspec --profile --pattern "engines/*/spec/{,/*/**}/*_spec.rb,spec/features/admin/*/*_spec.rb,spec/lib/{,/*/**}/*_spec.rb"

Finished in 8 minutes 10 seconds (files took 6.6 seconds to load)
898 examples, 0 failures, 2 pending

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 a pull request may close this issue.

3 participants