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.

Sign up for GitHub

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

Jump to bottom

chore: bump minimum node version to 14.x #1768

Merged
merged 3 commits into from
May 2, 2022
Merged

chore: bump minimum node version to 14.x #1768

merged 3 commits into from
May 2, 2022

Conversation

Chriscbr
Copy link
Contributor

@Chriscbr Chriscbr commented Apr 14, 2022 •
edited
Loading

Fixes #1739

BREAKING CHANGE: new versions of projen now require node v14, since node v12 reaches EOL on April 30, 2022.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Chriscbr Chriscbr added the pr/do-not-merge ⚙️ used by automation label Apr 14, 2022
@Chriscbr Chriscbr requested a review from a team April 14, 2022 01:51
@mergify mergify bot added the contribution/core ⚙️ used by automation label Apr 14, 2022
@Chriscbr Chriscbr force-pushed the rybickic/upgrade-node14 branch from 71f9ddc to ad54dbd Compare April 14, 2022 01:55
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022 •
edited
Loading

Codecov Report

Merging #1768 (e7235ef) into main (d90284c) will increase coverage by 5.43%.
The diff coverage is 91.83%.

@@            Coverage Diff             @@
##             main    #1768      +/-   ##
==========================================
+ Coverage   88.06%   93.49%   +5.43%     
==========================================
  Files         132      157      +25     
  Lines        5109    27497   +22388     
  Branches     1207     1494     +287     
==========================================
+ Hits         4499    25709   +21210     
- Misses        610     1788    +1178
Impacted Files Coverage Δ
src/java/index.ts 100.00% <ø> (ø)
src/python/index.ts 100.00% <ø> (ø)
src/release/bump-version.ts 99.32% <ø> (+0.69%) ⬆️
src/release/index.ts 100.00% <ø> (ø)
src/release/publisher.ts 94.87% <ø> (-4.36%) ⬇️
src/release/release-trigger.ts 100.00% <ø> (ø)
src/release/release.ts 92.29% <ø> (-5.56%) ⬇️
src/release/tag-version.ts 93.10% <ø> (+6.43%) ⬆️
src/release/update-changelog.ts 100.00% <ø> (ø)
src/sample-file.ts 81.54% <ø> (-12.07%) ⬇️
... and 270 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aaa602...e7235ef. Read the comment docs.

@Chriscbr
Copy link
Contributor Author

Added the pr/do-not-merge label since I'm not sure if there's actually a pressing need to merge this in immediately. But Node v12 is deprecated at the end of this month and we have no interest in supporting that version after it has been unsupported, so enforcing it in new versions with the engine requirement seems like the right thing to do for the project's health. 👍

Chriscbr
Chriscbr commented Apr 14, 2022
View reviewed changes
.projenrc.js
Comment on lines +62 to +63
minNodeVersion: "14.0.0",
workflowNodeVersion: "14.17.0", // required by @typescript-eslint/eslint-plugin@5.19.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

projen needs a higher version of node on its GitHub actions runner in order to install all of projen's dev dependencies successfully and run full builds. But consumers of projen don't actually need those since we bundle all of our runtime dependencies. (For example, it's safe to run npx projen new python when the installed version of node is v14.0.0). Hence why these version numbers are different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Node < 14.11.0 is vulnerable to CVEs. 14.x is in maintenance. Its 2yrs old this week. Should we really be supporting that? This feels like overkill.

https://www.cvedetails.com/vulnerability-list/vendor_id-12113/Nodejs.html

Copy link
Contributor

Choose a reason for hiding this comment

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

the workflow separation, not the 12->14,

kaizencc
kaizencc approved these changes Apr 18, 2022
View reviewed changes
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

I don't have a problem with this. I agree that the workflow separation seems odd but I think the point is that projen should support whatever min node version it can.

@Chriscbr
Copy link
Contributor Author

Chriscbr commented Apr 19, 2022 •
edited
Loading

Before we merge this I think I need to undo some changes made in #1709 that removed the default "upgrade-projen" workflow. I'm realizing that I think having separate upgrade workflows/tasks for upgradingn projen and for upgrading other dependencies might actually be the happier path, and removing that was probably a mistake. projen does have breaking changes every now and then, and so including it with all other dependencies is a bit too chaotic / unreliable for most projects. In particular, this change may not get automatically picked up with the upgrade.yml workflow because installing the new version of projen on a workflow using node v12 will just fail (and not even attempt to create a PR).

Skipping this. Let's just make the bump.

@Chriscbr Chriscbr mentioned this pull request Apr 25, 2022
Merged
mergify bot pushed a commit that referenced this pull request Apr 25, 2022
@Chriscbr
chore: deprecate node v12 support (#1795)
6db1b04 
Node v12 reaches end of life on April 30, 2022. This PR adds a warning that projen will be dropping support for old node releases.

We'll give this message a week to incubate before actually bumping the version we support (#1768).

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@Chriscbr Chriscbr removed the pr/do-not-merge ⚙️ used by automation label May 2, 2022
@mergify mergify bot merged commit 6da24c1 into main May 2, 2022
@mergify mergify bot deleted the rybickic/upgrade-node14 branch May 2, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgollucci pgollucci pgollucci left review comments

@kaizencc kaizencc kaizencc approved these changes

Assignees
No one assigned
Labels
contribution/core ⚙️ used by automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update projen to require node v14
4 participants
@Chriscbr @codecov-commenter @pgollucci @kaizencc