Make HAProxy configurable#240
Open
mclasmeier wants to merge 13 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe change centralizes command configuration assembly, moves the config-file flag to the root command, adds configurable HAProxy bind ports, propagates Roxie settings through subshell execution, and records the active HAProxy port for Central connection output. ChangesConfiguration and HAProxy command flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant runCommandOrSubshell
participant tryStartHAProxy
participant startHAProxy
participant HAProxy
Command->>runCommandOrSubshell: RoxieConfig
runCommandOrSubshell->>tryStartHAProxy: CentralDeploymentInfo
tryStartHAProxy->>startHAProxy: Validated startup request
startHAProxy->>HAProxy: Start with rendered config and bind port
HAProxy-->>runCommandOrSubshell: Active HAProxyPort
runCommandOrSubshell-->>Command: Forwarded Central connection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cmd/flags.go, cmd/main.go: Enable flag inheritance for --config, so that it is available on subcommands. cmd/deploy_test.go: Extract subcommand from root command so that they related a parent/child and therefore parsing of `--config` works as expected.
mclasmeier
force-pushed
the
mc/haproxy-configurable
branch
from
51d181c to
301552d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/subshell.go (1)
150-153: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider capturing HAProxy stderr for diagnostics.
cmd.Stderr = nildiscards all HAProxy error output. If HAProxy fails to bind or encounters config errors, there's no diagnostic information. Sincecmd.Start()only verifies the process launched (not that it runs successfully), a silent startup failure would leave the user with a banner advertising a URL that doesn't work.♻️ Proposed refactor: capture stderr for error reporting
cmd := exec.Command("haproxy", "-f", configPath) cmd.Stdin = nil cmd.Stdout = nil -cmd.Stderr = nil +var stderr bytes.Buffer +cmd.Stderr = &stderr if err := cmd.Start(); err != nil { os.Remove(configPath) return nil, "", fmt.Errorf("failed to start haproxy: %w", err) } +// Give HAProxy a moment to fail on config/bind errors. +time.Sleep(200 * time.Millisecond) +if cmd.ProcessState != nil && cmd.ProcessState.Exited() { + os.Remove(configPath) + return nil, "", fmt.Errorf("haproxy exited immediately: %s", stderr.String()) +} centralDeploymentInfo.HAProxyPort = roxieConfig.HAProxy.BindPort🤖 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 `@cmd/subshell.go` around lines 150 - 153, Update the HAProxy command setup around exec.Command to capture stderr instead of discarding it, and use the captured diagnostic output when cmd.Start or subsequent startup handling reports failure. Preserve the existing stdin/stdout behavior while ensuring bind and configuration errors are surfaced to the user rather than leaving a misleading success banner.
🤖 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 `@cmd/subshell.go`:
- Around line 150-153: Update the HAProxy command setup around exec.Command to
capture stderr instead of discarding it, and use the captured diagnostic output
when cmd.Start or subsequent startup handling reports failure. Preserve the
existing stdin/stdout behavior while ensuring bind and configuration errors are
surfaced to the user rather than leaving a misleading success banner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 0abbd1c6-ac9a-4d12-bbff-21702a5b8f7c
📒 Files selected for processing (9)
cmd/deploy.gocmd/deploy_test.gocmd/flags.gocmd/main.gocmd/shell.gocmd/subshell.gointernal/deployer/config.gointernal/haproxy/config.gointernal/haproxy/config_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/shell.go
- internal/deployer/config.go
- cmd/deploy.go
- cmd/main.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
--config/-coption to load YAML configuration, including reading from standard input.