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

feat: support node v17 #2380

Merged
merged 10 commits into from
Oct 21, 2021
Merged

feat: support node v17 #2380

merged 10 commits into from
Oct 21, 2021

Conversation

trentm
Copy link
Member

@trentm trentm commented Oct 19, 2021

Node v17.0.0 was released today per https://nodejs.org/en/about/releases/

image

We should officially add support (modulo this is not an LTS release). Adding to "engines" avoids this npm warning:

npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'elastic-apm-node@3.21.1',
npm WARN EBADENGINE   required: { node: '^8.6.0 || 10 || 12 || 14 || 15 || 16' },
npm WARN EBADENGINE   current: { node: 'v17.0.0', npm: '8.1.0' }
npm WARN EBADENGINE }

We will also add node v17 to list of versions we test.

Checklist

@trentm trentm self-assigned this Oct 19, 2021
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Oct 19, 2021
@apmmachine
Copy link
Contributor

apmmachine commented Oct 19, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-21T16:29:52.347+0000

  • Duration: 25 min 23 sec

  • Commit: db85abb

Test stats 🧪

Test Results
Failed 0
Passed 22
Skipped 0
Total 22

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

Otherwise the "test/start/..." tests that change the cwd and cannot find
the relative path to our openssl config file:

    running (cwd: ./test/start/env): node --unhandled-rejections=strict test.test.js
    OpenSSL configuration error:
    C007EDBE107F0000:error:80000002:system library:BIO_new_file:No such file or directory:../deps/openssl/openssl/crypto/bio/bss_file.c:67:calling fopen(./test/openssl-config-for-testing.cnf, rb)
    C007EDBE107F0000:error:10000080:BIO routines:BIO_new_file:no such file:../deps/openssl/openssl/crypto/bio/bss_file.c:75:
    C007EDBE107F0000:error:07000072:configuration file routines:def_load:no such file:../deps/openssl/openssl/crypto/conf/conf_def.c:179:
    /home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/test.js:132
        if (err) throw err

This does not fail with node v16 and earlier. I don't know why. Perhaps
it is the change to OpenSSL v3 in node v17.
@trentm
Copy link
Member Author

trentm commented Oct 20, 2021

Waiting on https://hub.docker.com/_/node "node:17" docker image for our Jenkins CI, which is waiting on docker-library/official-images#11138 to be fixed, which will then go through this process https://github.com/docker-library/faq#an-images-source-changed-in-git-now-what

@trentm
Copy link
Member Author

trentm commented Oct 20, 2021

% node --unhandled-rejections=strict test/instrumentation/modules/hapi/basic-legacy-path.test.js
TAP version 13
# extract URL from request
ok 1 no error from startServer
node:events:368
      throw er; // Unhandled 'error' event
      ^

Error: connect ECONNREFUSED ::1:58759
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1161:16)
    at TCPConnectWrap.callbackTrampoline (node:internal/async_hooks:130:17)
Emitted 'error' event on ClientRequest instance at:
    at Socket.socketErrorListener (node:_http_client:447:9)
    at Socket.emit (node:events:390:28)
    at emitErrorNT (node:internal/streams/destroy:164:8)
    at emitErrorCloseNT (node:internal/streams/destroy:129:3)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  errno: -61,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '::1',
  port: 58759
}

Node.js v17.0.0

@trentm trentm marked this pull request as ready for review October 20, 2021 23:37
@trentm trentm requested a review from astorm October 20, 2021 23:37
@trentm
Copy link
Member Author

trentm commented Oct 20, 2021

I'll add a changelog entry tomorrow, after I merge another change and update this branch, to avoid the merge conflict.

Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

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

Changes look like the usual bit shifting between versions. 👍 Thanks for being on top of the Node.js release train. 🚋

@trentm trentm merged commit 0e59a23 into master Oct 21, 2021
@trentm trentm deleted the trentm/node-17 branch October 21, 2021 18:51
trentm added a commit that referenced this pull request Oct 25, 2021
This was broken in #2380. Versions of hapi <=16 did not support the
'host' option to `new Server`, but instead to `server.connection`.
trentm added a commit that referenced this pull request Oct 25, 2021
#2387)

This was broken in #2380. Versions of hapi <=16 did not support the
'host' option to `new Server`, but instead to `server.connection`.
Also really old versions of fastify (v0.35.7) cannot even be npm installed
with the npm included in node v17. This drops support for fastify <1.0.0
and updates the TAV test matrix to only test a given fastify version
with its supported versions of node per https://www.fastify.io/docs/latest/LTS/
@trentm trentm mentioned this pull request Apr 20, 2022
2 tasks
trentm added a commit that referenced this pull request Oct 13, 2022
…reement on default from server and client (as discussed earlier here #2380 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants