-
Notifications
You must be signed in to change notification settings - Fork 134
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
chore: major dev maintenance for RUM agent #1434
Conversation
e3891bd
to
5fd390e
Compare
5fd390e
to
dda5670
Compare
4c7dc34
to
44e8061
Compare
44e8061
to
f5db046
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
@@ -10,7 +10,7 @@ ARG NODEJS_VERSION | |||
|
|||
# 1. Install node | |||
RUN apt-get -qq update && apt-get -qq install -y curl && \ | |||
curl -sL https://deb.nodesource.com/setup_${NODEJS_VERSION}.x | bash - && \ | |||
curl -sL https://deb.nodesource.com/setup_14.x | bash - && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that you have set the NODE_OPTIONS on benchmark scripts, can you remove the hardcoded version here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I will let the hardcoded version here and remove the NODE_OPTIONS from benchmark scripts until we decide to apply this update to the benchmarking, too. My surprise was big when I saw that this dockerfile is also built in a different step (which provides the Node 18 version) of the CI: https://github.com/elastic/apm-agent-rum-js/actions/runs/6284894505/job/17066717885
EDIT: In fact, thinking more about this, I would let things are they are:
- Having the comments will help to have context
- Once we do the docker update we will need to change the installation setup anyway (like we do in the puppeteer one, so the hardcoded 14 will go away anyway)
And, just for more visibility, this is the capture of the Node.js 14 warning I mentioned in the PR's description:
ARG NODEJS_VERSION | ||
FROM node:${NODEJS_VERSION} | ||
FROM ubuntu:jammy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you talk to the Robots team about using this custom image?
The other option is to fix our obserability ci image which is from our shared pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mention anything explicitly because, in the other image, we were using ubuntu:bionic
which is another "custom" image.
I can share this with Robots, and discuss if it would be required to align approaches in future iterations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, It's fine to use any distro base image from a trusted provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Node and npm
legacy-peer-deps=true
in a new .npmrc. This allows us to update things gradually. In a nutshell, that flag makes npm behave like it was doing in versions 4-6.E2E Developer Experience
webdriverio
andwdio
dependencies to the latest one. Mainly for improving the reporting and exit code behaviour, more info:- [🐛 Bug]: onComplete Hook Provides an exit code of 1 When Test Suite Passes When Using Cucumber Tag Expression Due to Error in onRunnerStart hook of wdio-video-reporter webdriverio/webdriverio#9440
- [🐛 Bug]: if session initialisation failed, the result of the test is not be part of the reporter webdriverio/webdriverio#8307 (we are victims of this one every now and then and it's so frustrating, it's so time-consuming)
webdriver
async mechanism https://webdriver.io/docs/async-migration/Webpack compatibility with Node 18
- our
webpack
config uses now the first solution(xxhash64
) offered by the creator ofwebpack
. More info here- for the
angular package
the second solution(--openssl-legacy-provider
flag) is the one applied since updatingangular-cli
dependencies is not an option given the huge the task is.Final thoughts
This lack of maintenance was causing several DX problems and with the passing of time to fix this was becoming more and more difficult because of the enormous gap between the versions.
Possible future maintenance candidates: lerna, angular cli, benchmarking, and load-testing (the last two still use Node 14. Adapting Playwright and its related dependencies to work with Node.js 18 is not a piece of cake, maybe this should be the one to do first because the source from where we installed Node.js 14 is going to become inactive sooner rather than later, you can see that warning in the logs), etc. No need to rush though, it can be done gradually since this PR now solves the main blockers we had.