Improve apps release #22127
Improve apps release #22127
Conversation
WalkthroughThis pull request introduces a new script for managing application releases, located in Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
.github/scripts/release-apps.js (1)
160-161: Add error handling when invoking the main functionThe
main()function is asynchronous but is called without handling potential rejections. To prevent unhandled promise rejections, wrap the call in a.catch()method.Apply this diff to handle errors when calling
main():-main(); +main().catch(err => { + console.error('An unexpected error occurred:'); + console.error(err); + process.exit(1); +});.github/workflows/ci.yml (1)
1138-1139: Fix environment variable interpolation in jsDelivr cache purge URLThe URL in the
Purge jsdelivr cachestep uses{{ env.current_minor }}, but GitHub Actions expressions require${{ }}syntax. The variable will not interpolate correctly, resulting in an incorrect URL.Apply this diff to fix the variable interpolation:
url: | - https://cdn.jsdelivr.net/ghost/portal@~{{ env.current_minor }}/umd/portal.min.js + https://cdn.jsdelivr.net/ghost/portal@~${{ env.current_minor }}/umd/portal.min.js
🧹 Nitpick comments (2)
.github/scripts/release-apps.js (2)
4-4: Add missing semicolon for consistent syntaxThere's a missing semicolon at the end of the import statement on line 4. Adding it will maintain consistent syntax throughout the code.
Apply this diff to add the semicolon:
const readline = require('readline/promises') +;
59-69: Reuse a singlereadlineinterface to improve efficiencyCurrently, separate
readlineinterfaces are created ingetNewVersionandgetChangelog. Reusing a single interface throughout the script can improve efficiency and resource management.Here's how you can modify the code:
+const rl = readline.createInterface({input: process.stdin, output: process.stdout}); async function getNewVersion() { - const rl = readline.createInterface({input: process.stdin, output: process.stdout}); const bumpTypeInput = await rl.question('Is this a patch, minor or major (patch)? '); - rl.close(); const bumpType = bumpTypeInput.trim().toLowerCase() || 'patch'; if (!['patch', 'minor', 'major'].includes(bumpType)) { console.error(`Unknown bump type ${bumpTypeInput} - expected one of "patch", "minor, "major"`) process.exit(1); } return semver.inc(APP_VERSION, bumpType); } async function getChangelog(newVersion) { - const rl = readline.createInterface({input: process.stdin, output: process.stdout}); const i18nChangesInput = await rl.question('Does this release contain i18n updates (Y/n)? '); - rl.close(); const i18nChanges = i18nChangesInput.trim().toLowerCase() !== 'n'; // Rest of the function } +// Close the readline interface after all prompts are completed async function main() { await ensureEnabledApp(); await ensureNotOnMain(); await ensureCleanGit(); console.log(`Running release for ${APP_NAME}`); console.log(`Current version is ${APP_VERSION}`); + const newVersion = await getNewVersion(); + const changelog = await getChangelog(newVersion); + + rl.close(); console.log(`Bumping to version ${newVersion}`); await updatePackageJson(newVersion); await exec(`git add package.json`); // Rest of the function }Also applies to: 91-94
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/scripts/release-apps.js(1 hunks).github/workflows/ci.yml(1 hunks)apps/portal/package.json(1 hunks)ghost/core/core/shared/config/defaults.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- ghost/core/core/shared/config/defaults.json
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Comments-UI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (1)
apps/portal/package.json (1)
29-29: Updatedshipscript to use the new release management scriptThe
shipscript now correctly points to the newrelease-apps.js, streamlining the release process.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
.github/scripts/release-apps.js (1)
160-161: Add error handling when invoking the main functionThe
main()function is asynchronous but is called without handling potential rejections. To prevent unhandled promise rejections, wrap the call in a.catch()method.Apply this diff to handle errors when calling
main():-main(); +main().catch(err => { + console.error('An unexpected error occurred:'); + console.error(err); + process.exit(1); +});.github/workflows/ci.yml (1)
1138-1139: Fix environment variable interpolation in jsDelivr cache purge URLThe URL in the
Purge jsdelivr cachestep uses{{ env.current_minor }}, but GitHub Actions expressions require${{ }}syntax. The variable will not interpolate correctly, resulting in an incorrect URL.Apply this diff to fix the variable interpolation:
url: | - https://cdn.jsdelivr.net/ghost/portal@~{{ env.current_minor }}/umd/portal.min.js + https://cdn.jsdelivr.net/ghost/portal@~${{ env.current_minor }}/umd/portal.min.js
🧹 Nitpick comments (2)
.github/scripts/release-apps.js (2)
4-4: Add missing semicolon for consistent syntaxThere's a missing semicolon at the end of the import statement on line 4. Adding it will maintain consistent syntax throughout the code.
Apply this diff to add the semicolon:
const readline = require('readline/promises') +;
59-69: Reuse a singlereadlineinterface to improve efficiencyCurrently, separate
readlineinterfaces are created ingetNewVersionandgetChangelog. Reusing a single interface throughout the script can improve efficiency and resource management.Here's how you can modify the code:
+const rl = readline.createInterface({input: process.stdin, output: process.stdout}); async function getNewVersion() { - const rl = readline.createInterface({input: process.stdin, output: process.stdout}); const bumpTypeInput = await rl.question('Is this a patch, minor or major (patch)? '); - rl.close(); const bumpType = bumpTypeInput.trim().toLowerCase() || 'patch'; if (!['patch', 'minor', 'major'].includes(bumpType)) { console.error(`Unknown bump type ${bumpTypeInput} - expected one of "patch", "minor, "major"`) process.exit(1); } return semver.inc(APP_VERSION, bumpType); } async function getChangelog(newVersion) { - const rl = readline.createInterface({input: process.stdin, output: process.stdout}); const i18nChangesInput = await rl.question('Does this release contain i18n updates (Y/n)? '); - rl.close(); const i18nChanges = i18nChangesInput.trim().toLowerCase() !== 'n'; // Rest of the function } +// Close the readline interface after all prompts are completed async function main() { await ensureEnabledApp(); await ensureNotOnMain(); await ensureCleanGit(); console.log(`Running release for ${APP_NAME}`); console.log(`Current version is ${APP_VERSION}`); + const newVersion = await getNewVersion(); + const changelog = await getChangelog(newVersion); + + rl.close(); console.log(`Bumping to version ${newVersion}`); await updatePackageJson(newVersion); await exec(`git add package.json`); // Rest of the function }Also applies to: 91-94
🛑 Comments failed to post (4)
.github/scripts/release-apps.js (3)
45-50:
⚠️ Potential issueProperly handle exceptions from
execinensureCleanGitfunctionThe
execfunction may reject the Promise if the command fails. Currently, errors fromexecare not caught, which may cause unhandled exceptions. Wrap theexeccall in atry...catchblock to handle possible errors.Apply this diff to fix the error handling in
ensureCleanGit:async function ensureCleanGit() { + try { const localGitChanges = await exec(`git status --porcelain`); - if (localGitChanges.stderr) { - console.error(`There was an error checking the local git status`) - console.error(`${localGitChanges.stderr}`); - process.exit(1); - } - if (localGitChanges.stdout) { console.error(`You have local git changes - are you sure you're ready to release?`) console.error(`${localGitChanges.stdout}`); process.exit(1); } + } catch (err) { + console.error(`There was an error checking the local git status`); + console.error(`${err.message}`); + process.exit(1); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async function ensureCleanGit() { try { const localGitChanges = await exec(`git status --porcelain`); if (localGitChanges.stdout) { console.error(`You have local git changes - are you sure you're ready to release?`); console.error(`${localGitChanges.stdout}`); process.exit(1); } } catch (err) { console.error(`There was an error checking the local git status`); console.error(`${err.message}`); process.exit(1); } }
31-36:
⚠️ Potential issueHandle exceptions when using
execto prevent uncaught errorsThe
execfunction returns a Promise that rejects if the command exits with a non-zero status. Currently, if thegitcommand fails, the script may terminate with an unhandled exception. Wrappingexeccalls in atry...catchblock ensures proper error handling.Apply this diff to fix the error handling in
ensureNotOnMain:async function ensureNotOnMain() { + try { const currentGitBranch = await exec(`git branch --show-current`); - if (currentGitBranch.stderr) { - console.error(`There was an error checking the current git branch`) - console.error(`${currentGitBranch.stderr}`); - process.exit(1); - } - - if (currentGitBranch.stdout.trim() === 'main') { + const currentBranch = currentGitBranch.stdout.trim(); + if (currentBranch === 'main') { console.error(`The release can not be done on the "main" branch`) process.exit(1); } + } catch (err) { + console.error(`There was an error checking the current git branch`); + console.error(`${err.message}`); + process.exit(1); + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async function ensureNotOnMain() { try { const currentGitBranch = await exec(`git branch --show-current`); const currentBranch = currentGitBranch.stdout.trim(); if (currentBranch === 'main') { console.error(`The release can not be done on the "main" branch`); process.exit(1); } } catch (err) { console.error(`There was an error checking the current git branch`); console.error(`${err.message}`); process.exit(1); } }
72-74:
⚠️ Potential issueCorrect the path to
defaults.jsoninupdateConfigfunctionThe path to
defaults.jsonincludes a duplicatecoredirectory. It should likely be'../../ghost/core/shared/config/defaults.json'instead of'../../ghost/core/core/shared/config/defaults.json'.Apply this diff to fix the path:
const defaultConfigPath = path.resolve(__dirname, '../../ghost/core/core/shared/config/defaults.json'); const defaultConfig = require(defaultConfigPath); -const defaultConfigPath = path.resolve(__dirname, '../../ghost/core/core/shared/config/defaults.json'); +const defaultConfigPath = path.resolve(__dirname, '../../ghost/core/shared/config/defaults.json'); +const defaultConfig = require(defaultConfigPath);Committable suggestion skipped: line range outside the PR's diff.
.github/workflows/ci.yml (1)
1128-1128:
⚠️ Potential issueCorrect the
ifexpression syntax when referencing environment variablesIn GitHub Actions, when using environment variables in
ifexpressions, wrap them with${{ }}to ensure proper evaluation. Without this syntax, the condition may not work as intended.Apply this diff to fix the
ifexpressions:- name: Publish to npm - if: env.version_changed == 'true' + if: ${{ env.version_changed == 'true' }} working-directory: apps/portal env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} run: npm publish --access public - name: Purge jsdelivr cache - if: env.version_changed == 'true' + if: ${{ env.version_changed == 'true' }} uses: gacts/purge-jsdelivr-cache@v1 with: url: |Also applies to: 1135-1135
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #22127 +/- ##
==========================================
+ Coverage 74.01% 74.43% +0.42%
==========================================
Files 1272 1266 -6
Lines 75840 75576 -264
Branches 10102 10085 -17
==========================================
+ Hits 56130 56257 +127
+ Misses 18766 18371 -395
- Partials 944 948 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
allouis
force-pushed
the
improve-apps-release
branch
from
2fdcb7a to
e5baaf7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/scripts/release-apps.js (1)
4-4: Add missing semicolon for consistency.For consistency with the rest of the codebase, consider adding a semicolon at the end of the
requirestatement.Apply this diff:
-const readline = require('readline/promises') +const readline = require('readline/promises');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/scripts/release-apps.js(1 hunks)apps/portal/package.json(1 hunks)ghost/core/core/shared/config/defaults.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/portal/package.json
- ghost/core/core/shared/config/defaults.json
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Signup-form tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Comments-UI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Lint
- GitHub Check: i18n
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (1)
.github/scripts/release-apps.js (1)
87-88: Verify that updating the config version to major.minor is intentional.In the
updateConfigfunction, the version is set to${semver.major(newVersion)}.${semver.minor(newVersion)}, omitting the patch version. Please confirm that excluding the patch version is intentional and aligns with your versioning strategy.
allouis
force-pushed
the
improve-apps-release
branch
2 times, most recently
from
a93b2f0 to
d667c99
Compare
This uses the same approach that we have for admin-x-activitypub. When a version bump for the @tryghost/portal package is commited to the `main` branch, we will build the package and then publish it to npm, as well as purging the jsdelivr cache of the last version.
This will be used for search and comments too, but I want to start with just one app to get things going. Key things the script does: - Enforces that we're not on main & have clean working dir - Creates a well structured commit message w/ basic changelog - Updates the package.json and default config The script explicitly does not push anything to GitHub or to NPM, giving us more control about the contents of the commit
Since the release script for portal writes to this file, I wanted to just match the JSON.stringify output so we don't get large diffs.
allouis
force-pushed
the
improve-apps-release
branch
from
d667c99 to
f52dba6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
1075-1138:⚠️ Potential issueReview the new
publish_portaljob for consistency and proper condition evaluation
The addition of thepublish_portaljob is well structured and mirrors the existing release job for@tryghost/admin-x-activitypub. A couple of points for improvement:
- If-condition Syntax: Several steps (e.g. building, publishing, purging cache) use
if: env.version_changed == 'true'
It is recommended to wrap these expressions in${{ }}(e.g.if: ${{ env.version_changed == 'true' }}) to ensure proper evaluation in GitHub Actions. This change is similar to previous feedback—please update these conditions accordingly.- URL Interpolation in Cache Purge Step: In the “Purge jsdelivr cache” step, the URL is currently written as:
https://cdn.jsdelivr.net/ghost/portal@~{{ env.current_minor }}/umd/portal.min.js
For correct interpolation, it should include the missing dollar sign before the braces, like so:
https://cdn.jsdelivr.net/ghost/portal@~${{ env.current_minor }}/umd/portal.min.js
Making these adjustments will improve consistency and ensure that the conditional logic and URL interpolation behave as intended.
🧹 Nitpick comments (2)
apps/portal/README.md (2)
76-78: Clarify the release commit merge step
For both the patch release (line 77) and the minor/major release (line 82) sections, the instruction to “Merge the release commit tomain” has been added. This change clearly emphasizes that the release commit must be merged to trigger the new CI/CD process. Ensure that all team members are aware of this revised workflow step.
85-88: Enhanced JsDelivr cache purge instructions
The newly added “JsDelivr cache” section provides clear manual steps to purge the cache in case the CI does not clear it automatically. The instructions, including the URLs with the${PORTAL_VERSION}variable, add useful context for managing cache issues. Verify that the usage of the tilde (~) in the URL is consistent with your npm/semver conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/scripts/release-apps.js(1 hunks).github/workflows/ci.yml(1 hunks)apps/portal/README.md(1 hunks)apps/portal/package.json(1 hunks)ghost/core/core/shared/config/defaults.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/portal/package.json
- ghost/core/core/shared/config/defaults.json
- .github/scripts/release-apps.js
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Signup-form tests
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Comments-UI tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Admin tests - Chrome
ref #22127 The update to the Portal release process has now been tested on main, it would be good to get Search & Comments updated to the same process so that it's easier to reason about releasing all of them.
ref #22127 The update to the Portal release process has now been tested on main, it would be good to get Search & Comments updated to the same process so that it's easier to reason about releasing all of them.
ref #22127 The update to the Portal release process has now been tested on main, it would be good to get Search & Comments updated to the same process so that it's easier to reason about releasing all of them.
Improve apps release process
The current flow for releasing Portal, Search, et al. is error prone and wonky. The package is published to
npmbefore being merged to the repository, and you can end up in weird states due to a lack of checks during.This PR aims to make the process smoother, starting with Portal - but with the idea of moving the others apps to it.
The first change is that the publishing of the package is handled by GitHub CI, we already have an established pattern for this with the
@tryghost/admin-x-activitypubpackage, and the code for that has been largely copied.The way it works is that when there is a commit to
mainwhich has changed theversionfield of the@tryghost/portalpackage.json - we will build the package, publish it to npm, and then purge the jsDelivr cache.The second change build on top of this, and introduces a new script for the
yarn shipcommand in @tryghost/portal - this is a simple script which ensures your local state it suitable for creating a release, and then will create a local commit with the version bump and a basic changelog (A list of commits which referenced a Linear issue and touched a file in the portal package, and depending on the answer to a question about i18n updates, a note that the translations have been updated)It is then up to the developer to get amend this commit if necessary and get it merged to main, to kick of the release process described above.
Here's a small demo of creating a patch with i18n changes, followed by a minor without
CleanShot.2025-02-06.at.17.08.45.mp4