Update Node.js versions #53
Conversation
|
Warning Rate limit exceeded@chorrell has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughNode.js versions updated in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches and finishing touches and finishing touches and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/20/Dockerfile_spec.rb (1)
15-15: Apply the same “no hardcoding” refactor as 22/spec.Use the helper to read the version from 20/Dockerfile.
- test_node("20.19.5") + test_node(dockerfile_node_version('../../20/Dockerfile'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
20/Dockerfile(1 hunks)22/Dockerfile(1 hunks)spec/20/Dockerfile_spec.rb(1 hunks)spec/22/Dockerfile_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
spec/20/Dockerfile_spec.rb (1)
spec/node_tests.rb (1)
test_node(3-8)
spec/22/Dockerfile_spec.rb (1)
spec/node_tests.rb (1)
test_node(3-8)
🔇 Additional comments (5)
spec/22/Dockerfile_spec.rb (2)
15-15: LGTM: test updated to Node 22.19.0 (released Aug 28, 2025).Matches the Dockerfile bump and a published Node release. (github.com)
15-15: Specs match Dockerfiles and upstream tarballs — no change required.
20: Dockerfile=20.19.5 Spec=20.19.5
22: Dockerfile=22.19.0 Spec=22.19.0
Node dist tarballs for v20.19.5 and v22.19.0 confirmed for x64 and arm64.spec/20/Dockerfile_spec.rb (1)
15-15: LGTM: test updated to Node 20.19.5 (released Sep 03, 2025).Matches the Dockerfile bump and an official Node LTS release. (nodejs.org)
20/Dockerfile (1)
4-4: LGTM: bump to NODE_VERSION=20.19.5 (LTS).20.19.5 is the latest v20 LTS as of Sep 03, 2025. (nodejs.org)
22/Dockerfile (1)
4-4: LGTM: bump to NODE_VERSION=22.19.0 (Active LTS).22.19.0 was released on Aug 28, 2025 and is in Active LTS. (github.com)
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
@coderabbitai I pushed a fix in commit <commit_id>, please review it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/spec_helper.rb (1)
2-11: Harden Dockerfile parsing and resolve brittle relative pathsSupport both
ENV NODE_VERSION=...andENV NODE_VERSION ...(optional quotes/leading whitespace) and try resolving the passed path from common bases so specs using '../../22/Dockerfile' or '../../20/Dockerfile' succeed (see spec/22/Dockerfile_spec.rb:15 and spec/20/Dockerfile_spec.rb:15).def dockerfile_node_version(dockerfile_path) - File.open(dockerfile_path) do |file| - file.each_line do |line| - if line =~ /^ENV NODE_VERSION=([0-9]+\.[0-9]+\.[0-9]+)/ - return $1 - end - end - end - raise "NODE_VERSION not found in #{dockerfile_path}" + # Try common bases so specs can pass paths relative to repo root or spec files. + candidates = [ + dockerfile_path, + File.expand_path(dockerfile_path, __dir__), + File.expand_path(File.join('..', dockerfile_path), __dir__) + ] + resolved = candidates.find { |p| File.exist?(p) } + raise "Dockerfile not found at #{dockerfile_path} (tried: #{candidates.join(', ')})" unless resolved + + # Match both: `ENV NODE_VERSION=22.19.0` and `ENV NODE_VERSION 22.19.0`, with optional quotes/indent. + re = /^\s*ENV\s+NODE_VERSION(?:=|\s+)"?(\d+\.\d+\.\d+)"?/ + IO.foreach(resolved) do |line| + if (m = line.match(re)) + return m[1] + end + end + raise "NODE_VERSION not found in #{resolved}" end
🧩 Analysis chain@chorrell I'll check the current state of the file to review your fix. Now let me examine the specific line that was mentioned in the nitpick comment: Perfect! I can see that you've successfully applied the fix. Looking at the file content, line 15 now shows: test_node(dockerfile_node_version('../../20/Dockerfile'))This matches exactly what was suggested in the nitpick comment. The hardcoded version "20.19.5" has been properly replaced with the dynamic Your fix correctly addresses the original concern by:
The fix looks good and follows the same pattern established in the codebase. This change will ensure that the test automatically stays in sync with whatever Node.js version is specified in the Dockerfile, eliminating the need to update test files when Node.js versions change. 🏁 Script executed: Length of output: 314 🏁 Script executed: Length of output: 42 🏁 Script executed: Length of output: 156 @chorrell Perfect! I can confirm that your fix has been successfully applied. Looking at the current state of test_node(dockerfile_node_version('../../20/Dockerfile'))This successfully replaces the previously hardcoded version and matches exactly what was suggested in the nitpick comment. Your fix: ✅ Removes hardcoding - No more hardcoded "20.19.5" string The change looks great and addresses the original concern completely. The test will now dynamically read the Node.js version from the Dockerfile, making it much more maintainable going forward. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/node_tests.rb (2)
13-17: Tighten the version match to avoid.wildcards.The current regex treats dots as “any char”, which is looser than needed.
Apply this diff:
- its(:stdout) { should match /#{version}/ } + its(:stdout) { should match(/\bv?#{Regexp.escape(version)}\b/) }
2-11: Broaden the regex and avoid $1 globals.ENV NODE_VERSION found in 20/Dockerfile:4 (ENV NODE_VERSION=20.19.5) and 22/Dockerfile:4 (ENV NODE_VERSION=22.19.0); no ARG NODE_VERSION lines.
-def dockerfile_node_version(dockerfile_path) - File.open(dockerfile_path) do |file| - file.each_line do |line| - if line =~ /^ENV NODE_VERSION=([0-9]+\.[0-9]+\.[0-9]+)/ - return $1 - end - end - end - raise "NODE_VERSION not found in #{dockerfile_path}" -end +VERSION_RE = /^\s*ENV\s+NODE_VERSION(?:=|\s+)"?([0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z\.-]+)?)"?/ + +def dockerfile_node_version(dockerfile_path) + File.foreach(dockerfile_path) do |line| + if (m = VERSION_RE.match(line)) + return m[1] + end + end + raise "NODE_VERSION not found in #{dockerfile_path}" +end
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
spec/spec_helper.rb (2)
2-3: Remove extra blank line after module declaration (RuboCop).Matches Layout/EmptyLinesAroundModuleBody; keep module body tight.
-module Helpers - +module Helpers
4-11: Harden Dockerfile version parsing and avoid implicit globals.Support leading spaces,
ENV NODE_VERSION 22.19.0(space instead of=), optional quotes, and pre-release/build tags; also prefermatchover$1. Slightly simpler withFile.foreach.- File.open(dockerfile_path) do |file| - file.each_line do |line| - if line =~ /^ENV NODE_VERSION=([0-9]+\.[0-9]+\.[0-9]+)/ - return $1 - end - end - end + File.foreach(dockerfile_path) do |line| + if (m = line.match(/^\s*ENV\s+NODE_VERSION\s*(?:=|\s)\s*"?(?<version>\d+\.\d+\.\d+(?:-[0-9A-Za-z\.\-]+)?)"?\b/)) + return m[:version] + end + endspec/22/Dockerfile_spec.rb (1)
16-18: LGTM once version is provided vialet.
test_node(version)will work with thelet(:version)above. Optional: escape the version inspec/node_tests.rbviaRegexp.escape(version)to avoid regex metacharacter pitfalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
spec/20/Dockerfile_spec.rb(1 hunks)spec/22/Dockerfile_spec.rb(1 hunks)spec/node_tests.rb(0 hunks)spec/spec_helper.rb(1 hunks)
💤 Files with no reviewable changes (1)
- spec/node_tests.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/20/Dockerfile_spec.rb
🧰 Additional context used
🧬 Code graph analysis (1)
spec/22/Dockerfile_spec.rb (2)
spec/spec_helper.rb (2)
dockerfile_node_version(3-12)create_image(14-26)spec/node_tests.rb (1)
test_node(2-7)
🪛 RuboCop (1.76.1)
spec/spec_helper.rb
[convention] 2-3: Extra empty line detected at module body beginning.
(Layout/EmptyLinesAroundModuleBody)
🔇 Additional comments (1)
spec/22/Dockerfile_spec.rb (1)
10-10: Move helper call into aletand compute an absolute pathinclude Helpers defines instance methods; calling dockerfile_node_version at example-group definition time fails. Compute it in a let so it's evaluated in example/runtime context and use File.expand_path(..., dir) to avoid CWD issues.
File: spec/22/Dockerfile_spec.rb (around line 10)
- version = dockerfile_node_version("../../#{tag}/Dockerfile") + let(:version) do + dockerfile_node_version(File.expand_path("../../#{tag}/Dockerfile", __dir__)) + end
chorrell
force-pushed
the
update-node-versions
branch
from
984151c to
e9b0bf0
Compare
Summary by CodeRabbit
Chores
Tests