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

chore(deps): upgrade PM2 #8910

Merged
merged 4 commits into from
Sep 28, 2023
Merged

chore(deps): upgrade PM2 #8910

merged 4 commits into from
Sep 28, 2023

Conversation

jamesgorrie
Copy link
Contributor

@jamesgorrie jamesgorrie commented Sep 26, 2023

What does this change?

Runs yarn add --force pm2 to reinstall [email protected] with their dependency vulnerabilities fixed.

pm2 don't bump their package but rather fix in place.

The snyk reports were generated locally by running snyk monitor dotcom-rendering hence they're in the guardian-people project.

Why?

To have no high vulnerabilities.

Screenshots

Before After
before after
Snyk report Snyk report

@jamesgorrie jamesgorrie requested a review from a team as a code owner September 26, 2023 11:10
@jamesgorrie jamesgorrie added the run_chromatic Runs chromatic when label is applied label Sep 26, 2023
@github-actions
Copy link

github-actions bot commented Sep 26, 2023

Size Change: 0 B 🆕

Total Size: 0 B

compressed-size-action

@jamesgorrie jamesgorrie added run_chromatic Runs chromatic when label is applied and removed run_chromatic Runs chromatic when label is applied labels Sep 26, 2023
Copy link
Member

@arelra arelra left a comment

Choose a reason for hiding this comment

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

Actually I think the pm2 version we use on our live instances is installed to /usr/local/node/pm2 as part of the AMI. Would that always be the latest (i.e fixed) version?

@jamesgorrie
Copy link
Contributor Author

jamesgorrie commented Sep 26, 2023

Actually I think the pm2 version we use on our live instances is installed to /usr/local/node/pm2 as part of the AMI. Would that always be the latest (i.e fixed) version?

@arelra I think it definitely is, so technically we don't actually have this vulnerability on the live instances.

Saying that, I do think we should remove that from those boxes as it's a very opaque dependency and could cause any number of hard-to-debug issues through being implicitly used there.

I think we would need to ensure we can the installed pm2 on those boxes before doing that though.

I am going to merge this as it remove our reported, if not real, vulnerabilities in Snyk.

@arelra
Copy link
Member

arelra commented Sep 26, 2023

@arelra I think it definitely is, so technically we don't actually have this vulnerability on the live instances.

Great thanks for confirming.

Saying that, I do think we should remove that from those boxes as it's a very opaque dependency and could cause any number of hard-to-debug issues through being implicitly used there.

Agreed we should be using the version that is in our dependencies.

I am going to merge this as it remove our reported, if not real, vulnerabilities in Snyk.

Thanks for sorting this!

yarn.lock Outdated
Comment on lines 16188 to 16193
[email protected]:
version "6.3.0"
resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.0.tgz#ee0a64c8af5e8ceea67687b133761e1becbd1d3d"
integrity sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw==

semver@^6.0.0, semver@^6.1.1, semver@^6.1.2, semver@^6.3.0, semver@^6.3.1:
Copy link
Contributor

Choose a reason for hiding this comment

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

@jamesgorrie I'm not sure I follow this change 🤔. It looks like the net effect would be to downgrade the version of semver to 6.3.0, which was originally upgraded away from in #8715?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch @JamieB-gu yes @pm2/io@~5.0.0 is still using [email protected]. In which case we need to keep the resolution.

Copy link
Member

@arelra arelra Sep 26, 2023

Choose a reason for hiding this comment

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

Interestingly [email protected]'s dependency @pm2/io@~5.0.0 has been updated to address the semver vulnerability and is now at @pm2/[email protected]:

keymetrics/pm2-io-apm@c5996e3

As pm2 accepts a patch this should mean we can specifically install/upgrade pm2/io@~5.0.2 to pick up this update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there has been a few issues where yarn was following semver so not updating it's lockfile as it was technically backwards compatible.

So basically the same thing you're talking about @arelra.

I removed pm2 and re-added it which seems to have given us a more expected lockfile, but annoyingly we do still have high vulnerabilities, which I think are related to licensing.

@jamesgorrie
Copy link
Contributor Author

jamesgorrie commented Sep 27, 2023

As part of this we discovered we still have a .snyk file we should sort out #8924

@jamesgorrie jamesgorrie merged commit 62fd77c into main Sep 28, 2023
27 checks passed
@jamesgorrie jamesgorrie deleted the updgrade-pm2 branch September 28, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run_chromatic Runs chromatic when label is applied
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants