Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(issue 285): Add provenance support #294
feat(issue 285): Add provenance support #294
Changes from 1 commit
a305d60
1d861f2
3fe46d6
acb725f
05c2316
34601b7
17f111d
8c6f9ee
f43f4ba
710e9b9
4660c39
ac51350
96f0e26
08c4323
cad103e
cfcf53d
11cc1d5
84d6f05
1577c5d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only main outstanding thing I need to understand and we need to decide if we want to do something about.
The action at the moment uses whatever version of Node is installed in the runner, which you already figured out should be good enough when using the built-in runners (ubuntu specifically, but let's assume this is true for the others too).
Yet there is a way for us to force a specific version of Node by using the setup-node action from within our action. This would allow us to make sure we install a recent node version, meaning that we would know provenance would work (thereby allowing us to remove much of the npm-version-specific logic you have written), yet I'm not sure if the action forcing a certain node version is a sensible decision for this action. We just never had this problem before, but maybe it's just about the right time we think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say no because it adds maintenance burden on our side (we'd need to choose when to update now and in future) and seems to go against least-surprise: right now, there's a very clear division of responsibility, where the user-side yml sets up the environment and runner, then our script runs inside their environment.
I'd thought about this, there's a few ways we could do it if we wanted to, the least obtrusive I could think of was adding something like
"engines": { "npm": ">=9.5.0" }
to package.json, which is better than specifying a node version because it allows users to be more up to date (e.g. they can jump to Node 20 before we do), and it doesn't impose on node version when all we have any reason to care about is NPM version.But even that felt a bit heavy-handed for an opt-in feature. If a user wants an old NPM version (e.g. IIRC some projects stick to 8.5 to avoid side effects of changes to dependency resolution), and they don't care about provenance, there's really nothing wrong with that and no reason to stand in their way. Just tell them why it didn't work if they do try out provenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if there's a way to highlight certain lines using github code highlighting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems not, sadly: