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

Bump node version to 18 #496

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Bump node version to 18 #496

merged 5 commits into from
Nov 14, 2023

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Nov 7, 2023

Node 16 is EOL. Fixes #493.

@H-Shay H-Shay marked this pull request as draft November 7, 2023 22:30
@H-Shay H-Shay force-pushed the shay/fix_node branch 2 times, most recently from d180dc8 to 43e04e5 Compare November 8, 2023 19:11

interface Context extends Mocha.Context {
moderator?: MatrixClient,
appservice?: MjolnirAppService
}

dns.setDefaultResultOrder('ipv4first');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed because apparently node 17+ defaults to IVP6 but GH actions doesn't support IPV6 - see discussion at https://github.com/matrix-org/conference-bot/pull/186/files/85059d8f92ddcfb80657a58f60ef891ce704dfec..723a939d4726aa9de86fae0693137d78674154cb
this caused me a lot of pain until I figured it out...

Copy link
Member

Choose a reason for hiding this comment

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

what a nightmare, and weird that it tries to do that...

We should put a similar comment from the conference bot's code here too, for when we inevitably try to remove it by accident. Likewise on the other files.

@H-Shay H-Shay marked this pull request as ready for review November 8, 2023 20:07
@H-Shay H-Shay requested a review from turt2live November 8, 2023 20:08
@FSG-Cat
Copy link

FSG-Cat commented Nov 9, 2023

Just wanted to note that #493 requires a release to be fixed fully. Just bumping node version to 18 doesnt actually fully address the issue due to the share of users who are not running mainline.

@turt2live
Copy link
Member

Our process is to close issues as fixed prior to release, then release as needed. The "Fixes" keyword autocloses the issue upon merge, which is the desired effect here.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm - thanks!


interface Context extends Mocha.Context {
moderator?: MatrixClient,
appservice?: MjolnirAppService
}

dns.setDefaultResultOrder('ipv4first');

Copy link
Member

Choose a reason for hiding this comment

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

what a nightmare, and weird that it tries to do that...

We should put a similar comment from the conference bot's code here too, for when we inevitably try to remove it by accident. Likewise on the other files.

@H-Shay H-Shay merged commit 8bf628e into main Nov 14, 2023
5 checks passed
@H-Shay
Copy link
Contributor Author

H-Shay commented Nov 14, 2023

Added requested comments and merged #496 into main, thanks for the review!

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.

EOL NodeJS version in use for Docker Images on Main and all Existing releases
3 participants