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

Build: Publish to NPM with provenance #23917

Closed
wants to merge 2 commits into from
Closed

Conversation

gabibguti
Copy link

@gabibguti gabibguti commented Aug 22, 2023

Closes #23916

What I did

Adds a flag to publish to npm with provenance.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

  1. On the next release, run the regular release workflow publish.yml
  2. Check if the workflow passes
  3. Check if the packages pages on npm show the provenance badge
    (e.g. https://www.npmjs.com/package/next#provenance)

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
@yannbf yannbf changed the title Publish to npm with provenance Build: Publish to NPM with provenance Aug 22, 2023
@yannbf yannbf added build Internal-facing build tooling & test updates ci:normal labels Aug 22, 2023
@yannbf
Copy link
Member

yannbf commented Aug 22, 2023

Muito obrigado @gabibguti ! 🙌
Is there anything we need to do on the org side in NPM?

@gabibguti
Copy link
Author

@yannbf Imagina!

Is there anything we need to do on the org side in NPM?

No. But, double checking the requirements, we need to make sure we are using npm 9.5.0+ when publishing, I'll check if we need to bump actions/setup-node version. And, as far as I understand, all packages being published by storybook are public, can you confirm that?

@gabibguti
Copy link
Author

Yeah, we need to change actions/setup-node version from 16 to 18. I'll bump, let me know if you see any blockers or problems here.

npm supports publishing with provenance starting at version 9.5.0+. When setting up node in the workflow, we need to use node 18.x+ to make sure we install npm 9.5.0+ to publish with provenance.

Signed-off-by: Gabriela Gutierrez <gabigutierrez@google.com>
@yannbf
Copy link
Member

yannbf commented Aug 22, 2023

And, as far as I understand, all packages being published by storybook are public, can you confirm that?

That's right, AFAIK!

Yeah, we need to change actions/setup-node version from 16 to 18. I'll bump, let me know if you see any blockers or problems here.

I believe we use node 16 in our actions because that's the lowest node version that we support, however given that your change only affects the publish workflow, I think it's totally fine. @JReinhold could you confirm?

@JReinhold
Copy link
Contributor

There's a subtle but important gotcha here that is blocking this from working. The command:

yarn workspaces foreach --parallel --no-private --verbose npm publish --tolerate-republish --tag someTag --provenance

yarn workspaces foreach will run a yarn command, not execute a CLI*. That means that this runs yarn npm publish in every package and not npm publish. This is crucial, because yarn npm publish is it's own separate CLI that doesn't support the --provenance flag (don't blame me for their confusing command-naming).

In theory we could instead use yarn exec and do this:

yarn workspaces foreach --parallel --no-private --verbose exec 'yarn pack && npm publish --tag someTag --provenance'

However that comes with it's own set of problems:

  1. npm publish doesn't support the --tolerate-republish flag, meaning that it will fail if you're attempting to republish an existing version.
"why would we even do this?" you ask

Well, the npm registry is super flaky, so when publishing 90 packages at the same time it's not uncommon for some of the packages to silently fail publishing. That's why we have retry logic in place that keeps publishing until all packages are successful. But that retry logic will be much more complex if we're no longer allowed to retry all packages, but have to selectively retry the failed packages.

  1. There's a bug with npm publish where it doesn't use the given tarball but instead creates its own. We worked around that issue for publishing to the local Verdaccio registry in Build: Fix publish script #23948, but it's certainly not a hack I love.

That's not to say we should not do this PR at all, but we need to solve the above two points as part of this if we want to.

@yannbf
Copy link
Member

yannbf commented Oct 3, 2023

So it seems like we are blocked from doing this change until this issue is resolved:

yarnpkg/berry#5430

@wojtekmaj
Copy link

Psst @yannbf! You can work around this issue by packing using Yarn, but publishing using npm:

wojtekmaj/react-async-button@5856920

@gabibguti
Copy link
Author

My understanding in this case is that we would really need yarn to support the provenance flag. Sorry about that folks, I did not see the tolerate republish problem. As @JReinhold suggested, we can do the yarn pack, then publish with npm, but we would need another logic to perform the retrying the publish for packages (--tolerate-republish), because the publish might fail since we are publishing multiple packages.

@JReinhold
Copy link
Contributor

@gabibguti's last comment is correct. I'm closing this for now, we can always re-visit if we want to do the extra work needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates ci:normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants