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

Update workflows and action to NodeJS 20 #166

Closed
wants to merge 3 commits into from
Closed

Conversation

deejay1
Copy link
Contributor

@deejay1 deejay1 commented Jan 26, 2024

Build with recent @actions/core and Node 20

Fixes #160

Copy link

@Kichura Kichura left a comment

Choose a reason for hiding this comment

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

LGTM although one thing still confuses me.

- name: Set Node.js 16.x
uses: actions/setup-node@v3.7.0
- name: Set Node.js 20.x
uses: actions/setup-node@v4.0.1
Copy link

Choose a reason for hiding this comment

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

Shouldn't v4 be used instead?

uses: actions/setup-node@v3.7.0
- uses: actions/checkout@v4
- name: Set Node.js 20.x
uses: actions/setup-node@v4.0.1
Copy link

Choose a reason for hiding this comment

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

Same question applies from check-dist review.

Copy link

Choose a reason for hiding this comment

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

Copy link

@Kichura Kichura Jan 27, 2024

Choose a reason for hiding this comment

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

not what i meant, meant that you can use v4 to have it auto-choose the latest version instead of having to relay on a specific version tag - essentially a wildcard of some sort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could as well be v4, but as the current workflow has the version pinned, I decided to keep the convention. Probably should have to look into the file history a bit, for decisions why it was pinned.

Copy link

Choose a reason for hiding this comment

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

Checked the file history and it seems to have been created with the pinned version already in place, although for me - i don't see the reasoning why this should stay pinned unless it was for an urgent reason which i couldn't find.

@bigdaz
Copy link
Member

bigdaz commented Jan 28, 2024

@JLLeitschuh How do you feel about bumping the required Node version without a major version bump of the action? (In general this is considered a breaking change).

@JLLeitschuh
Copy link
Contributor

(In general this is considered a breaking change).

I find it very unlikely that others are binding to this action via the API directly. So in that case, is that truly an "API" breaking change?

I don't think it is. If we were changing the API of the action.yml then I'd say that we'd need to do a major version bump. But if all downstream users can update without needing to modify their application of this plugin, I'm not that concerned.

@JLLeitschuh
Copy link
Contributor

Also, @bigdaz, I'm not a maintainer on any https://github.com/gradle projects anymore, so ultimately, it's your and @eskatos's call

I'm not against being made a maintainer on this project once again, but, as it currently stands, this project is completely outside my control

@bigdaz
Copy link
Member

bigdaz commented Jan 29, 2024

I find it very unlikely that others are binding to this action via the API directly. So in that case, is that truly an "API" breaking change?

It's a breaking change in that the action will no longer run on older runners that don't have a recent version of node installed. This would only be a problem for self-hosted runners (and I guess old versions of GitHub Enterprise).

@JLLeitschuh
Copy link
Contributor

This would only be a problem for self-hosted runners (and I guess old versions of GitHub Enterprise).

🤔 This is super helpful. Honestly, I could go either way. I think that since, for the predominant use case, on github.com this will not be an API breaking change I'm not too concerned.

Does anyone else want to weigh in on how this question has been handled by other popular actions?

I'm also happy to reach out to GitHub folks internally and see if they have advice.

@bigdaz
Copy link
Member

bigdaz commented Jan 29, 2024

Does anyone else want to weigh in on how this question has been handled by other popular actions?

My observation is that all of the action implementations from https://github.com/actions have had a major version bump with the update to Node 20.
eg: https://github.com/actions/setup-java/releases/tag/v4.0.0

I guess we should do the same, even though the risk is low.

@JLLeitschuh
Copy link
Contributor

I can support that. It's already the suggestion of the Open Source Security Foundation to bind your actions versions to commit hashes, instead of open versions to prevent supply chain attacks. And I believe that's what Dependabot has started doing as well, so our end-users will likely get a PR to rev the version regardless of how we do this.

@bigdaz
Copy link
Member

bigdaz commented Jan 29, 2024

And I believe that's what Dependabot has started doing as well

I haven't seen this. My understanding is that Dependabot will keep the version scheme you are currently using. So if you bind to v3.5.1 it will bump to v4.0.1, but if you bind to v3 the bump will be to v4.

I'm not sure what Dependabot does with commit-hash versions: perhaps handling these correctly is something recently added to Dependabot.

@bigdaz
Copy link
Member

bigdaz commented Jan 29, 2024

This was subsumed by #170. Thanks for the contribution!

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.

Upgrading Node.js to 20
6 participants