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

Remove nodejs v16 support. #94

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove nodejs v16 support. #94

wants to merge 2 commits into from

Conversation

ramhr
Copy link
Contributor

@ramhr ramhr commented Oct 22, 2024

User description

nodejs 16 has reached end-of-life on September last year, and this month it reaches the end of it's maintenance mode as well, removing all official support.

In order to keep maintaining the package and update dependencies we reached a point where we need to stop support for it as well.


PR Type

enhancement, configuration changes


Description

  • Removed support for Node.js v16 as it has reached end-of-life.
  • Updated the CI workflow to only test against Node.js versions 18.x and 20.x.
  • Changed the Node.js engine requirement in package.json to version 18 or higher.

Changes walkthrough 📝

Relevant files
Configuration changes
ci.yml
Update CI workflow to remove Node.js v16 support                 

.github/workflows/ci.yml

  • Removed Node.js v16 from the CI workflow matrix.
  • Updated supported Node.js versions to 18.x and 20.x.
  • +1/-1     
    Enhancement
    package.json
    Update Node.js engine requirement to version 18 or higher

    package.json

    • Updated Node.js engine requirement to version 18 or higher.
    +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    nodejs 16 has reached end-of-life on September last year, and this month it reaches the end of it's maintenance mode as well, removing all official support.
    
    In order to keep maintaining the package and update dependencies we reached a point where we need to stop support for it as well.
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    CI Configuration
    Verify that removing Node.js v16 from the CI workflow doesn't break any existing tests or processes that might still depend on this version.

    Dependency Compatibility
    Ensure that all dependencies in the project are compatible with Node.js v18 and above, as the engine requirement has been updated.

    Copy link

    codiumai-pr-agent-pro bot commented Oct 22, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure consistency between CI node versions and package.json engine requirements

    Consider adding a step to verify that the package.json node engine version matches
    the minimum version in the CI matrix. This ensures consistency between the CI
    environment and the project requirements.

    .github/workflows/ci.yml [17-19]

     strategy:
       matrix:
         node-version: [18.x, 20.x]
    +- name: Verify node version consistency
    +  run: |
    +    min_version=$(node -e "console.log(require('./package.json').engines.node.replace('>=', ''))")
    +    if [[ ! "${matrix.node-version[0]}" =~ ^$min_version ]]; then
    +      echo "Minimum node version in package.json doesn't match CI matrix"
    +      exit 1
    +    fi
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion adds a verification step to ensure that the Node.js version specified in the CI matrix matches the minimum version in package.json. This is a good practice to maintain consistency and prevent potential issues due to version mismatches.

    7
    Specify an upper bound for the Node.js version to ensure compatibility

    Consider specifying an upper bound for the Node.js version to prevent potential
    compatibility issues with future Node.js releases.

    package.json [55-57]

     "engines": {
    -  "node": ">=18"
    +  "node": ">=18 <21"
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding an upper bound to the Node.js version in package.json can help prevent compatibility issues with future Node.js releases. This is a reasonable precaution, although it may limit the flexibility of using newer Node.js versions.

    6

    💡 Need additional feedback ? start a PR chat

    Copy link

    CI Failure Feedback 🧐

    Action: Build and run unit tests (18.x)

    Failed stage: Run npm test [❌]

    Failed test name: "before all" hook: beforeAll in "{root}"

    Failure summary:

    The action failed due to the following reasons:

  • The "before all" hook in the test suite failed because the RabbitMQ health check did not pass.
  • The specific error was that the Docker container arnavmq-tests was not running, leading to a failure
    in executing the command docker exec arnavmq-tests rabbitmqctl status.
  • This resulted in an error response from the Docker daemon and the command exited with error code 1.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    297:  shell: /usr/bin/bash -e {0}
    298:  ##[endgroup]
    299:  > arnavmq@0.16.4 test
    300:  > dot-only-hunter test && nyc mocha --recursive --exit
    301:  Hunting inside 'test' for tests with `.only`...
    302:  All good!
    303:  1) "before all" hook: beforeAll in "{root}"
    304:  0 passing (20s)
    305:  1 failing
    306:  1) "before all" hook: beforeAll in "{root}":
    307:  Error: RabbitMQ health check failed after 20 iterations: Command failed: docker exec arnavmq-tests rabbitmqctl status
    308:  Error response from daemon: container 6d26d427154ac837445d3f6456260ba82524e171dc38cfc4ea53bddc46afe773 is not running
    309:  `docker exec arnavmq-tests rabbitmqctl status` (exited with error code 1)
    ...
    
    327:  utils.js            |    62.5 |        0 |      50 |    62.5 | 19-22             
    328:  src/modules/hooks    |   29.57 |        0 |    3.84 |   30.43 |                   
    329:  base_hooks.js       |     8.1 |        0 |      10 |    8.57 | 4-19,42-120       
    330:  connection_hooks.js |      50 |      100 |       0 |      50 | 11-33             
    331:  consumer_hooks.js   |   42.85 |      100 |       0 |   42.85 | 14-80             
    332:  index.js            |     100 |      100 |     100 |     100 |                   
    333:  producer_hooks.js   |      50 |      100 |       0 |      50 | 16-44             
    334:  ----------------------|---------|----------|---------|---------|-------------------
    335:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    @@ -16,7 +16,7 @@ jobs:

    strategy:
    matrix:
    node-version: [16.x, 18.x, 20.x]
    node-version: [18.x, 20.x]
    Copy link
    Member

    Choose a reason for hiding this comment

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

    please test also version 22

    @shamil shamil removed their assignment Oct 22, 2024
    @shamil shamil removed their assignment Oct 22, 2024
    @yosiat yosiat assigned ramhr and unassigned yosiat Oct 23, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    4 participants