Skip to content

Conversation

@liranmauda
Copy link
Contributor

@liranmauda liranmauda commented Nov 10, 2025

Explain the Changes

  • Update nodejs from v22.11.0 to v24.4.1

Summary by CodeRabbit

  • Chores
    • Updated development Node.js runtime to a newer supported version.
  • Tests
    • Improved CI workflow to better decide whether to pull or rebuild base images based on repository changes, added retry logic for pulls, and clearer status reporting during build/pull steps.

- Update nodejs from v22.11.0 to v24.4.1

Signed-off-by: liranmauda <liran.mauda@gmail.com>
@coderabbitai
Copy link

coderabbitai bot commented Nov 10, 2025

Walkthrough

Updates CI workflow to add conditional logic for pulling vs building base/builder images (file-change gating using .nvmrc and builder.dockerfile, retry pull loop), and updates .nvmrc Node.js version from 22.11.0 to 24.4.1. No runtime code changes.

Changes

Cohort / File(s) Summary
CI workflow changes
.github/workflows/run-pr-tests.yaml
Added decision flow to choose between pulling or locally building noobaa-base/builder images: introduced should_pull flag, file-change gating via base_files (including .nvmrc and builder.dockerfile), pull-with-retry path that runs only when should_pull true, and updated final status output and comments. Also reformatted some permission/concurrency indentation.
Node runtime config
.nvmrc
Bumped Node.js version from 22.11.0 to 24.4.1.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor GitHubActions as "GitHub Actions"
    participant Repo as "Repo / Changed Files"
    participant Decision as "Determine should_pull"
    participant RemoteRegistry as "Image Registry"
    participant LocalBuilder as "Local Build"

    GitHubActions->>Repo: checkout PR
    GitHubActions->>Decision: check base_files changed? (e.g., .nvmrc, builder.dockerfile)
    Decision-->>GitHubActions: should_pull = false (if changed) / true (if not)
    alt should_pull = true
        GitHubActions->>RemoteRegistry: attempt to pull builder image (with retry)
        RemoteRegistry-->>GitHubActions: pull success or failure
        alt pull success
            GitHubActions->>GitHubActions: tag/use pulled image
        else pull failure
            GitHubActions->>LocalBuilder: fall back -> build locally
        end
    else should_pull = false
        GitHubActions->>LocalBuilder: build locally (skip pull)
    end
    GitHubActions->>GitHubActions: set should_build status and continue workflow
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Pay attention to conditional logic and boolean flow around should_pull / should_build.
  • Review file-change detection pattern for base_files to ensure correct paths and globbing.
  • Validate the pull retry loop and fallback behavior to avoid false negatives or unnecessary rebuilds.
  • Check permissions/concurrency indentation changes to ensure no semantic regression in workflow execution.

Possibly related PRs

Suggested reviewers

  • nimrod-becker
  • romayalon

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change in the changeset—updating Node.js version from 22.11.0 to 24.4.1 in .nvmrc and related workflow adjustments.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2017b80 and 2eee045.

