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

Add node 18 to tests and remove EoL node versions #3048

Merged
merged 12 commits into from
Jun 23, 2022

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Jun 17, 2022

Based on discussions we've had in several SIG meetings.

https://endoflife.date/nodejs

  • API will continue to support old runtimes as long as is practical
  • Allows us to update our CI tooling to versions which no longer support these deprecated runtimes

@dyladan dyladan requested a review from a team June 17, 2022 18:00
@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #3048 (64c6384) into main (b891509) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3048      +/-   ##
==========================================
+ Coverage   92.65%   92.66%   +0.01%     
==========================================
  Files         187      187              
  Lines        6178     6178              
  Branches     1304     1304              
==========================================
+ Hits         5724     5725       +1     
+ Misses        454      453       -1     
Impacted Files Coverage Δ
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

@pichlermarc
Copy link
Member

Will the package.json "engines" fields be updated in a separate PR? 🤔

@dyladan
Copy link
Member Author

dyladan commented Jun 21, 2022

Will the package.json "engines" fields be updated in a separate PR? 🤔

@open-telemetry/javascript-maintainers what do you think? Should the engines match the testing matrix? NPM doesn't check the engines field AFAIK but yarn does

@pichlermarc
Copy link
Member

Will the package.json "engines" fields be updated in a separate PR? thinking

@open-telemetry/javascript-maintainers what do you think? Should the engines match the testing matrix? NPM doesn't check the engines field AFAIK but yarn does

I think the engines field also serves as documentation to some degree. We should also update the "Supported Runtimes" and "Node Support" Sections in README.md to reflect the changes in the testing setup.

@dyladan
Copy link
Member Author

dyladan commented Jun 22, 2022

@legendecas @Flarna since your approvals I've added a lot of changes (particularly "engines" field) PTAL and make sure you're still happy with your approval

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Imho engines should be updated like it now is. The check can be bypassed if the user feels confident about it, but since they will be stepping into a wild west, it should be a conscious decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants