Added support for portal & other apps in containerized Ghost#22121
Conversation
WalkthroughThe changes consolidate the handling of command-line flags and environment variables for the Ghost application. In the Changes
Sequence Diagram(s)sequenceDiagram
participant User as Developer
participant Script as dev.js
participant Env as Environment
User ->> Script: Run command with flags (--flag)
Env -->> Script: Supply GHOST_DEV_APP_FLAGS value
Script ->> Script: Merge CLI flags and env variable into GHOST_APP_FLAGS array
Script ->> Script: Evaluate and choose command (ghost, admin, etc.)
Script -->> User: Execute chosen command, handling errors (e.g., Stripe) accordingly
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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: 0
🔭 Outside diff range comments (1)
.github/scripts/dev.js (1)
106-127:Details
🧩 Analysis chain
Consider adding validation for mutually exclusive flags.
While the portal implementation looks good, there's a potential edge case where both HTTP and HTTPS URLs could be set if the environment has
portal__urlpre-configured and the HTTPS flag is toggled.
🏁 Script executed:
#!/bin/bash # Check if there are any existing configurations that might conflict rg "portal__url.*=.*localhost:(4175|4176)" --type-add 'config:*.json' --type configLength of output: 85
I'll run another shell script to search for any other occurrences of setting or referencing
"portal__url"in the codebase (and to review the surrounding context in.github/scripts/dev.js) to check if there's any evidence of conflicting pre-configurations or additional logic for mutually exclusive flags.
🏁 Script executed:
#!/bin/bash # 1. Search for assignments of 'portal__url' throughout the repository rg "COMMAND_GHOST.env\\['portal__url'\\]" -n . # 2. Show the surrounding context in .github/scripts/dev.js (lines 90 to 140) sed -n '90,140p' .github/scripts/dev.jsLength of output: 2212
Validation for mutually exclusive flags in portal configuration is missing.
Our inspection of.github/scripts/dev.jsshows thatCOMMAND_GHOST.env['portal__url']is always overwritten within the portal block based solely on the presence of thehttpsflag without checking whether a value has been pre-configured. This implies a potential edge case where an externally setportal__urlmight conflict with the in-code assignment, leading to unpredictable behavior.
- The file directly sets the URL based on the flag without validating against an existing configuration.
- There’s no condition to handle or warn if both conditions might inadvertently override one another.
Recommendation: Implement a validation check to ensure that if a
portal__urlis already configured, the code either retains the pre-configured setting or issues an appropriate warning/error when mutually exclusive flags (HTTP versus HTTPS) are activated.
🧹 Nitpick comments (3)
.github/scripts/dev.js (2)
129-138: Consider consolidating the signup form command logic.The conditional command for signup form could be simplified.
- command: GHOST_APP_FLAGS.includes('signup') ? 'nx run @tryghost/signup-form:dev' : 'nx run @tryghost/signup-form:preview', + command: `nx run @tryghost/signup-form:${GHOST_APP_FLAGS.includes('signup') ? 'dev' : 'preview'}`,
140-149: Consider standardizing flag naming conventions.The announcement bar flags use multiple variations (
announcement-bar,announcementBar,announcementbar). While this provides flexibility, it might be better to standardize on kebab-case for consistency with other flags.-if (GHOST_APP_FLAGS.includes('announcement-bar') || GHOST_APP_FLAGS.includes('announcementBar') || GHOST_APP_FLAGS.includes('announcementbar') || GHOST_APP_FLAGS.includes('all')) { +if (GHOST_APP_FLAGS.includes('announcement-bar') || GHOST_APP_FLAGS.includes('all')) {Also applies to: 178-198
compose.yml (1)
10-21: Consider security implications of exposed ports.While the port mappings are necessary for development, exposing multiple ports increases the attack surface. Consider:
- Using a reverse proxy for HTTPS endpoints
- Limiting port exposure in production
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/scripts/dev.js(8 hunks)compose.yml(2 hunks)package.json(1 hunks)
🔇 Additional comments (3)
.github/scripts/dev.js (1)
37-40: LGTM! Improved flag handling mechanism.The new implementation elegantly consolidates flags from both environment variables and command-line arguments, making it more flexible for containerized environments.
compose.yml (1)
31-34: LGTM! Well-structured environment variables.The environment variables are properly configured with default empty values, making them optional but available when needed.
package.json (1)
37-43: LGTM! Well-structured Docker scripts.The Docker scripts are properly updated to:
- Use consistent environment variable handling with defaults
- Maintain consistent formatting across all commands
- Provide flexibility in profile selection
1 participant
no issue
yarn devcommand accepts dash-dash-args to conditionally build & run other apps, like portal, comments, etc.compose.ymlfileyarn devto also check for theGHOST_DEV_APP_FLAGSenvironment variable for these flags, which can be forwarded into the container and make it easier to run these apps using docker compose.