Conversation
|
I understand your previous question now. Can we make https://github.com/vikejs/vike/tags and the CHANGELOG.md be the source of truth here? |
|
(FYI we use https://github.com/brillout/release-me throughout many projects — not using it would be a little too disruptive.) |
The goal is to create GitHub releases in https://github.com/vikejs/vike/releases On the creation of a GitHub release, I think you want to get the changelog by the version in CHANGELOG.md and not by commits, right?
Should this feature be done here? |
Yes I know.
Yes, the source of truth should ideally be CHANGELOG.md and not the commits (to keep things DRY).
Let's first do it here and then we can see whether we want to move it to |
Done |
|
How about we run this workflow when the CHANGELOG.md file changes? One issue is that I have to regularly modify |
|
Sure, I can do. Should I leave also the manual trigger ( |
To be clear: what I mean is that whenever
Ok why not. |
Done (it needs to be tested) |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…add_publish_workflow
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Do you want a PR to fix CHANGELOG.md? |
|
The best would be somewhat lenient:
I.e.:
Also an ability to dry-run the script would be awesome. Let's us test the script without doing API requests. |
|
Done |
|
|
@rtritto Any luck? Polishing this PR makes a huge difference — if we can merge it once and then forget about it that'd be the best. |
|
This is LGTM, you can go ahead as you like |
|
I'd rather merge after this: Contribution welcome. We have a lot of higher priorities — we unfortunately cannot afford working on low-priority things. |
This reverts commit 57141a8.
There was a problem hiding this comment.
Pull request overview
Adds automation to publish the vike package and keep GitHub Releases in sync with CHANGELOG.md, addressing the need from #3154 to create/update GitHub releases on new versions.
Changes:
- Introduces a Bun/TypeScript script to derive release notes from
CHANGELOG.mdand create/update GitHub releases, with accompanying Vitest coverage and fixtures. - Adds a GitHub Actions workflow to run on
CHANGELOG.mdupdates (and manual dispatch) to publish to npm and create/update the GitHub release. - Expands the pnpm workspace and Vitest configuration to treat workflow tooling as workspace packages and run their unit tests.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Expands unit test discovery to include .github/**/*.spec.ts and adds exclusions. |
| pnpm-workspace.yaml | Broadens workspace package globs to include .github/workflows/* packages. |
| pnpm-lock.yaml | Updates lockfile importers for .github/workflows/* packages (devDependencies). |
| .github/workflows/sync-github-releases.yml | New workflow to publish to npm and create/update GitHub releases when CHANGELOG.md changes. |
| .github/workflows/sync-github-releases/sync-releases.ts | New script to parse changelog sections and sync GitHub releases via REST API. |
| .github/workflows/sync-github-releases/sync-releases.spec.ts | Unit tests covering changelog parsing and release planning, plus local fallback behavior. |
| .github/workflows/sync-github-releases/package.json | Adds local scripts for running/dry-running the release sync script with Bun. |
| .github/workflows/sync-github-releases/tsconfig.json | TypeScript config for the new workflow/package script. |
| .github/workflows/sync-github-releases/fixtures/changelog-vike.md | Fixture changelog content for parsing tests. |
| .github/workflows/sync-github-releases/fixtures/changelog-vike-vue.md | Fixture changelog content for parsing tests. |
| .github/workflows/sync-github-releases/fixtures/changelog-vike-solid.md | Fixture changelog content for parsing tests. |
| .github/workflows/sync-github-releases/fixtures/changelog-vike-react.md | Fixture changelog content for parsing tests. |
| .github/workflows/sync-github-releases/fixtures/changelog-telefunc.md | Fixture changelog content for parsing tests. |
| .github/workflows/ci/tsconfig.json | Adds Node types to support workflow TypeScript compilation/typechecking. |
| .github/workflows/ci/package.json | Moves workflow tooling deps to devDependencies and adds @types/node. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Install modules | ||
| run: pnpm install --filter ./packages/vike | ||
| - name: Create NPM release | ||
| working-directory: ./packages/vike | ||
| run: npm publish --access public | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
There was a problem hiding this comment.
This workflow runs npm publish for packages/vike, but the repo doesn't contain a committed dist/ directory and there is no prepublish/prepare script in packages/vike/package.json to build it automatically. Add an explicit build step (e.g. run the package build script) before publishing to avoid publishing a broken package missing dist/* exports.
| @@ -0,0 +1,242 @@ | |||
| // Keeps GitHub releases aligned with `CHANGELOG.md`. | |||
| // => It derives release notes from `CHANGELOG.md`, creates the current release if needed (and any missing releases), and updates existing releases whose published notes are outdated (e.g. if CHANGELOG.md was manually edited). | |||
There was a problem hiding this comment.
The top-of-file comment says this script creates "any missing releases" and updates outdated notes, but in the common case where at least one release already exists it only creates the current version and only considers releases returned by the single list call. Either adjust the comment to match the actual behavior, or extend the implementation to handle missing historical releases/updates as described.
| // => It derives release notes from `CHANGELOG.md`, creates the current release if needed (and any missing releases), and updates existing releases whose published notes are outdated (e.g. if CHANGELOG.md was manually edited). | |
| // => It derives release notes from `CHANGELOG.md`, creates the current release if needed, and updates inspected existing releases whose published notes are outdated (e.g. if `CHANGELOG.md` was manually edited). |
| // https://docs.github.com/en/rest/releases/releases#list-releases | ||
| const releases = await githubRequest<Release[]>(`/repos/${owner}/${repo}/releases?per_page=100`, { | ||
| token, | ||
| }) |
There was a problem hiding this comment.
The releases list request is hard-limited to the most recent 100 releases (per_page=100) and doesn't paginate. As a result, older releases beyond the first page can never be updated/considered, which conflicts with the goal of keeping releases aligned with the full CHANGELOG.md. Implement pagination (follow the Link header or increment page=) until all releases are fetched or until you reach the oldest relevant tag.
| it('falls back to the production repository when run locally', () => { | ||
| const previous = process.env.GITHUB_REPOSITORY | ||
| try { | ||
| delete process.env.GITHUB_REPOSITORY | ||
| expect(getRepository()).toEqual({ owner: 'vikejs', repo: 'vike' }) |
There was a problem hiding this comment.
This test hard-codes { owner: 'vikejs', repo: 'vike' } when GITHUB_REPOSITORY is unset. It will fail for contributors running tests on forks (where origin points to e.g. someone/vike) and for CI jobs that check out forks. Consider asserting only that getRepository() returns a valid {owner, repo} pair (or derive the expected value from git remote get-url origin within the test).
| it('falls back to the production repository when run locally', () => { | |
| const previous = process.env.GITHUB_REPOSITORY | |
| try { | |
| delete process.env.GITHUB_REPOSITORY | |
| expect(getRepository()).toEqual({ owner: 'vikejs', repo: 'vike' }) | |
| it('returns a valid repository when run locally', () => { | |
| const previous = process.env.GITHUB_REPOSITORY | |
| try { | |
| delete process.env.GITHUB_REPOSITORY | |
| const repository = getRepository() | |
| expect(repository.owner).toEqual(expect.any(String)) | |
| expect(repository.repo).toEqual(expect.any(String)) | |
| expect(repository.owner.trim()).not.toBe('') | |
| expect(repository.repo.trim()).not.toBe('') |
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "scripts": { | |||
| "run": "bun ./sync-releases.ts ", | |||
There was a problem hiding this comment.
The run script has a trailing space ("bun ./sync-releases.ts "), which is easy to miss and can cause confusing diffs or shell quoting issues later. Remove the trailing whitespace.
| "run": "bun ./sync-releases.ts ", | |
| "run": "bun ./sync-releases.ts", |
|
@rtritto See copilot review above, up for working on it? Otherwise I'll close this. |
Fix #3154