feat: Add create-package-releases and upload-package-sboms- and verify-package#63
feat: Add create-package-releases and upload-package-sboms- and verify-package#63Luca Forstner (lforst) wants to merge 7 commits into
create-package-releases and upload-package-sboms- and verify-package#63Conversation
David Elner (delner)
left a comment
David Elner (delner)
left a comment
There was a problem hiding this comment.
I think adding some of these actions are fine, but I recommend we make some structural changes to them before adopting them as a part of the "public API" of our action library.
| @@ -0,0 +1,26 @@ | |||
| name: Verify Release Package Attestations | |||
There was a problem hiding this comment.
It doesn't look like this is actually used in braintrustdata/braintrust-sdk-javascript#2216 : can we remove this action?
There was a problem hiding this comment.
Oh I must have lost it. We theoretically should, otherwise we cannot guarantee that the artifacts have been tampered with after uploading as artifacts right?
| node-version: 'lts/*' | ||
| registry-url: https://registry.npmjs.org | ||
|
|
||
| - name: Install npm with OIDC trusted publishing support |
There was a problem hiding this comment.
Can we extract this to steps/lang/js/npm/setup? Seems generic enough: don't want to have to manage this version multiple places, and it probably should be configurable.
There was a problem hiding this comment.
I would quite honestly not use an action as a primitive that sets up npm. That operation has already way too many primitives in the ecosystem. As it stands, I find this is beautifully self-contained.
| shell: bash | ||
| run: npm install -g "npm@11.6.2" | ||
|
|
||
| - name: Publish tarballs in manifest order |
There was a problem hiding this comment.
You could consider moving this to its own step if you think it's worth readability, but otherwise I think this can stay here.
There was a problem hiding this comment.
I think it's fine, don't wanna unnecessarily make things generic.
|
|
||
| <%= render_step('slack/send', text: "${{ steps.message-published.outputs.text }}") %> | ||
|
|
||
| - name: Build failure Slack message |
There was a problem hiding this comment.
Why not reuse steps/release/lang/js/on-failure for this?
There was a problem hiding this comment.
steps/release/lang/js/on-failure combines two things: lang/js/env-dump and release/notify-failure
env-dumpis a bit pointless in a monorepo, and a bit pointless in general I think. The information it prints is irrelevant to publishing.release/notify-failureis problematic for a few reasons:- In the flow I came up with, GH releases and tags are done after publishing the tarballs.
release/notify-failurewants a tag. release/notify-failurewants one tag while in the monorepo we have multiple tags.
- In the flow I came up with, GH releases and tags are done after publishing the tarballs.
| @param clobber [String] whether to overwrite existing assets | ||
| @param dry_run [String] whether to only print planned uploads | ||
| -%> | ||
| - name: Upload package SBOMs to GitHub releases |
There was a problem hiding this comment.
Do we use this in multiple actions or have immediate intent to? If not, I think we should fold this into the upload-package-sboms action as its not reused.
| @@ -0,0 +1,55 @@ | |||
| name: Upload Package SBOMs to GitHub Releases | |||
| description: > | |||
| Attach package SBOM assets to package GitHub releases from a release manifest. This | |||
There was a problem hiding this comment.
Why not fold this into create-package-releases? Is SBOM supposed to be severable? (I'd think not.)
There was a problem hiding this comment.
Honestly dunno. I haven't played with SBOM too much but if there is a case where SBOM creation is not possible we wouldn't be stuck. But I can do it for now 👍
| artifact_base="${name#@}" | ||
| artifact_base="${artifact_base//\//-}" | ||
| artifact_base="${artifact_base//@/-}" | ||
| expected="${sbom_asset:-$artifact_base-$version.sbom.json}" |
There was a problem hiding this comment.
I think it'd be better if we explicitly required an SBOM asset here instead of guess at a derivation. The convention is hidden and looks npm-ish which doesn't fit well with making this a language agnostic behavior.
2 participants
Adds smaller units of work that are usable in the js repo.
Flow for the JS repo would be:
.tgzfiles alongside SBOMs and uploads those as artifacts..tgzfiles to npm, pushes package tags based on changesets, creates a github release per package tag, attaches the SBOMs to the github releases, comments on issues closed by the releasesThe JS SDK side of things is tracked in this PR: braintrustdata/braintrust-sdk-javascript#2216