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: use node 21 for github actions #5842

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

chore: use node 21 for github actions #5842

wants to merge 14 commits into from

Conversation

riqwan
Copy link
Contributor

@riqwan riqwan commented Dec 11, 2023

I've personally had great boost in local dev time and memory consumption moving from 16 to 21.

Let me know if there might be issues with release actions with the bump to 21

Copy link

vercel bot commented Dec 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Jan 4, 2024 3:25pm
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jan 4, 2024 3:25pm
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jan 4, 2024 3:25pm

Copy link

changeset-bot bot commented Dec 11, 2023

⚠️ No Changeset found

Latest commit: 03ef3f6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@riqwan riqwan changed the title chore: test gh actions chore: use node 20 for github actions Dec 11, 2023
@riqwan riqwan marked this pull request as ready for review December 11, 2023 15:34
@riqwan riqwan requested a review from a team as a code owner December 11, 2023 15:34
@riqwan riqwan changed the title chore: use node 20 for github actions chore: use node 21 for github actions Dec 11, 2023
@riqwan
Copy link
Contributor Author

riqwan commented Dec 12, 2023

@olivermrbl @adrien2p works with some minor changes!

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

pretty awesome!

only thing I can think of is whether the github actions should be run with the minimum node version we support to ensure we are not "accidentally" approving changes that are only compatible with the latest version of node

that said, if this reduces CI time, I think we can solve this concern elsewhere, e.g. with a release pipeline

@adrien2p
Copy link
Member

In the ci, we can also specify multiple versions of node and the ci will run for each of them, but then it will be longer but ensure it works for all the versions tested

@olivermrbl
Copy link
Contributor

olivermrbl commented Jan 3, 2024

We are currently experiencing an issue related to an old node version in our CI, so I think we should try to make this land on develop soon.

My suggestion for the above "issue" would be to include a separate workflow, we can dispatch every time we release with our minimum required node version as input. It can be the same workflow with defaulting to v21 unless specified otherwise.

I am not comfortable running our actions with one version, while officially supporting a different one. We need to ensure the minimum required version is stable.

What do you think?

@adrien2p
Copy link
Member

adrien2p commented Jan 3, 2024

@olivermrbl why dont we provide an array of versions with min and 21 in the current workflow?

@olivermrbl
Copy link
Contributor

olivermrbl commented Jan 3, 2024

why dont we provide an array of versions with min and 21 in the current workflow?

Wouldn't that make the pipeline take significantly longer time to complete?

Update: I am keen to give this a try, but if it extends the CI time too much, I'd rather go with a different approach.

@adrien2p
Copy link
Member

adrien2p commented Jan 3, 2024

why dont we provide an array of versions with min and 21 in the current workflow?

Wouldn't that make the pipeline take significantly longer time to complete?

They should run in parallel, at least from my experience with the extender, all versions ran at the same time.

We just need to provide the matrix

@riqwan
Copy link
Contributor Author

riqwan commented Jan 3, 2024

If we are experiencing an issue with our current version, would the other workflow even run successfully?

@olivermrbl
Copy link
Contributor

They should run in parallel, at least from my experience with the extender, all versions ran at the same time

Nice, I didn't know that. But I guess that makes the whole idea of the PR redundant?

@adrien2p
Copy link
Member

adrien2p commented Jan 3, 2024

The only left question is the one from riqwan, the 16.14 will likely create memory issues (jest + v8 update) and 16.10 is currently creating issues with some packages.

@adrien2p
Copy link
Member

adrien2p commented Jan 3, 2024

Yes this pr would slight change to use strategy: matrix: node: []

@olivermrbl
Copy link
Contributor

If we are experiencing an issue with our current version, would the other workflow even run successfully?

No, you're right – this would have to be fixed. From what I can tell, updating the Node version used in one of our workflows resolves the issue, we are facing. It seems to be a dependency requiring a minimum Node version of 16.14 and we default to 16.10.

Did you experience issues with 16.14 specifically @riqwan or would this be an OK change to resolve it here and now?

@adrien2p
Copy link
Member

adrien2p commented Jan 3, 2024

There is the jest memory issues above 16.10 and under 20 something. Because of a v8 update that occurs at that time

@olivermrbl
Copy link
Contributor

There is the jest memory issues above 16.10 and under 20 something

But since this workflow is not testing anything with jest, it should be fine no? We are just cloning our starter to ensure our CLI works as expected.

@adrien2p
Copy link
Member

adrien2p commented Jan 3, 2024

The integration tests are likely to fail often

Override: you mean for the specific cli workflow. Should be fine 👌

@olivermrbl
Copy link
Contributor

Override: you mean for the specific cli workflow. Should be fine 👌

Yes exactly. I am only using 16.14 for this specific workflow. None of the integration test flows should be affected.

@riqwan
Copy link
Contributor Author

riqwan commented Jan 3, 2024

No issues with 16.14, this would be fine. I was more interested in the integration tests for the performance gains.

@olivermrbl
Copy link
Contributor

I was more interested in the integration tests for the performance gains.

Do you have an idea of how much it cuts off? If it's significant, I think it's worth trying to find a way to make it land.

@riqwan
Copy link
Contributor Author

riqwan commented Jan 4, 2024

@olivermrbl I could only run it without cache to test, and it was only about a minute faster.

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.

None yet

3 participants