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

Add support for node 14 and 16, mark older versions legacy #66

Merged
merged 14 commits into from
Nov 23, 2021

Conversation

badrange
Copy link
Contributor

@badrange badrange commented Nov 22, 2021

Here is the corresponding documentation PR:
lando/lando#3219

@badrange badrange changed the title Add support for node 14 and 16, remove some stone age node configs Add support for node 14 and 16, mark older versions legacy Nov 22, 2021
@reynoldsalec
Copy link
Sponsor Member

reynoldsalec commented Nov 22, 2021

Thanks @badrange for the contribution! Two things to round out this work:

  • add an example for node16 (copying over the node14 example and replacing the relevant versions should work).
  • make sure the "patch version" service in the node14 example grabs a legitimate docker hub image (I get the error ERROR: manifest for node:14.18.3 not found: manifest unknown: manifest unknown when trying to pull node:14.18.3.)

@pirog, how should we handle legacy node version support going forward? Should we keep the node10/12 examples, maybe combine them into a node-legacy example?

- yarn
command: /app/node_modules/.bin/nodemon src/app-custom.js --watch src --ignore *.test.js
patch:
type: node:14.18.3
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I don't think this actually exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, you're quite right about that, should be fixed now.

@badrange
Copy link
Contributor Author

Thank you for the quick response @reynoldsalec - should be fixed now. I also took the liberty of upgrading the dependencies in the new package.jsons.

@reynoldsalec
Copy link
Sponsor Member

Thanks @badrange, I can spin up node14 now.

When I try to test node16 I'm getting a message ERROR ==> node version 16.13.0 is not supported; you'll need to add 16.13 to the supportedVersions array in the node/builder.js.

You'll also want to all your tests to the .github/workflows/pr-node-tests.yml file under the leia-tests section; you use the title of the README file there.

@badrange
Copy link
Contributor Author

Thanks for the feedback @reynoldsalec - I now managed to get the tests to pass.

@@ -15,7 +15,8 @@ jobs:
node-version:
- '14'
leia-tests:
- node-10-example
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Spoke with Pirog and he wants us to keep all the legacy version options available/tested. Let's keep the node-10-example tests @badrange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, makes sense! I pushed an update, let's hope the CI/CD pipeline works.

@reynoldsalec reynoldsalec merged commit 1ea1320 into lando:main Nov 23, 2021
@reynoldsalec
Copy link
Sponsor Member

You're a champ @badrange, I'm going to merge this in.

Would you have interest in maintaining the node recipe once we split it out of Lando core? Might be up your alley...

@badrange badrange deleted the feature/support-node-14-16 branch November 23, 2021 22:25
@badrange
Copy link
Contributor Author

Happy times!

Let's get back with maintainership once that happens, depends on the work situation you know! :-)

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.

2 participants