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

Update things related to some package manager influencing setup process #7578

Merged
merged 3 commits into from
May 11, 2021

Conversation

psychoslave
Copy link
Contributor

@psychoslave psychoslave commented May 7, 2021

What? Why?

Fixes #7521

The version specified in the .node-version file is no longer supported, and trying to install it through nodenv or the like will not allow to run the project on a bright new system.

This patch:

  • set node version to the last LST 14.16.1
  • follows the recommendations of @Matt-Yorkley to remove the node test in ./script/setup.
  • replaces npm with yarn
  • remove everything related to newrelic

What should we test?

Anything that allows to make sure no dependencies require a version

Release notes

Fix hindrances on new setup related to Node version

Changelog Category: Technical changes

Dependencies

Documentation updates

Already done:

Possibly might also require some edits:

@psychoslave
Copy link
Contributor Author

Github specific shortcut syntax to issues still seem to be a mystery for me 😆

@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #7578 (a4d6e69) into master (5a16954) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7578   +/-   ##
=======================================
  Coverage   93.20%   93.20%           
=======================================
  Files         635      635           
  Lines       18136    18136           
=======================================
  Hits        16903    16903           
  Misses       1233     1233           

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 5a16954...a4d6e69. Read the comment docs.

@psychoslave psychoslave changed the title Update things related to node version Update things related to some package manager influencing setup process May 7, 2021
@viktorsmari
Copy link
Contributor

@psychoslave are you using nodenv or another tool like nvm?
I was thinking if https://github.com/nvm-sh/nvm has become the standard with its .nvmrc file.

Last update to the nodenv repo was July 2020 and it has 1.5k stars
Last update to the nvm repo 5 days ago and it has 48k stars

We might also be able to symlink these files, but I thought the .nvmrc file was the 'unofficial' winner 😸

@psychoslave
Copy link
Contributor Author

Hello @viktorsmari

I did installed nodenv as it was recommended in the documentation for dealing with dependencies on Ubuntu. That said @Matt-Yorkley reported that using the official Ubuntu Node package is enough.

The project currently doesn't have a .nvmrc, but it can be added, this is all up to the community. For a matter of simplicity, I would recommend to avoid to add multiple concurrent solution in the repository. Maybe nodenv was retained because its sub-commands are aligned with rbenv which is also highlighted in the documentation.

If some consensus arise in OFN community that it would be great to move to nvm as a recommanded tool, then I would suggest to remove references to nodenv in the main path of the documentation and push that instead.

In any case, it's always possible to add specific documentation in a peripheral wiki page/section explaining how to set up a symlink that won't be tracked by git so no one is bothered with all possible preferences.

Anyway, as interesting as the topic might be, I think it should be treated in its own issue and pull request. This one doesn't intend to address something that it is not directly pertaining to the #7578.

@viktorsmari
Copy link
Contributor

viktorsmari commented May 8, 2021

I agree, fewer files are better and symlinking might be an overcomplication 😄

Since this PR is removing the only reference to nodenv in the repo, maybe the .node-version file is not needed at all, what does @jibees say, do you use any particular Node version manager?

This could be part of another PR / discussion, I just thought it was related.

We could also remove references to nodenv in the wiki, I found them in 2 places:

Maybe detailing in the wiki how each version manager is installed on multiple operating systems should not be the responsibility of OFN? These things change and every time I personally install rbenv, I would rather follow the official documentation of rbenv.

Maybe it would be enough if we link to the official docs of each package manager, instead of copy pasting their installation instructions. Just a thought 😃

Copy link
Contributor

@Matt-Yorkley Matt-Yorkley left a comment

Choose a reason for hiding this comment

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

Thanks!

@andrewpbrett
Copy link
Contributor

Github specific shortcut syntax to issues still seem to be a mystery for me 😆

I fixed that, I think the problem was you had the extra [ and ] around the issue number. You just need #1234, for example.

@andrewpbrett andrewpbrett merged commit 9ae7c5e into openfoodfoundation:master May 11, 2021
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.

Setup script fails for new devs
4 participants