-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
tools: Update README.node_modules #20627
tools: Update README.node_modules #20627
Conversation
@allisonkarlitskaya gentle review ping? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but maybe a minor tweak?
tools/README.node_modules.md
Outdated
when building straight from a tarball it is also not necessary to have Node.js | ||
installed. | ||
Cockpit does not use npm or Node.js at runtime. When building/installing from | ||
a release tarball it is also not necessary to have Node.js installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you want to press the point that we do use the modules at runtime, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's what the "and for runtime JavaScript libraries." in the previous paragraph was about. I changed this whole section again, hopefully more clear now?
- Clean up unnecessarily complicated grammar/language - Fix some outright wrong statements like "Cockpit does not use npm packages code at runtime" -- we very clearly do (React/PF). - `npm-install.yml` is no more; replace with description of reposchutz. - node-cache.git is shared between all org projects - Setting up a new one from scratch only applies to forks or other orgs. Otherwise the key deployment is driven by a bots script.
So that viewing in GitHub works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is substantially more explicit, thanks!
npm-install.yml
is no more; replace with description of reposchutz.Preview: https://github.com/martinpitt/cockpit/blob/node-docs/tools/README.node_modules.md