feat: add `access`, fix provenance if new & unscoped #305

AlanSl · 2023-05-23T12:34:14Z

closes #301

This follows on from #302 (I started a new branch and PR based on that one as we're both on personal forks) and does two related things:

Allows users to manually set the access control of their package in the input options, like access: public or access: restricted
Fixes Can't release a newly created package with provenance #301 and allows never-published unscoped packages to be published with provenance in their first release, by inserting --access: public if and only if:
- the user hasn't specified access control in any way, and
- the package is unscoped (therefore, inherently public and cannot be private / restricted), and
- the package hasn't been published before (so NPM's provenance feature would fail to see it as public unless we do this)

This would be a minor version bump on the action: the access control is a new feature, there's no breaking change or requirement to use the new feature because the provenance fix only has an affect when the package would have been public anyway.

Here's an example of an unscoped package published for the first time with provenance using this, without explicitly (redundantly) specifying the package to be public: https://www.npmjs.com/package/alansl-new-release-tester-2/v/0.1.0

@simoneb looks like I can't add you via "assign reviews" so I'll ping you here

I'm sure it used to do this via Husky?

src/utils/packageInfo.js

AlanSl · 2023-05-23T13:00:36Z

test/publishToNpm.test.js

@@ -90,15 +100,18 @@ tap.test(
      'pack',
      '--dry-run',
    ])
+    t.pass('npm pack called')


I added a lot of these as without at least one test, Tap's output doesn't say which test in a file failed

mhh interesting, I wasn't aware of that actually and I'm somewhat surprised, but ok tap does have some weird behaviors and I never quite found the output very readable, so I'm good with this change

simoneb

A few minor code changes if you don't mind, approving to avoid blocking. Overall LGTM, thanks for all the work and testing

src/release.js

src/utils/provenance.js

AlanSl · 2023-05-24T08:10:50Z

Comments all make sense, I've simplified the options overrides and moved that fixed array to const.js with other constants

simoneb

A few more comments. Would you mind testing the weird tap behavior in isolation to make sure we're not looking at a red herring? I would be surprised if an exception being thrown in a test (and not caused by a failing tap assertion) resulted in tap not printing the test where it occurred.

simoneb · 2023-05-24T08:37:09Z

src/release.js

@@ -100,22 +100,41 @@ module.exports = async function ({ github, context, inputs }) {
    const opticToken = inputs['optic-token']
    const npmToken = inputs['npm-token']
    const provenance = /true/i.test(inputs['provenance'])
+    const access = inputs['access']
+
+    // Can't limit custom action inputs to fixed options like "choice" type in a manual action


I think this comment is somewhat redundant as it's fairly obvious that you can't restrict what inputs are provided to an action

simoneb · 2023-05-24T08:40:45Z

src/utils/provenance.js

+  const optionAdjustments = await getAccessAdjustment(publishOptions)
+
+  return optionAdjustments ?? {}


can we simplify this logic (no need to return an empty object) and rename the function to reflect better what it's doing (it's returning provenance-related publish options)

simoneb · 2023-05-24T08:41:44Z

src/utils/publishToNpm.js


+  const packageInfo = await getPublishedInfo()
+  const packageName = packageInfo?.name
  // Package has not been published before
  if (!packageName) {


nit: can we make this condition depend on packageInfo instead of packageInfo?.name? then, inside this conditional, we can confidently say that packageInfo.name exists

🤔 it's probably impossible in real life fornpm view --json to return an object that had no name property, but if it did and we did this, further down we'd exec npm --view undefined@x.y.x.

Can simplify it to if (!packageInfo?.name) return true(and ${packageInfo.name}@${version} below) I guess, that'd still be type safe for what the external service returns.

fair enough

simoneb · 2023-05-24T08:44:10Z

test/packageInfo.test.js

+    error: new Error('code E418 - unexpected teapot'),
+  })
+
+  t.rejects(mocks.getPublishedInfo, /teapot/)


when you make async assertions (such as .rejects or .resolves), you need to either return or (preferably, in my opinion) await the result of the assertion, otherwise the assertion will be a noop

Makes sense.

While mopping up my own instances of this, I've found a few pre-existing examples where I think tests aren't behaving as intended, e.g. there's several with a pattern like this in the execWithOutput tests from a few months ago:

t.resolves(execWithOutputModule.execWithOutput('ls', ['-al']), output) execStub.calledWith('ls', ['-al'])

I think these are intended to be like this:

await t.resolves(execWithOutputModule.execWithOutput('ls', ['-al']), output) t.ok(execStub.calledWith('ls', ['-al']))

...because as previously written, the execStub.calledWith will fire before the async exec actually happens, and just returns false into the void with no effect? I might fix these in a small separate PR.

There are also quite a lot of preexisting tests that end with unawaited unreturned t.rejects or t.resolves like the one above did.

yes please, any async tap assertions need to be awaited for them to have any effect

simoneb · 2023-05-24T08:45:04Z

test/provenance.test.js

  async t => {
-    t.throws(
-      () => checkProvenanceViability(),
+    t.rejects(


same as previous comment. you've turned a sync assertion into an async one, hence you have to await the promise it returns

simoneb · 2023-05-24T08:47:43Z

test/publishToNpm.test.js

@@ -90,15 +100,18 @@ tap.test(
      'pack',
      '--dry-run',
    ])
+    t.pass('npm pack called')


mhh interesting, I wasn't aware of that actually and I'm somewhat surprised, but ok tap does have some weird behaviors and I never quite found the output very readable, so I'm good with this change

test/publishToNpm.test.js

AlanSl · 2023-05-24T15:15:31Z

@simoneb I think I responded to all the comments, ready for re-review / merge (I don't have access to merge it myself)

Claudia Donea and others added 9 commits May 23, 2023 13:03

fix: add public inpuput to allow provenance

b00328d

fix: linting errors

a24ece2

fix: refactor funcion

c655dbc

feat: decouple access option from provenance

6b50e22

fix: support provenance when new & unscoped

2b4f8a7

chore: update readme

ec38403

chore: another readme fix

5b51287

chore: another readme fix

6a43ddf

fix: fix lint

ffca8f5

I'm sure it used to do this via Husky?

AlanSl commented May 23, 2023

View reviewed changes

fix: tests that pass locally but not in ci

b595384

simoneb approved these changes May 23, 2023

View reviewed changes

src/release.js Outdated Show resolved Hide resolved

src/release.js Outdated Show resolved Hide resolved

src/release.js Outdated Show resolved Hide resolved

src/utils/provenance.js Outdated Show resolved Hide resolved

AlanSl added 3 commits May 24, 2023 08:58

chore: apply overrides more cleanly

e529e23

chore: define access options out of fn

01f7b15

Simplify ensureProv...'s return type

f22d1a5

simoneb reviewed May 24, 2023

View reviewed changes

AlanSl added 5 commits May 24, 2023 10:49

chore: remove redundant t tests

77051e5

chore: don't leave async tests hanging

e3501e3

chore: remove comment

d3e5945

chore: further simplify prov options

4d5326d

chore: simpler packageName logic

69cdfa1

AlanSl requested a review from simoneb May 24, 2023 15:15

simoneb approved these changes May 24, 2023

View reviewed changes

simoneb merged commit 71aae61 into nearform-actions:main May 25, 2023

simoneb mentioned this pull request May 25, 2023

fix: add public inpuput to allow provenance #302

Closed

2 tasks

This was referenced May 25, 2023

chore: await async tests, assert exec calls #308

Merged

Await and check logic of async assertions in tests #309

Closed

github-actions bot mentioned this pull request Jun 2, 2023

Release pending! #313

Closed

optic-release-automation bot mentioned this pull request Jun 5, 2023

[OPTIC-RELEASE-AUTOMATION] release/v4.6.0 #314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `access`, fix provenance if new & unscoped #305

feat: add `access`, fix provenance if new & unscoped #305

AlanSl commented May 23, 2023 •

edited

Loading

AlanSl May 23, 2023

simoneb May 24, 2023

simoneb left a comment

AlanSl commented May 24, 2023

simoneb left a comment

simoneb May 24, 2023

simoneb May 24, 2023

simoneb May 24, 2023

AlanSl May 24, 2023

simoneb May 24, 2023

simoneb May 24, 2023

AlanSl May 24, 2023 •

edited

Loading

simoneb May 24, 2023

simoneb May 24, 2023

simoneb May 24, 2023

AlanSl commented May 24, 2023

		const optionAdjustments = await getAccessAdjustment(publishOptions)

		return optionAdjustments ?? {}

feat: add access, fix provenance if new & unscoped #305

feat: add access, fix provenance if new & unscoped #305

Conversation

AlanSl commented May 23, 2023 • edited Loading

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

AlanSl commented May 24, 2023

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

AlanSl May 24, 2023 • edited Loading

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

AlanSl commented May 24, 2023

feat: add `access`, fix provenance if new & unscoped #305

feat: add `access`, fix provenance if new & unscoped #305

AlanSl commented May 23, 2023 •

edited

Loading

AlanSl May 24, 2023 •

edited

Loading