-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Show Cardinal info at cardinal start #98
Conversation
- Reports the Redis, Cardinal, and Cardinal Editor ports for the `cardinal start` command. - pauses after showing info until user hits <ENTER>
WalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StartCmd
participant PortFinder as common.FindUnusedPort
participant Services as getServices(cfg)
participant Docker
User->>StartCmd: Execute "world cardinal start"
StartCmd->>User: Display header (style.CLIHeader)
StartCmd->>User: Show Redis and Cardinal service addresses
alt runEditor enabled
StartCmd->>PortFinder: Find available port for Cardinal Editor
end
StartCmd->>User: Prompt "Press <ENTER> to continue"
User->>StartCmd: Press <ENTER>
StartCmd->>Services: Retrieve services list
Services-->>StartCmd: Return services
StartCmd->>Docker: Start services via Docker
Assessment against linked issues
✨ Finishing Touches
🪧 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 (
|
How to use the Graphite Merge QueueAdd the label graphite/merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/world/cardinal/start.go (1)
134-135
: Consider handling ReadBytes error.The error from
ReadBytes
is currently discarded. While this might be acceptable for user input, consider logging the error for debugging purposes.-_, _ = bufio.NewReader(os.Stdin).ReadBytes('\n') +if _, err := bufio.NewReader(os.Stdin).ReadBytes('\n'); err != nil { + // Log the error but continue execution + fmt.Fprintf(os.Stderr, "Warning: Error reading user input: %v\n", err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/world/cardinal/start.go
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Go
- GitHub Check: Go
- GitHub Check: Unit & Coverage
- GitHub Check: Go
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
cmd/world/cardinal/start.go (3)
4-17
: LGTM! Import changes align with new functionality.The new imports are properly organized and support the enhanced user interaction features.
115-131
: LGTM! Well-structured service information display.The implementation provides clear visibility of service addresses and properly handles the Cardinal Editor configuration.
150-154
: LGTM! Improved code organization.Moving service configuration to
getServices
enhances maintainability while preserving robust error handling.
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.
cardinal start
command.Closes: PLAT-10
Summary by CodeRabbit