feat(issue 285): Add provenance support #294

AlanSl · 2023-05-02T11:59:01Z

closes #285

Adds provenance support, with:

Guards against silent failure or known misleading errors from NPM
Basic usage documentation
Unit tests and basic integration-like test cases in "release" tests

simoneb

some comments, but overall LGTM, very nice job

simoneb · 2023-05-02T14:00:32Z

README.md

+ - In the `.yml` file that configures your release action:
+   - Add `provenance: true` to your list of **inputs**
+   - Add `id-token: write` to your list of **permissions**
+ - Ensure your CI runner uses NPM >= 9.5.0 (should be the default if Node >= 18)


how do we ensure this? is it something that the user needs to do or, since this is an action, we can do that ourselves and therefore not delegate this responsibility to the user?

The current approach appears to have been that we do delegate this responsibility to the user, but we provide them with a working example config to follow (a .github/workflows/release.yml containing runs-on: ubuntu-latest). The readme said:

It is important to be aware that you are responsible for:

The context where the command is executed.
The command will be executed using the current GitHub runner configuration (e.g. the one set on runs-on).
You can customize it by...

But we can probably drop or amend this line since our recommended image already provides NPM 9.5.1, so they'll already get a valid version unless they deviated from our guidance. If they did for some reason, we give an informative error message, then beyond that, it's on them.

I've reworked this section in the "Rework readme advice" commit

README.md

simoneb · 2023-05-02T14:01:59Z

README.md

+   - Add `provenance: true` to your list of **inputs**
+   - Add `id-token: write` to your list of **permissions**
+ - Ensure your CI runner uses NPM >= 9.5.0 (should be the default if Node >= 18)
+ - Ensure your `package.json` is complete and correct. NPM will cancel the release with specific errors if it finds a problem. Requirements may change in future NPM releases but include things like a `"repository"` field with `"url"` property matching the format `"git+https://github.com/user/repo"`.


is there documentation about this? I mean saying that the package file needs to be "complete and correct" isn't quite enough if provenance imposes additional constraints. either list the constraints or, even better, link to where these constraints are explained in the official docs

Sadly they're not documented (nor is it straightforward to see requirements in the source).

I've reworked this section in the "Rework readme advice" commit.

simoneb · 2023-05-02T14:02:39Z

README.md

+ - Ensure your CI runner uses NPM >= 9.5.0 (should be the default if Node >= 18)
+ - Ensure your `package.json` is complete and correct. NPM will cancel the release with specific errors if it finds a problem. Requirements may change in future NPM releases but include things like a `"repository"` field with `"url"` property matching the format `"git+https://github.com/user/repo"`.
+
+If `provenance: true` is in your YML inputs and any condition isn't met, the release will cancel with an error. 


let's be more specific. when will this fail exactly? I assume when you're trying to publish the package right?

👍 I've reworked this section in the "Rework readme advice" commit

simoneb · 2023-05-02T14:03:19Z

README.md

+
+If `provenance: true` is in your YML inputs and any condition isn't met, the release will cancel with an error. 
+
+If you run a release without `provenance: true` when a previous release had provenance, the provenance banner on your package's NPM landing page will be removed but the banner will remain on the subpage for the previous release.


this is certainly good to know, but is it really something that we need to document or shall we just redirect the user to the official docs? I mean we are not the owners of this information, and it may change

👍 I've reworked this section in the "Rework readme advice" commit

simoneb · 2023-05-02T14:27:06Z

src/utils/provenance.js

+ * Split out as its own export so it can be easily mocked in tests.
+ */
+async function getNpmVersion() {
+  return await execWithOutput('npm', ['-v'])


nit: not necessary to await the last async function invoked by an async function, it can simply be returned

src/utils/provenance.js

simoneb · 2023-05-02T14:29:53Z

src/utils/execWithOutput.js

-  const options = {
-    silent: false,
-  }
+  options.silent = false


minor: can we implement this differently so if for whatever reason the caller specifies that silent should be true, we don't override it?

simoneb · 2023-05-02T14:31:10Z

test/release.test.js

@@ -200,6 +216,73 @@ tap.test('Should publish to npm without optic', async () => {
  })
 })

