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

tools: update GitHub Actions workflows #2231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VoltrexKeyva
Copy link
Contributor

Thanks for contributing!

  • Have you updated CHANGELOG.md?

@VoltrexKeyva
Copy link
Contributor Author

This also fixes a typo in the changelog.

@VoltrexKeyva VoltrexKeyva force-pushed the update-actions branch 5 times, most recently from 03457be to 5c0e40f Compare April 18, 2023 12:36
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Do you have a specific motivation for wanting to drop older versions of Node.js? Right now it's not adding to our maintenance burden, and folks have complained when we stopped providing prebuilds for EOL versions, so we've continued to test them in CI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file isn't used in this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then is there a reason it exists in this branch? Should we remove it?

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@VoltrexKeyva
Copy link
Contributor Author

VoltrexKeyva commented Apr 18, 2023

Do you have a specific motivation for wanting to drop older versions of Node.js? Right now it's not adding to our maintenance burden, and folks have complained when we stopped providing prebuilds for EOL versions, so we've continued to test them in CI.

My main motivation for that is to be able to update dependencies without having to worry about it breaking support for older Node.js versions since most of them are removing support for versions like v10 and v12, as well as being able to use newer features not available in older versions that are more convenient to use and offer better performance (such as the nullish coalescing operator (??) and others), since most users use the newer versions rather than the older ones nowadays, but I would understand if you folks still want to support older versions.

@LinusU
Copy link
Collaborator

LinusU commented Apr 19, 2023

I personally think that we shouldn't rush to release a new major version since it creates extra churn for the consumers of this package. We only have three dependencies right now, and only one of them has a newer version available, simple-get, and there is nothing in the new version that would benefit us at this time...

@VoltrexKeyva
Copy link
Contributor Author

Fair enough, I'll revert the changes that removes the old Node.js versions from the testing matrix.

@VoltrexKeyva VoltrexKeyva force-pushed the update-actions branch 3 times, most recently from da5223c to 25f739c Compare April 19, 2023 08:05
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

LGTM 👍

The only thing I'm not 100% sure about is the check-latest: true, not sure that it's worth busting the cache more often just to always be on the latest version. In fact, I'm not sure that it's nice to automatically be on the latest version because of the same reasons discussed before, even if breakage from newer Node.js versions should be very rare...

@VoltrexKeyva
Copy link
Contributor Author

@LinusU the main reason I think getting the latest version is better is because if we use the cached one, a scenario can occur where it lands on a Node.js version with regressions and/or critical issues and keeps using that, however always getting the latest version should avoid this problem as such issues in the runtime generally get fixed pretty quickly.

I also encountered this problem myself which was very annoying to deal with.

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.

3 participants