-
Notifications
You must be signed in to change notification settings - Fork 0
fix(release): make changelog generation resilient to GitHub flakiness #21
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| --- | ||
|
|
||
| Internal release-infra change (no version bump): make Changesets changelog | ||
| generation resilient to transient GitHub GraphQL failures by retrying and | ||
| falling back to a git-based changelog, so the "version packages" release step | ||
| no longer aborts on "Failed to parse data from GitHub / Premature close". |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| // Resilient changelog generator for changesets. | ||
| // | ||
| // We normally use @changesets/changelog-github so the generated CHANGELOG gets | ||
| // rich, linked entries (PR numbers, author credits). That generator calls the | ||
| // GitHub GraphQL API, which is intermittently flaky in CI: a single dropped | ||
| // connection surfaces as | ||
| // | ||
| // Failed to parse data from GitHub | ||
| // Invalid response body while trying to fetch https://api.github.com/graphql: Premature close | ||
| // | ||
| // and, because @changesets/apply-release-plan generates all entries inside one | ||
| // Promise.all(), that single rejection aborts the entire `changeset version` | ||
| // run ("We have escaped applying the changesets..."). The release PR never gets | ||
| // created and a human has to babysit re-runs. | ||
| // | ||
| // This wrapper makes the GitHub enrichment best-effort: | ||
| // 1. Retry the GitHub call a few times. @changesets/get-github-info batches | ||
| // via dataloader, which clears failed keys on rejection, so each retry | ||
| // re-issues the GraphQL query rather than replaying a cached failure. | ||
| // 2. If GitHub is still unreachable, fall back to @changesets/changelog-git — | ||
| // plain entries with commit SHAs, no network required. | ||
| // | ||
| // The release proceeds either way; the only downside when GitHub is down is a | ||
| // less decorated changelog for that one release (which can be polished by hand | ||
| // afterwards if desired). Warnings are logged so the degradation is visible in | ||
| // the CI output. | ||
|
|
||
| const github = require("@changesets/changelog-github").default; | ||
| const git = require("@changesets/changelog-git").default; | ||
|
|
||
| // Read tunables at call time (not module load) so tests can override them via | ||
| // process.env without fighting ES module import hoisting. Invalid overrides | ||
| // (non-numeric, negative, NaN) are ignored in favour of the safe defaults so a | ||
| // typo'd env var can't silently break retries. | ||
| function getConfig() { | ||
| const attempts = Number(process.env.CHANGELOG_GITHUB_ATTEMPTS); | ||
| const delay = Number(process.env.CHANGELOG_GITHUB_RETRY_MS); | ||
| return { | ||
| // Always run at least one attempt. | ||
| maxAttempts: Number.isInteger(attempts) && attempts >= 1 ? attempts : 3, | ||
| // Non-negative, finite delay only. | ||
| retryDelayMs: Number.isFinite(delay) && delay >= 0 ? delay : 1000, | ||
| }; | ||
| } | ||
|
Comment on lines
+35
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If We should validate that both parsed values are valid non-negative integers, falling back to safe defaults if they are not.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed — 🤖 Generated by /pr-fixup command |
||
|
|
||
| // Extract a log-safe message from an unknown thrown value. The underlying | ||
| // generators throw Error objects, but we must never let a non-Error rejection | ||
| // (e.g. `throw undefined`) make `.message` throw inside the catch block — that | ||
| // would crash the very generation this wrapper exists to keep alive. | ||
| const errorMessage = (err) => (err && err.message) || String(err); | ||
|
|
||
| const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
|
|
||
| // Run the GitHub generator with retries; fall back to the git generator if it | ||
| // keeps failing. `label` is only used for log messages. | ||
| async function withFallback(label, githubFn, gitFn) { | ||
| const { maxAttempts, retryDelayMs } = getConfig(); | ||
| let lastError; | ||
|
|
||
| for (let attempt = 1; attempt <= maxAttempts; attempt++) { | ||
| try { | ||
| return await githubFn(); | ||
| } catch (error) { | ||
| lastError = error; | ||
| if (attempt < maxAttempts) { | ||
| console.warn( | ||
| `[changelog] GitHub enrichment for ${label} failed ` + | ||
| `(attempt ${attempt}/${maxAttempts}): ${errorMessage(error)}. Retrying...`, | ||
| ); | ||
| // Linear backoff: 1x, 2x, ... the base delay. | ||
| await sleep(retryDelayMs * attempt); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.warn( | ||
| `[changelog] GitHub enrichment for ${label} failed after ${maxAttempts} ` + | ||
| `attempts: ${errorMessage(lastError)}. Falling back to a plain ` + | ||
| `(git) changelog entry for this release.`, | ||
| ); | ||
| return gitFn(); | ||
| } | ||
|
Comment on lines
+56
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If Additionally, if We should safely extract the error message using optional chaining/fallback (
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — fixed. Added an 🤖 Generated by /pr-fixup command |
||
|
|
||
| async function getReleaseLine(changeset, type, options) { | ||
| return withFallback( | ||
| "release line", | ||
| () => github.getReleaseLine(changeset, type, options), | ||
| () => git.getReleaseLine(changeset, type, options), | ||
| ); | ||
| } | ||
|
|
||
| async function getDependencyReleaseLine(changesets, dependenciesUpdated, options) { | ||
| return withFallback( | ||
| "dependency release line", | ||
| () => github.getDependencyReleaseLine(changesets, dependenciesUpdated, options), | ||
| () => git.getDependencyReleaseLine(changesets, dependenciesUpdated, options), | ||
| ); | ||
| } | ||
|
|
||
| // `withFallback` is exported for unit testing (vi.mock cannot intercept the | ||
| // require() of the underlying generators across the CJS boundary, so the retry | ||
| // /fallback logic is tested directly with injected fakes instead). | ||
| module.exports = { getReleaseLine, getDependencyReleaseLine, withFallback }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | ||
| // @ts-expect-error - plain CJS module, no type declarations | ||
| import changelog from "./changelog.cjs"; | ||
|
|
||
| const { withFallback } = changelog; | ||
|
|
||
| // The wrapper delegates getReleaseLine / getDependencyReleaseLine to | ||
| // `withFallback`, which holds the retry-then-fall-back logic. We test that logic | ||
| // directly with injected fakes rather than mocking @changesets/changelog-github | ||
| // (vi.mock cannot intercept the require() inside the .cjs module). | ||
| describe("changelog withFallback", () => { | ||
| beforeEach(() => { | ||
| // Remove retry delays so the failing-path tests run instantly. vi.stubEnv + | ||
| // vi.unstubAllEnvs keeps these mutations from leaking out of the file. | ||
| vi.stubEnv("CHANGELOG_GITHUB_RETRY_MS", "0"); | ||
| vi.stubEnv("CHANGELOG_GITHUB_ATTEMPTS", undefined); | ||
| vi.spyOn(console, "warn").mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllEnvs(); | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("uses the GitHub generator when it succeeds (no fallback)", async () => { | ||
| const githubFn = vi.fn().mockResolvedValue("- GH line"); | ||
| const gitFn = vi.fn().mockResolvedValue("- git line"); | ||
|
|
||
| const result = await withFallback("release line", githubFn, gitFn); | ||
|
|
||
| expect(result).toBe("- GH line"); | ||
| expect(githubFn).toHaveBeenCalledTimes(1); | ||
| expect(gitFn).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("retries the GitHub generator and keeps its result on recovery", async () => { | ||
| const githubFn = vi | ||
| .fn() | ||
| .mockRejectedValueOnce(new Error("Premature close")) | ||
| .mockResolvedValue("- GH line"); | ||
| const gitFn = vi.fn().mockResolvedValue("- git line"); | ||
|
|
||
| const result = await withFallback("release line", githubFn, gitFn); | ||
|
|
||
| expect(result).toBe("- GH line"); | ||
| expect(githubFn).toHaveBeenCalledTimes(2); | ||
| expect(gitFn).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("falls back to the git generator after GitHub keeps failing", async () => { | ||
| const githubFn = vi.fn().mockRejectedValue(new Error("Premature close")); | ||
| const gitFn = vi.fn().mockResolvedValue("- abc1234: git line"); | ||
|
|
||
| const result = await withFallback("release line", githubFn, gitFn); | ||
|
|
||
| expect(result).toBe("- abc1234: git line"); | ||
| expect(githubFn).toHaveBeenCalledTimes(3); // default maxAttempts | ||
| expect(gitFn).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("falls back without throwing when GitHub rejects with a non-Error value", async () => { | ||
| // A non-Error rejection must not make `.message` throw inside the catch. | ||
| const githubFn = vi.fn().mockRejectedValue(undefined); | ||
| const gitFn = vi.fn().mockResolvedValue("- git line"); | ||
|
|
||
| const result = await withFallback("release line", githubFn, gitFn); | ||
|
|
||
| expect(result).toBe("- git line"); | ||
| expect(githubFn).toHaveBeenCalledTimes(3); | ||
| expect(gitFn).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("honours a custom attempt count via CHANGELOG_GITHUB_ATTEMPTS", async () => { | ||
| vi.stubEnv("CHANGELOG_GITHUB_ATTEMPTS", "5"); | ||
| const githubFn = vi.fn().mockRejectedValue(new Error("Premature close")); | ||
| const gitFn = vi.fn().mockResolvedValue("- git line"); | ||
|
|
||
| await withFallback("release line", githubFn, gitFn); | ||
|
|
||
| expect(githubFn).toHaveBeenCalledTimes(5); | ||
| expect(gitFn).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("ignores an invalid attempt-count override and uses the default", async () => { | ||
| vi.stubEnv("CHANGELOG_GITHUB_ATTEMPTS", "-1"); | ||
| const githubFn = vi.fn().mockRejectedValue(new Error("Premature close")); | ||
| const gitFn = vi.fn().mockResolvedValue("- git line"); | ||
|
|
||
| await withFallback("release line", githubFn, gitFn); | ||
|
|
||
| expect(githubFn).toHaveBeenCalledTimes(3); // -1 ignored -> default 3 | ||
| expect(gitFn).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("tolerates a non-numeric retry-delay override without producing NaN waits", async () => { | ||
| vi.stubEnv("CHANGELOG_GITHUB_RETRY_MS", "not-a-number"); | ||
| vi.stubEnv("CHANGELOG_GITHUB_ATTEMPTS", "1"); // 1 attempt -> no sleep path | ||
| const githubFn = vi.fn().mockRejectedValue(new Error("Premature close")); | ||
| const gitFn = vi.fn().mockResolvedValue("- git line"); | ||
|
|
||
| const result = await withFallback("release line", githubFn, gitFn); | ||
|
|
||
| expect(result).toBe("- git line"); | ||
| expect(githubFn).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("exposes getReleaseLine and getDependencyReleaseLine for changesets", () => { | ||
| expect(typeof changelog.getReleaseLine).toBe("function"); | ||
| expect(typeof changelog.getDependencyReleaseLine).toBe("function"); | ||
| }); | ||
| }); |
Some generated files are not rendered by default. Learn more about how customized files appear on 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.
🟡 MEDIUM RISK
Suggestion: The import logic may fail if the required modules do not have a .default export. Consider using a fallback to the module itself for better compatibility.
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.
Holding off on this one. Both deps are exact-pinned (
@changesets/changelog-github@0.6.0,@changesets/changelog-git@0.2.1) and I verified each exports onlydefault(Object.keys(require(...))→['default']). Changesets' own loader (apply-release-plan) also unwraps.default, so if it were ever missing the whole release would break regardless — and|| require(...)would just resolve to the namespace object (which has nogetReleaseLine), so it wouldn't actually add safety. Keeping the explicit.default.🤖 Generated by /pr-fixup command