Conversation
If the published commit-ish was built and tested before, reuse that build rather than making a new build. This saves time, but more importantly it reduces the possibility of introducing new issues through a rebuild.
There was a problem hiding this comment.
Pull request overview
Updates the release publishing automation to publish the same build outputs that were produced (and intended to be) validated by CI, with a fallback build path when no suitable CI artifacts are available, plus several GitHub Actions cleanups.
Changes:
- Add a “find CI run + download artifacts” flow to
publish.yml, with a fallback to running the reusableci.ymlbuild when needed. - Extend
ci.ymlto support aforce-full-buildinput and refine package filtering/test skipping behavior; retain build artifacts longer. - Misc. workflow hygiene: concurrency controls and small composite-action cleanups.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/publish.yml | Looks up CI run artifacts for the release target and publishes using downloaded artifacts, with fallback build path. |
| .github/workflows/ci.yml | Adds force-full-build input, refines changed-package resolution, and uploads build artifacts with longer retention. |
| .github/workflows/ghpages-cleanup.yml | Adds concurrency to avoid overlapping cleanups. |
| .github/workflows/commitlint.yml | Cancels in-progress runs for the same PR/ref via concurrency. |
| .github/actions/test-package/action.yml | Simplifies Playwright install to a step-level conditional. |
| .github/actions/install-dependencies/action.yml | Minor cleanup (remove redundant working directory). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Test report for task-herder28 tests 28 ✅ 0s ⏱️ Results for commit 9c4f407. ♻️ This comment has been updated with latest results. |
Test report for scratch-svg-renderer 1 files 60 suites 0s ⏱️ Results for commit 9c4f407. ♻️ This comment has been updated with latest results. |
Test report for scratch-render 1 files 55 suites 3s ⏱️ Results for commit 9c4f407. ♻️ This comment has been updated with latest results. |
Test report for scratch-gui 2 files 62 suites 10m 54s ⏱️ Results for commit 9c4f407. ♻️ This comment has been updated with latest results. |
Test report for scratch-vm 1 files 886 suites 1m 57s ⏱️ Results for commit 9c4f407. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Updates the release publishing automation so npm publish reuses the exact build artifacts produced and tested by CI, reducing the chance of drift between “tested” and “published” outputs, along with several GitHub Actions cleanups/refinements.
Changes:
- Add
prepublishOnlyguards to prevent local/manual publishes outside CI. - Rework
publish.ymlto locate and download prior CI build artifacts for the release target (with a fallback full CI build when artifacts don’t exist). - Refine
ci.ymlpackage filtering/skip logic and tighten up workflow/action behavior (concurrency, conditional steps, artifact retention).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/task-herder/package.json | Adds prepublishOnly to block non-CI publishes. |
| packages/scratch-vm/package.json | Replaces prepublish with prepublishOnly to block non-CI publishes. |
| packages/scratch-svg-renderer/package.json | Adds prepublishOnly to block non-CI publishes. |
| packages/scratch-render/package.json | Replaces prepublish with prepublishOnly to block non-CI publishes. |
| packages/scratch-gui/package.json | Adds prepublishOnly to block non-CI publishes. |
| .github/workflows/publish.yml | Finds/uses tested CI build artifacts for publishing; adds fallback build; adds release comments. |
| .github/workflows/ghpages-cleanup.yml | Adds concurrency grouping to avoid overlapping cleanup runs. |
| .github/workflows/commitlint.yml | Cancels in-progress runs for the same PR/head to reduce CI load. |
| .github/workflows/ci.yml | Adds force-full-build input, refines package-change resolution, and adjusts when test/results run; sets artifact retention. |
| .github/actions/test-package/action.yml | Moves Playwright install gating to step-level if. |
| .github/actions/install-dependencies/action.yml | Minor cleanup (removes redundant working-directory). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
If there's some sort of complex emergency where publishing locally is necessary, use `--ignore-scripts` to skip the `prepublishOnly` protection.
cwillisf
force-pushed
the
publish-tested-builds
branch
from
993c36e to
9c4f407
Compare
KManolov3
left a comment
KManolov3
left a comment
There was a problem hiding this comment.
Looks good to me! Pretty happy that we'll be cutting the publish time 🙌
| echo "Not pushing to develop because the tag we're operating on is behind" | ||
| fi | ||
|
|
||
| - name: Comment on resolved issues and merged PRs |
|
|
||
| npm run build:dist --workspace @scratch/scratch-gui | ||
| npm publish --access=public --tag="${{steps.npm_tag.outputs.npm_tag}}" --workspace=@scratch/scratch-gui | ||
| npm publish --access=public --tag="$NPM_TAG" --ignore-scripts --workspace=@scratch/scratch-gui |
There was a problem hiding this comment.
I wondered why we were building here as well. I remember an issue from a while back, where GUI was using the changes from the old published version of svg-renderer/vm/.. instead of what's locally available in the build.
The second install-dependencies that you removed referred to that. I suppose that since the build is now done before updating the version, it shouldn't have the same issue - is that assumption correct?
There was a problem hiding this comment.
Honestly, I'm a bit nervous about things like that. I think it'll be OK, since all the package.json files are updated by the npm version command calling scripts/npm-version.sh, and I don't believe the package.json files are built into the bundle output. If I'm wrong about that, then this needs to be more complicated. (It probably should be complicated, but I was hoping to take it one step at a time...)
I think the ideal is something like:
- make a local version bump commit
- build and test
- if tests pass, merge the version bump upstream (FF if possible)
- npm publish
I'm still working through that mentally... let me know if you see any gaps. The build scripts for our Android app do something like that, and it's a bit of a pain to set up, but it works.
Coming back to reality, I will definitely be carefully inspecting the first publish after this merge. It wouldn't surprise me too much if there's a problem introduced by the build happening before the version bump, in which case I'll either try to fix it quickly or revert this change.
Proposed Changes
Main change: make sure that the publish workflow uses the same build output that was tested.
Supporting changes: lots of GHA cleanup & refinement.
Reason for Changes
This removes the possibility of introducing new issues between the build-and-test in the CI workflow and the build-and-publish of the publishing workflow. In theory, those two builds should be the same, but we couldn't be certain of that before. By publishing the existing build artifacts, we can now be certain. It also saves some time in the publishing workflow. In the event that we publish a commit-ish for which no build is available - for example, if we publish a commit that affected only docs, or only files in
.github/- then fall back to building everything as part of the publishing workflow.The rest of the changes are more of a "while I'm in here" set of things, including:
env:for these reasons. Not all of the values are vulnerable to that, but I'd like to establish it as a habit.testandresultsif no packages have been modifiedTest Coverage
I tested as much as I could locally by running shell commands in a terminal, etc., but the real test will be when this is merged and the first time we publish a new release after that.