Skip to content

Conversation

@thavaahariharangit
Copy link
Contributor

@thavaahariharangit thavaahariharangit commented Nov 5, 2025

What are you trying to accomplish?

In my previous PR update, I updated the Dockerfile. However, I overlooked some related changes, which I’ve now addressed in this update.

Anything you want to highlight for special attention from reviewers?

I've addressed @yeikel's feedback in this comment.

Regarding the question about Node 22 and NPM 11 compatibility (reference):

The Dockerfile is already configured correctly - it will automatically pull the latest Node.js 22.x (which is currently 22.21.1) from the nodesource repository. The NODEJS_VERSION=22 just specifies the major version for the repository URL.

The current configuration is optimal:

  • NODEJS_VERSION=22 - This is correct and will install the latest 22.x
  • NPM_VERSION=11.6.2 - This is compatible (requires Node.js >= 22.9.0)

How will you know you've accomplished your goal?

I reproduced the original error in this workflow: GitHub Actions run, where the updater defaulted to installing npm@10.

updater | 2025/11/05 17:33:07 INFO <job_1144412314> Returned (engines) info "npm" : ""
updater | 2025/11/05 17:33:07 INFO <job_1144412314> Guessed version info "npm" : "10"
updater | 2025/11/05 17:33:07 INFO <job_1144412314> Installing "npm@10"
updater | 2025/11/05 17:33:07 INFO <job_1144412314> Started process PID: 1537 with command: {} corepack install npm@10 --global --cache-only {}
  proxy | 2025/11/05 17:33:07 [008] GET [https://registry.npmjs.org:443/npm](https://registry.npmjs.org/npm)
  proxy | 2025/11/05 17:33:07 [008] 200 [https://registry.npmjs.org:443/npm](https://registry.npmjs.org/npm)
  proxy | 2025/11/05 17:33:07 [010] GET [https://registry.npmjs.org:443/npm/-/npm-10.9.4.tgz](https://registry.npmjs.org/npm/-/npm-10.9.4.tgz)
  proxy | 2025/11/05 17:33:07 [010] 200 [https://registry.npmjs.org:443/npm/-/npm-10.9.4.tgz](https://registry.npmjs.org/npm/-/npm-10.9.4.tgz)
  proxy | 2025/11/05 17:33:08 [012] GET [https://registry.npmjs.org:443/npm/10.9.4](https://registry.npmjs.org/npm/10.9.4)
  proxy | 2025/11/05 17:33:08 [012] 200 [https://registry.npmjs.org:443/npm/10.9.4](https://registry.npmjs.org/npm/10.9.4)

After applying the update, the local logs now show that the correct version [email protected] is being installed and used, confirming that the engine constraints are being properly processed and the issue is resolved.

updater | 2025/11/05 23:01:03 INFO Returned (engines) info "npm" : "11"
updater | 2025/11/05 23:01:03 INFO Fetching version for package manager: npm
updater | 2025/11/05 23:01:03 INFO Started process PID: 1307 with command: {} corepack npm -v {}
updater | 2025/11/05 23:01:04 INFO Process PID: 1307 completed with status: pid 1307 exit 0
updater | 2025/11/05 23:01:04 INFO Total execution time: 0.2 seconds
updater | 2025/11/05 23:01:04 INFO Installed version of npm: 11.6.2
updater | 2025/11/05 23:01:04 INFO Installed version for npm: 11.6.2
updater | 2025/11/05 23:01:04 INFO Processing engine constraints for npm
updater | 2025/11/05 23:01:04 INFO Parsed constraints for npm: >=10.1.0
updater | 2025/11/05 23:01:04 INFO Version requirement for npm: >= 10.1.0
updater | 2025/11/05 23:01:04 INFO Running node command: node -v
updater | 2025/11/05 23:01:04 INFO Started process PID: 1319 with command: {} node -v {}
updater | 2025/11/05 23:01:04 INFO Process PID: 1319 completed with status: pid 1319 exit 0
updater | 2025/11/05 23:01:04 INFO Total execution time: 0.01 seconds

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@thavaahariharangit thavaahariharangit requested a review from a team as a code owner November 5, 2025 23:08
NPM_V8 = "8"
NPM_V9 = "9"
NPM_V10 = "10"
NPM_V11 = "11"
Copy link
Contributor

@yeikel yeikel Nov 5, 2025

Choose a reason for hiding this comment

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

Can we add a test that validates that this works?

ie:

Using

"engines": {
"npm": ">=11.0.0"
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

@thavaahariharangit Please review github/docs#41144 as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we add a test that validates that this works?

ie:

Using

"engines": {
"npm": ">=11.0.0"
  }

Test added: b846671

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thavaahariharangit Please review github/docs#41144 as well

I have approved this PR, It's a draft PR, we need to wait @yeikel to update this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding the test 🙏

And yes, as soon as this change is merged, I'll move the docs pull request to ready

@yeikel
Copy link
Contributor

yeikel commented Nov 5, 2025

One change related to this is #13404

If we can merge #13404 then we can ensure that this pull request does not also need to update bun/Dockerfile as I don't believe that bun needs to have and track a version for npm

@yeikel
Copy link
Contributor

yeikel commented Nov 5, 2025

This change should close #13205 and #13158

@thavaahariharangit
Copy link
Contributor Author

#13404

I am happy with this changes, I will merge and deploy with this changes.

Copy link
Contributor

@yeikel yeikel left a comment

Choose a reason for hiding this comment

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

Looks great now. Thanks!

@thavaahariharangit thavaahariharangit merged commit ac61dd5 into main Nov 6, 2025
90 of 92 checks passed
@thavaahariharangit thavaahariharangit deleted the harry/bump-npm-version-to-latest branch November 6, 2025 15:25
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.

4 participants