+tap.only(


remove .only, here and in the other tests

src/utils/provenance.js

README.md

simoneb

I'm approving with an outstanding question, see inline comments.

simoneb · 2023-05-03T11:31:49Z

README.md

+          semver: ${{ github.event.inputs.semver }}
+          npm-tag: ${{ github.event.inputs.tag }}
+          # add this to activate the action's provenance feature
+          provenance: true


wondering if there's a way to highlight certain lines using github code highlighting

Seems not, sadly:

There's an open feature request for it

There's a hacky workaround but it clashes with syntax highlighting and strips styling, only supports hard-to-see bolding of text in non-syntax-highlighted code blocks.

simoneb · 2023-05-03T11:38:52Z

README.md

+NPM has some internal [requirements](https://docs.npmjs.com/generating-provenance-statements#prerequisites) for generating provenance. Unfortunately as of May 2023, not all are documented by NPM; some key requirements are:
+
+- `id-token: write` must be added to your `release.yml`'s **permissions**
+- NPM must on version 9.5.0 or greater (this will be met if our recommended `runs-on: ubuntu-latest` is used)


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.

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.

AlanSl · 2023-05-03T13:55:59Z

@simoneb I've responded to the questions and pushed a couple of typo fixes I saw. If you're happy for it to be merged, looks like you need to do the final squash-merge:

This pull request can be automatically merged by project collaborators

Only those with write access to this repository can merge pull requests.

simoneb · 2023-05-03T13:57:31Z

Yep thanks I agree with your conclusions, thanks!

AlanSl added 10 commits April 28, 2023 11:15

Add provenance option and flag

a305d60

Set provenance to false by default

1d861f2

Add fail-fast provenance checks

3fe46d6

Use existing pattern for boolean strings

acb725f

Fix leftover from another branch

05c2316

Add unit tests for provenance utils

34601b7

Add test to publish util unit tests

17f111d

Add provenance integration tests

8c6f9ee

Refactor so tests can mock npm -v

f43f4ba

Add readme docs

710e9b9

AlanSl changed the title ~~feat(issue 286): Add provenance support~~ feat(issue 285): Add provenance support May 2, 2023

AlanSl added 2 commits May 2, 2023 13:03

Fix minor typos in readme

4660c39

Two more minor typo fixes

ac51350

simoneb reviewed May 2, 2023

View reviewed changes

AlanSl added 5 commits May 2, 2023 17:04

Allow silent option to be not false

96f0e26

tap.only -> tap.test

08c4323

Drop redundant await

cad103e

Link to NPM docs not blog

cfcf53d

Rework readme advice

11cc1d5

AlanSl requested a review from simoneb May 2, 2023 17:45

Fix section link

84d6f05

AlanSl commented May 2, 2023

View reviewed changes

README.md Show resolved Hide resolved

simoneb approved these changes May 3, 2023

View reviewed changes

Fix small typo in docs

1577c5d

AlanSl requested a review from simoneb May 3, 2023 13:56

simoneb approved these changes May 3, 2023

View reviewed changes

simoneb merged commit 69f72a7 into nearform-actions:main May 3, 2023

optic-release-automation bot mentioned this pull request May 4, 2023

[OPTIC-RELEASE-AUTOMATION] release/v4.5.0 #297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(issue 285): Add provenance support #294

feat(issue 285): Add provenance support #294

AlanSl commented May 2, 2023 •

edited

Loading

simoneb left a comment

simoneb May 2, 2023

AlanSl May 2, 2023

AlanSl May 2, 2023

simoneb May 2, 2023

AlanSl May 2, 2023

simoneb May 2, 2023

AlanSl May 2, 2023

simoneb May 2, 2023

AlanSl May 2, 2023

simoneb May 2, 2023

AlanSl May 2, 2023

simoneb May 2, 2023

AlanSl May 2, 2023

simoneb May 2, 2023

AlanSl May 2, 2023

simoneb left a comment

simoneb May 3, 2023

AlanSl May 3, 2023 •

edited

Loading

simoneb May 3, 2023

AlanSl May 3, 2023 •

edited

Loading

AlanSl commented May 3, 2023 •

edited

Loading

This pull request can be automatically merged by project collaborators

simoneb commented May 3, 2023


		If `provenance: true` is in your YML inputs and any condition isn't met, the release will cancel with an error.

		If you run a release without `provenance: true` when a previous release had provenance, the provenance banner on your package's NPM landing page will be removed but the banner will remain on the subpage for the previous release.

feat(issue 285): Add provenance support #294

feat(issue 285): Add provenance support #294

Conversation

AlanSl commented May 2, 2023 • edited Loading

simoneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simoneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanSl May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanSl May 3, 2023 • edited Loading

Choose a reason for hiding this comment

AlanSl commented May 3, 2023 • edited Loading

This pull request can be automatically merged by project collaborators

simoneb commented May 3, 2023

AlanSl commented May 2, 2023 •

edited

Loading

AlanSl May 3, 2023 •

edited

Loading

AlanSl May 3, 2023 •

edited

Loading

AlanSl commented May 3, 2023 •

edited

Loading