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

feat: install Node v16 when lockfileVersion is 2 #187

Closed
wants to merge 6 commits into from

Conversation

smoya
Copy link
Member

@smoya smoya commented Oct 18, 2022

Description
Partially fixes #123

Note: Not sure if this can be extracted to another action and reused?

Related issue(s)

@derberg
Copy link
Member

derberg commented Oct 19, 2022

I think this is actually the smartest solution we could do here 🍺 . So setting up Node environment based on lockfileVersion and maybe other prerequisites too.

I'm only not sure if the way you do it is proper:

  • there is a lot of repetition of the same command and yeah, we know what repetition ends up in future
  • current script assumes there are just 2 lockfile versions. Thing is that since npm got acquired by GitHub, I have a feeling that development got faster, and they will evolve to more versions I think. In official docs you even have mention of lockfile v3 -> https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json

sounds like best would be to have a custom GitHub action responsible for detecting what package-lock version is used and exposing it to output. It wouldn't have to be published to marketplace, could be placed inside this repo (example from generator -> https://github.com/asyncapi/generator/tree/master/.github/templates-list-validator and https://github.com/asyncapi/generator/blob/master/.github/workflows/templates-list-validation.yml#L14)

thoughts?

of course there is one downside, that we for example say that generator works with node > 12 but we will test on 16 only 😞 but these issues with lockfileVersion compatibility are out of our hands

we are not the only ones suffering https://twitter.com/search?q=lockfileVersion&src=typed_query&f=live

@smoya
Copy link
Member Author

smoya commented Mar 1, 2023

@derberg should we move forward with this?

@derberg
Copy link
Member

derberg commented Mar 27, 2023

@smoya did you see my comment?

other stuff:

  • package lock v3 is here 😄 so we definitely need to add it to this PR too 😄 v3 and node 18
  • please use actions/setup-node@v3 instead of actions/setup-node@v2
  • I don't think we can use "::set-output name=lockfileVersion::1" as according to https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/ it will stop working soon. I recommend to use actions/github-script@v4 action instead of bash. Thanks to it, you can use core library that is intend driven and their setOutput is future-proof. Also advantage of script is that you can put script in one file and then reuse in 3 workflows, so you kinda address my concern from previous comment

@smoya
Copy link
Member Author

smoya commented Mar 28, 2023

@smoya did you see my comment?

Nope 😆 For some reason, I did not. Thanks for taking the time to collect and summarize all the info.
I like all your suggestions, except I should write javascript instead of a simple shell script. It's just a grep! We can still redirect an output to the right env vars: echo "{name}={value}" >> $GITHUB_OUTPUT.
I like the idea of having all of that into just one single script and reuse it. In fact, I think the script will just return the node version we should use, something like a recommended-node-version-by-lockfile.sh or similar.

Copy link
Member

derberg commented Mar 28, 2023

yeah, for you grep for me magic 😃

I do not mind .sh as long as we just take this reusability direction. Global workflow can also push out .sh files to other repos

@derberg
Copy link
Member

derberg commented Apr 4, 2023

when you will work on this, refactor I mean, you might want to try apache/airflow#27371

more on composite actions -> https://asyncapi.slack.com/archives/C34AUKWQK/p1680613343254339

@derberg
Copy link
Member

derberg commented Apr 18, 2023

closing in favour of #230

@derberg derberg closed this Apr 18, 2023
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.

Update Node and npm to a more recent version
4 participants