-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: migrate to Craft #1232
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
base: main
Are you sure you want to change the base?
feat: migrate to Craft #1232
Conversation
- Remove @changesets/cli and @svitejs/changesets-changelog-github-compact - Remove changeset:add, changeset:consume, changeset:publish scripts - Delete prepare-publish.yml workflow
- Add .craft.yml with auto versioning and changelog generation - Add scripts/bump-version.sh to update package versions - Add .github/release.yml for conventional commit changelog categories Craft will automatically determine version bumps from conventional commits (feat: → minor, fix: → patch, feat!: → major).
- Update build.yml to trigger on release/** branches - Add release.yml using Craft's reusable release workflow - Update publish.yml to trigger on changelog changes - Add changelog-preview.yml for PR changelog comments
- Update changesets.mdx to explain conventional commits - Update releases.mdx to document new Craft release process
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛Ui
Other
Documentation 📚Website
Other
Build / dependencies / internal 🔧
Other
🤖 This preview updates automatically when you update the PR. |
| name: Preview Changelog | ||
| uses: getsentry/craft/.github/workflows/changelog-preview.yml@v2 | ||
| secrets: inherit |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the problem, add an explicit permissions block that restricts the default GITHUB_TOKEN access for this workflow. Because this workflow is only orchestrating a reusable workflow and does not itself perform any direct repository mutations, a safe and conservative default is contents: read. This adheres to the principle of least privilege while still allowing typical read operations (like fetching code) if needed by the reusable workflow.
The best way to fix this without changing existing functionality is:
- Add a
permissionsblock at the root level of.github/workflows/changelog-preview.yml, alongsidenameandon, so that it applies to all jobs in the workflow (including thechangelog-previewjob). - Set
contents: readas the minimal permission. If the reusable workflow needs additional scopes (for example,pull-requests: write), those should be added there, but we will not assume extra needs beyondcontents: readsince we cannot see the implementation of the reusable workflow and we must avoid altering behavior more than necessary.
Concretely:
- In
.github/workflows/changelog-preview.yml, after thename: Changelog Previewline, insert:
permissions:
contents: readNo imports or additional methods are required, as this is a YAML configuration change only.
-
Copy modified lines R6-R7
| @@ -3,6 +3,8 @@ | ||
| # https://getsentry.github.io/craft/ | ||
|
|
||
| name: Changelog Preview | ||
| permissions: | ||
| contents: read | ||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, edited, labeled, unlabeled] |
| name: Prepare Release | ||
| uses: getsentry/craft/.github/workflows/release.yml@v2 | ||
| with: | ||
| version: ${{ inputs.version || 'auto' }} | ||
| force: ${{ inputs.force }} | ||
| secrets: inherit |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the problem is fixed by explicitly declaring permissions for the GITHUB_TOKEN either at the top level of the workflow (affecting all jobs) or within the specific job. This overrides potentially broad repository defaults and enforces least-privilege access.
For this workflow, the cleanest fix without changing behavior is to add a permissions block at the workflow root (at the same indentation level as on: and jobs:). Because this workflow delegates all work to a reusable workflow maintained by getsentry/craft, we should provide a minimal but sensible default. A safe starting point commonly recommended is contents: read, which allows reading the repository but does not allow writes. If the reusable workflow requires more (for example, to push tags or releases), those permissions should be expanded in the reusable workflow itself; here we’re only addressing the absence of any explicit permissions in this caller. Since we must avoid assuming additional functional requirements, we’ll add the minimal permissions: contents: read at the top level just after the on: block. No imports or extra methods are needed, as this is just YAML configuration.
Concretely:
- Edit
.github/workflows/release.yml. - Insert a
permissions:section after theon:block (after line 17) withcontents: read. - Leave the
jobs:andreleasejob definitions unchanged.
-
Copy modified lines R19-R21
| @@ -16,6 +16,9 @@ | ||
| default: false | ||
| type: boolean | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| release: | ||
| name: Prepare Release |
|
|
||
| - name: Setup NPM dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The publish.yml workflow has a race condition. It queries for a completed build run ID while the build is still in progress, causing the artifact download to fail.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The publish.yml workflow is triggered by a push event, running concurrently with the Build & Test workflow. A step within the publish workflow queries for the run-id of a successfully completed Build & Test run from the same commit SHA. Because the build is still in progress when the query runs, the API call finds no successful run, resulting in an empty RUN_ID. This causes the subsequent actions/download-artifact@v5 step to fail, which blocks the entire release process.
💡 Suggested Fix
Replace the push trigger with a workflow_run trigger. This will ensure the publish.yml workflow only starts after the Build & Test workflow has successfully completed, resolving the race condition.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/publish.yml#L73-L74
Potential issue: The `publish.yml` workflow is triggered by a `push` event, running
concurrently with the `Build & Test` workflow. A step within the publish workflow
queries for the `run-id` of a successfully completed `Build & Test` run from the same
commit SHA. Because the build is still in progress when the query runs, the API call
finds no successful run, resulting in an empty `RUN_ID`. This causes the subsequent
`actions/download-artifact@v5` step to fail, which blocks the entire release process.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8280206
.github/workflows/publish.yml
Outdated
| branches: | ||
| - main | ||
| paths: | ||
| - "packages/spotlight/CHANGELOG.md" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition: Publish may run before Build completes
High Severity
The old workflow used workflow_run with types: completed to wait for the Build & Test workflow to finish before publishing. The new workflow triggers directly on push events, running in parallel with Build & Test. The get_run_id step queries for a completed build with status=success, but if the build hasn't finished yet, this returns an empty run-id. Subsequent artifact download steps will then fail because they cannot reference the build artifacts. This creates an unreliable publish process where timing determines success.
Additional Locations (1)
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| TAG="@spotlightjs/spotlight@${{ needs.get_version.outputs.version }}" | ||
| gh release upload "$TAG" packages/spotlight/dist-electron/*.{dmg,zip,blockmap,yml} --clobber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Electron upload fails when release doesn't exist
Medium Severity
When manually triggering with inputs.target == 'electron', the npm job is skipped because its condition requires inputs.target == 'npm' || inputs.target == 'all' || inputs.target == ''. Since the GitHub release is created in the npm job, the electron job then attempts gh release upload to a non-existent release tag, which will fail. The electron job's condition allows it to run when npm is skipped (needs.npm.result == 'skipped'), but doesn't account for the missing release.
Additional Locations (1)
- Update .craft.yml with proper npm and github targets - Simplify publish.yml to only handle Docker and Electron - NPM publishing and GitHub releases now handled by getsentry/publish - Trigger post-release workflow on release:published event The release flow is now: 1. release.yml -> craft prepare -> creates publish issue 2. getsentry/publish -> craft publish -> npm + GitHub release 3. publish.yml -> Docker tagging + Electron signing
BYK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Follow up:
- Also remove that annoying PR template please
- Me to do NPM org/token shenenigans
.github/workflows/publish.yml
Outdated
| # NPM publishing and GitHub release creation are handled by: | ||
| # 1. release.yml -> craft prepare -> creates publish issue in getsentry/publish | ||
| # 2. getsentry/publish -> craft publish -> publishes to npm + creates GitHub release | ||
| # 3. This workflow triggers on the release event to handle Docker and Electron |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should also be part of the Craft workflow. Craft already has a Docker target so that shouldn't be an issue: https://getsentry.github.io/craft/targets/docker/
For Electron, all we do is upload artifacts to a GitHub release which the GitHub target should already take care of. Ergo we should not have this file.
| name: Prepare Release | ||
| uses: getsentry/craft/.github/workflows/release.yml@v2 | ||
| with: | ||
| version: ${{ inputs.version || 'auto' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fallback is redundant as you already have a default above?
| version: ${{ inputs.version || 'auto' }} | |
| version: ${{ inputs.version }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also change the name of the file and remove all mentions of Changesets?
|
|
||
| This will guide you through the process to define the changeset. You have to select which package(s) are affected by | ||
| this change, if it is a patch/minor/major change, and provide a description for the change. | ||
| ## Changelog Preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably link here: https://getsentry.github.io/craft/configuration/#custom-changelog-entries-from-pr-descriptions
| You can specify: | ||
|
|
||
| - **Version**: Leave as `auto` to determine from commits, or specify `major`, `minor`, `patch`, or an explicit version like `4.10.0` | ||
| - **Force**: Check this to release even if there are release-blockers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably define what a release-blocker is? (Any issue having this label)
| github: | ||
| owner: getsentry | ||
| repo: spotlight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant, Craft infers this automatically
| github: | |
| owner: getsentry | |
| repo: spotlight |
| changelog: | ||
| filePath: packages/spotlight/CHANGELOG.md | ||
| policy: auto | ||
| scopeGrouping: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the default so redundant?
| scopeGrouping: true |
| # Artifacts are uploaded by GitHub Actions | ||
| artifactProvider: | ||
| name: github |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only and default artifact provider should be GitHub so this is redundant?
| # Artifacts are uploaded by GitHub Actions | |
| artifactProvider: | |
| name: github |
| artifactProvider: | ||
| name: github | ||
|
|
||
| # Build status from GitHub Actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Build status from GitHub Actions |
| - /^built-packages$/ | ||
| - /^spotlight-binaries$/ | ||
|
|
||
| # Targets - executed by getsentry/publish repo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deslop these comments please :)
Explains how the Craft + getsentry/publish release flow works, including the Post-Release workflow for Docker and Electron.
This PR migrates our release management from Changesets to Craft, Sentry's release tool.
Why Craft?
Changes
Removed
@changesets/cliand@svitejs/changesets-changelog-github-compactdependenciespackage.jsonprepare-publish.ymlworkflowAdded
.craft.yml- Craft configuration with auto versioning and npm/github targetsscripts/bump-version.sh- Version bump script called by Craft.github/release.yml- Changelog categories for conventional commits.github/workflows/release.yml- Craft release workflow.github/workflows/changelog-preview.yml- PR changelog previewsUpdated
.github/workflows/build.yml- Trigger on release/** branches.github/workflows/publish.yml- Trigger on changelog changes instead of changeset commit messagesNew Release Flow
Tested locally:
