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

ci: node pipelines now setup node version depending on lockfileVersion #230

Merged
merged 3 commits into from
Apr 20, 2023

Conversation

derberg
Copy link
Member

@derberg derberg commented Apr 18, 2023

This is related to very annoying thing about current pipelines, that they are set to Node 14 and in many projects package-lock v2 and even v3 is used, for newer versions of Node. As a result, many times happens that installation of project fail because of inconsistent support between old npm and new package lock structure.

more in #187

so what is this PR introducing:

  • composite action that can be reused across all repos and we can use it to verify version of package lock, and output proper node version that should be used
  • modify pipelines that require Node setup to setup version of the node that given package-lock.json requires

This PR was tested in:

@derberg
Copy link
Member Author

derberg commented Apr 18, 2023

@KhudaDad414 wdyt?

@smoya fyi

@jonaslagoni fyi, this is finally no going to be annoying anymore

KhudaDad414
KhudaDad414 previously approved these changes Apr 19, 2023
Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

LGTM!

smoya
smoya previously approved these changes Apr 19, 2023
Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

Thanks @derberg for moving this forward! You rock!

Couple of nits and questions. Otherwise, LGTM! 🚀 🌔

case 3:
nodeVersion = '18'
break;
default:
Copy link
Member

Choose a reason for hiding this comment

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

In which cases the default will apply?
Do you know if there is a case when there is no lockfileVersion in the package-lock.json?
If there is not, it will mean the version is >3, so I would say then the default should be the greatest version of node?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is hard to predict lockfile version evolution and what is latest node version. So in case 4 we set 16 and if it works, all fine, if it fails, it is actually good, we learn that there is a new version and we can fix it super fast here in the composite action

this default is only for projects that do not have package-lock, which is possible, by mistake, but possible

@@ -50,11 +50,15 @@ jobs:
id: packagejson
run: test -e ./package.json && echo "exists=true" >> $GITHUB_OUTPUT || echo "exists=false" >> $GITHUB_OUTPUT
shell: bash
- if: steps.packagejson.outputs.exists == 'true'
name: Check package-lock version
uses: asyncapi/.github/.github/actions/get-node-version-from-package-lock@master
Copy link
Member

Choose a reason for hiding this comment

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

Question: Is this being cached? It will fetch latest action when updated in master branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, taking latest from master, which is good, there are no issues with it, as it is in .github repo, so if we anyway change something to this composite action and it will be breaking, we will anyway update dependent workflows.

if there is a workflow that is not managed it .github repo and would like to use this composite action, this is fine, but then they need to use commitId and not branch name

@derberg derberg dismissed stale reviews from smoya and KhudaDad414 via 295fe05 April 19, 2023 08:59
@derberg derberg requested a review from smoya April 19, 2023 09:55
@derberg derberg requested a review from KhudaDad414 April 19, 2023 10:43
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Awesome 👍

@derberg
Copy link
Member Author

derberg commented Apr 20, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit a161727 into asyncapi:master Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants