Added defaults version validation to app bump check#28187
Conversation
no issue Public app minor bumps also need defaults.json updates so Ghost points at the released major/minor CDN version.
WalkthroughThe PR adds helpers to extract a major.minor string from semver and to read per-app versions from ghost/core/core/shared/config/defaults.json. It reorders the per-app validation to compute PR and main versions before the dependency-only skip, keeps the existing bump-above-main check, and then enforces that defaults.json’s app.version equals the PR major.minor, failing with a targeted message if it does not. A unit test was added that sets up a temp git repo and asserts the script fails when defaults.json has a stale version. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 267447ba30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const prMajorMinorVersion = getMajorMinorVersion(prVersion); | ||
| const mainMajorMinorVersion = getMajorMinorVersion(mainVersion); | ||
|
|
||
| if (prMajorMinorVersion !== mainMajorMinorVersion) { |
There was a problem hiding this comment.
Because the defaults check only runs when the package's major/minor changes relative to origin/main, it misses cases where defaults.json is already stale and the PR is a patch release within that same minor. For example, in this tree apps/comments-ui/package.json is 1.5.4 while defaults.json still has comments.version as 1.4; a PR bumping comments to 1.5.5 would pass this condition and Ghost would keep loading the 1.4 CDN range, so the newly released app still would not be used.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28187 +/- ##
==========================================
- Coverage 73.87% 73.64% -0.24%
==========================================
Files 1530 1536 +6
Lines 129809 130816 +1007
Branches 15572 15657 +85
==========================================
+ Hits 95893 96333 +440
- Misses 32954 33518 +564
- Partials 962 965 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
no issue Patch-only app releases can still expose an already-stale defaults.json version, so the check now compares defaults against every bumped package major/minor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/scripts/check-app-version-bump.test.js (2)
59-67: ⚡ Quick winGuard the spawned script with a timeout to avoid a hanging test process.
If the script hangs, this test can stall the whole suite. Add
timeout(and optionallymaxBuffer) to make failures bounded and easier to diagnose.Proposed change
const result = spawnSync(process.execPath, ['.github/scripts/check-app-version-bump.js'], { cwd: repo, encoding: 'utf8', + timeout: 15_000, + maxBuffer: 1024 * 1024, env: { ...process.env, PR_BASE_SHA: baseSha, PR_COMPARE_SHA: compareSha } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/check-app-version-bump.test.js around lines 59 - 67, The test's spawnSync call (result = spawnSync(process.execPath, ['.github/scripts/check-app-version-bump.js'], { ... })) can hang; add a timeout (e.g. timeout: 10000) to the options object (and optionally maxBuffer) so the child process is killed after a bounded period; update the options passed to spawnSync to include timeout (and maxBuffer if desired) while preserving cwd, encoding and env.
56-71: ⚡ Quick winAdd cleanup for the temporary git repo created by the test.
setupRepo()allocates a temp directory, but this test never removes it. Over repeated local/CI runs, that can accumulate in/tmp.Proposed change
-test('fails patch app bumps when defaults.json is stale for the package major/minor', () => { +test('fails patch app bumps when defaults.json is stale for the package major/minor', (t) => { const {baseSha, compareSha, repo} = setupRepo(); + t.after(() => { + fs.rmSync(repo, {recursive: true, force: true}); + }); const result = spawnSync(process.execPath, ['.github/scripts/check-app-version-bump.js'], {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/check-app-version-bump.test.js around lines 56 - 71, The test 'fails patch app bumps when defaults.json is stale for the package major/minor' creates a temporary repo via setupRepo() but never removes it; update the test to remove the temp directory (the repo variable returned by setupRepo) after the spawnSync call by adding cleanup in a finally-style block or after assertions so the temp dir is deleted (e.g., use fs.rmSync(repo, { recursive: true, force: true }) or call a cleanup function returned by setupRepo if available); reference setupRepo and the local variable repo to locate where to perform the removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/scripts/check-app-version-bump.test.js:
- Around line 59-67: The test's spawnSync call (result =
spawnSync(process.execPath, ['.github/scripts/check-app-version-bump.js'], { ...
})) can hang; add a timeout (e.g. timeout: 10000) to the options object (and
optionally maxBuffer) so the child process is killed after a bounded period;
update the options passed to spawnSync to include timeout (and maxBuffer if
desired) while preserving cwd, encoding and env.
- Around line 56-71: The test 'fails patch app bumps when defaults.json is stale
for the package major/minor' creates a temporary repo via setupRepo() but never
removes it; update the test to remove the temp directory (the repo variable
returned by setupRepo) after the spawnSync call by adding cleanup in a
finally-style block or after assertions so the temp dir is deleted (e.g., use
fs.rmSync(repo, { recursive: true, force: true }) or call a cleanup function
returned by setupRepo if available); reference setupRepo and the local variable
repo to locate where to perform the removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48ae5e22-50d7-43a7-ae3f-43ca37769d7b
📒 Files selected for processing (2)
.github/scripts/check-app-version-bump.js.github/scripts/check-app-version-bump.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/scripts/check-app-version-bump.js
2 participants
Summary