feat: Add create-package-releases and upload-package-sboms- and verify-package#63
Conversation
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.
|
|
||
| <%= 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.
There was a problem hiding this comment.
The purpose of env-dump is to help with debugging failed releases: sometimes there's problems with release tooling (version mismatches, runtime issues, etc.) Granted, I didn't refine this for Node, but it has been useful when the build has broken a few other times.
If you feel like this is bespoke enough to warrant its own failure handling, I can live with that though.
| 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.
There was a problem hiding this comment.
yup agree! I made it so that we make the sbom path part of the artifacts manifest a) so that it's not duplicated b) so that we don't have js-isms in a generic thing
Also, I renamed publish-tarballs -> publish-npm-tarballs and create-package-releases -> create-npm-package-github-releases.
David Elner (delner)
left a comment
David Elner (delner)
left a comment
There was a problem hiding this comment.
Few more things that stood out, but it's moving in the right direction.
| @@ -0,0 +1,128 @@ | |||
| name: Create npm Package GitHub Releases | |||
There was a problem hiding this comment.
This doesn't look npm specific? If it isn't we should rename (drop npm). If it is, I think we should move it to release/lang/js.
There was a problem hiding this comment.
I renamed it!
| if [ -z "$sbom_asset" ]; then | ||
| echo "::error title=SBOM asset missing::manifest package $name@$version must set sbom_asset." | ||
| rm -f "$notes_file" | ||
| exit 1 |
There was a problem hiding this comment.
If I'm reading this correctly, this means we could fail mid-way through releasing multiple packages? What's the recovery path when that happens?
I think we could mitigate this somewhat at the "validate" stage (pre-approval) but at a minimum we should have a means of healing, ideally self-healing.
(This was one of the advantages a "job per package" or "package per workflow" scheme offers as you can isolate/restart individual jobs. Here I'm not so sure how that works.)
There was a problem hiding this comment.
It's not that bad I think.
We have two failure modes:
- Something goes wrong at the "publish npm tarball" step
- Something goes wrong at the "create github releases" step
If we make both """idempotent""", meaning that re-running the action will not re-publish either a tarball, or a github release, I think we are perfectly fine right? I tried to implement this now.
There was a problem hiding this comment.
Yes, idempotency is the answer. I also applied this to other workflows in another branch.
| channel="$(jq -r '.channel // empty' <<< "$package_json")" | ||
| provenance="$(jq -r '.provenance // empty' <<< "$package_json")" | ||
|
|
||
| [ -n "$expected_tarball" ] || expected_tarball="$(artifact_base "$name" "$version").tgz" |
There was a problem hiding this comment.
Looks like this is also deriving (guessing) rather than using an explicit input? (Same as the SBOM comment before.)
There was a problem hiding this comment.
Made it so that publish-npm-tarballs wants a tarball_asset, requires it to be an exact match, and verifies exactly one downloaded tarball matches.
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