📒 Files selected for processing (1)
  • .nvmrc (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: guymguym
Repo: noobaa/noobaa-core PR: 9153
File: src/test/unrelated/for_await_stream.js:69-69
Timestamp: 2025-11-10T09:40:34.683Z
Learning: In the noobaa-core repository, Travis CI (.travis.yml) is unused. The active CI system is GitHub Actions, and the project uses Node.js 22.11.0 as specified in .nvmrc.
📚 Learning: 2025-11-10T09:40:34.683Z
Learnt from: guymguym
Repo: noobaa/noobaa-core PR: 9153
File: src/test/unrelated/for_await_stream.js:69-69
Timestamp: 2025-11-10T09:40:34.683Z
Learning: In the noobaa-core repository, Travis CI (.travis.yml) is unused. The active CI system is GitHub Actions, and the project uses Node.js 22.11.0 as specified in .nvmrc.

Applied to files:

  • .nvmrc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-jest-unit-tests
  • GitHub Check: run-package-lock-validation

@@ -1 +1 @@
22.11.0
24.4.1
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify compatibility and CI alignment for the two-major-version bump.

The update jumps from v22 to v24, which is a significant version bump that may introduce breaking changes. Before merging, please confirm:

  1. Compatibility: Are there any breaking changes or API deprecations between Node.js v22 and v24 that affect this project?
  2. LTS Status: Is v24.4.1 a stable/LTS release or will it require frequent updates?
  3. CI Alignment: Check if GitHub Actions workflows (.github/workflows/) or other CI configuration files also hardcode Node.js versions and need updating for consistency.
  4. Dependencies: Verify that package.json (if present) has compatible engines field or similar constraints.

🏁 Script executed:

#!/bin/bash
# Description: Check for hardcoded Node.js versions in CI config files and package.json

# Search for hardcoded Node.js versions in GitHub Actions workflows
echo "=== Checking GitHub Actions workflows for Node.js versions ==="
fd -e yml -e yaml '.github/workflows' 2>/dev/null | xargs grep -l 'node-version\|node_version' 2>/dev/null || echo "No workflow files found or no matches"

# Check package.json for engines field
echo -e "\n=== Checking package.json for engines field ==="
if [ -f "package.json" ]; then
  jq '.engines' package.json 2>/dev/null || echo "No engines field in package.json"
else
  echo "package.json not found"
fi

# Search for any other version references in CI or config files
echo -e "\n=== Searching for other Node.js version references ==="
rg -i 'node.*22\.|node.*v22|nodejs.*22' '.github/' -A2 -B2 2>/dev/null || echo "No v22 references found in .github/"

Length of output: 246


🌐 Web query:

What are the breaking changes and LTS status of Node.js v24 compared to v22?

💡 Result:

Short answer

  • Breaking changes: see migration highlights — dropped some prebuilt platforms (32-bit Windows x86 removed in v23; 32-bit Linux armv7 removed in v24), OpenSSL upgraded to 3.5 with default security level 2 (short keys/RC4 disallowed), stricter runtime/argument validation (fetch(), AbortSignal, stream/pipe error behavior, Buffer/path fixes), V8 bump (native addons need rebuild/possible API changes — V8 13.6), C/C++ toolchain requirements (GCC ≥12.2, Xcode ≥16.1), and several deprecations/removals with available codemods (fs constants, REPL/REPL APIs, some legacy TLS/fs APIs). Test native addons and code paths that touch crypto, streams, Buffer, and platform-specific binaries when migrating. [1]

  • LTS status and dates: Node.js v24 (codename “Krypton”) entered Active LTS on 2025-10-28 and is in LTS with support through end of April 2028. Node.js v22 (codename “Jod”) is in Maintenance LTS (post‑Active) and its planned End‑of‑Life is April 30, 2027. [2][1]

Sources

  1. Node.js migration guide: “v22 to v24” (breaking changes, OpenSSL, platforms, addons, codemods). [1]
  2. Node.js releases / schedule (v24 LTS date and v22 status/EoL). [2]

Confirm testing of breaking changes before merging the two-major-version upgrade.

The upgrade from v22 to v24 is a significant jump with documented breaking changes that require verification. v24 (codename "Krypton") entered Active LTS on 2025-10-28 with support through April 2028, making it a supported LTS release. However, breaking changes include dropped prebuilt platforms (32-bit Windows x86 removed in v23; 32-bit Linux armv7 removed in v24), OpenSSL upgraded to 3.5 with default security level 2, stricter runtime/argument validation, and V8 bump to 13.6.

Before merging, ensure:

  • Test native addons and crypto/stream operations: Test native addons and code paths that touch crypto, streams, Buffer, and platform-specific binaries when migrating.
  • Verify platform support: Confirm the project does not require 32-bit Windows or Linux armv7 support.
  • CI alignment: No CI workflow updates needed (no v22 hardcoding detected in GitHub Actions workflows).
🤖 Prompt for AI Agents
In .nvmrc around line 1 (currently "24.4.1"), you upgraded Node from v22 to v24
but did not verify breaking changes; before merging, run and document targeted
compatibility tests for native addons and any code touching crypto, streams,
Buffer, and platform-specific binaries (exercise CI, local builds, and
integration tests), confirm the project does not require dropped platforms
(32-bit Windows x86 or Linux armv7), update CI workflows if any hardcoded Node
versions are found or to add runners that match supported platforms, and add a
short PR note/changelog entry listing the verification steps and any CI changes
so reviewers can confirm the checks were completed.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
.github/workflows/run-pr-tests.yaml (2)

117-147: Clarify step purpose with a more descriptive name.

The step "Pull noobaa-builder image" implements conditional logic to decide whether to pull or build the builder image (similar to the "Should build noobaa-base image" step). The name is misleading—it suggests only pulling. Consider renaming to something like "Should build noobaa-builder image" or "Prepare noobaa-builder image" for consistency and clarity.

The underlying logic is sound: it checks if builder.dockerfile or .nvmrc changed, and either pulls a pre-built image (with retry) or marks for rebuild.

-      - name: Pull noobaa-builder image
+      - name: Should build noobaa-builder image
         id: should_build_builder
         if: ${{steps.should_build_base.outputs.should_build == 'true' ||
           steps.pull_base_image.outputs.pull_succeed == 'false'}}

104-114: Code duplication in retry loop logic (deferred refactor).

The retry loops for pulling base and builder images use nearly identical patterns (date-based retry over 20 days). In a future refactor, consider extracting this as a reusable workflow or helper script to reduce duplication and maintenance burden. Not urgent for this PR given "Chill" review mode.

Also applies to: 135-145

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2eee045 and aebc37d.

📒 Files selected for processing (1)
  • .github/workflows/run-pr-tests.yaml (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: guymguym
Repo: noobaa/noobaa-core PR: 9153
File: src/test/unrelated/for_await_stream.js:69-69
Timestamp: 2025-11-10T09:40:34.683Z
Learning: In the noobaa-core repository, Travis CI (.travis.yml) is unused. The active CI system is GitHub Actions, and the project uses Node.js 22.11.0 as specified in .nvmrc.
📚 Learning: 2025-06-19T10:01:46.933Z
Learnt from: nadavMiz
Repo: noobaa/noobaa-core PR: 9099
File: .github/workflows/run-pr-tests.yaml:52-70
Timestamp: 2025-06-19T10:01:46.933Z
Learning: In the noobaa-core repository, branch names don't contain forward slashes, so Docker tag sanitization is not needed in GitHub Actions workflows. Manual full builds and automated builds should use consistent tag naming to avoid mismatches.

Applied to files:

  • .github/workflows/run-pr-tests.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-package-lock-validation
  • GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
.github/workflows/run-pr-tests.yaml (3)

81-97: Good: File-change gating for base image rebuild.

The new logic correctly gates the base image rebuild on changes to critical files including .nvmrc. This ensures that Node.js version bumps trigger a fresh base image build rather than attempting to use a stale cached image.

Also applies to: 99-115


149-156: Approve: Conditional build flags based on pull success.

The logic correctly uses the -o builder flag when the builder image was successfully pulled, avoiding redundant rebuilds. If builder needs rebuilding (or pull failed), it performs a full build.


1-10: ✓ .nvmrc has been correctly updated to 24.4.1.

The verification confirms that .nvmrc was updated from v22.11.0 to v24.4.1 in commit 2eee0451. The workflow correctly gates on this file to trigger rebuilds when the Node.js version changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant