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

define acceptable node versions, and enforce at install time #42912

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

pes10k
Copy link
Contributor

@pes10k pes10k commented Dec 15, 2024

Simple change that

  1. defines allowed node versions in package.json
  2. enforce this in.npmrc

Resolves #42911

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@pes10k pes10k requested a review from a team as a code owner December 15, 2024 22:57
@mihaiplesa mihaiplesa requested a review from goodov December 15, 2024 22:58
@fallaciousreasoning
Copy link

Probably doesn't hurt having this but the recommended build instructions are to checkout brave-core now, so this should be caught during init

Additionally, we should be careful not to let these get out of sync 😆

@pes10k
Copy link
Contributor Author

pes10k commented Dec 15, 2024

ah, gotcha gotcha! Two suggestions then

  1. maybe it'd be good to also force the version check with a .npmrc in brave-core then too?
  2. making sure things don't fall out of sync sounds very good ha :) the wiki/README/other documentation already needs to stay in sync with brave-core's package.json though. I dont know where the "one true" definition should be but, just noting that things already need to stay in sync, and so maybe it can all be hoisted into one central location? WDYT?

@@ -6,10 +6,14 @@
"packages": {
"": {
"name": "brave",
"version": "1.75.0",
"version": "1.75.100",
Copy link
Member

Choose a reason for hiding this comment

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

doesn't look like version should be changed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, not sure what happened there, but fixed!

@pes10k pes10k force-pushed the add-node-engine-check branch from 7adcc7e to 586d088 Compare December 16, 2024 18:07
@pes10k
Copy link
Contributor Author

pes10k commented Dec 17, 2024

@goodov just pinging on this, is there anything further needed from me on this?

@goodov
Copy link
Member

goodov commented Dec 18, 2024

@goodov just pinging on this, is there anything further needed from me on this?

I don't think so. feel free to merge!

@pes10k
Copy link
Contributor Author

pes10k commented Dec 18, 2024

thanks! but no can do yet, i think i still need another approval from @brave/devops-team 🤔

@pes10k
Copy link
Contributor Author

pes10k commented Dec 18, 2024

thanks to ya both!

@pes10k pes10k merged commit 2e2f521 into master Dec 18, 2024
4 checks passed
@pes10k pes10k deleted the add-node-engine-check branch December 18, 2024 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce node version when building
4 participants