Conversation
|
replaced the old done channel + infinite select {} with a proper wait loop. We now launch the rollapp process with a dedicated signalChan (for ExecCmdFollow signal notifications) and execDone (for normal completion). The CLI waits on these channels once; if the process exits or a signal is propagated, we log the error and exit instead of hanging forever. The OS keyring prompt responses are collected once and passed down; no more infinite blocking after the rollapp panics on Linux. |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the rollapp start command handling by extracting duplicated code into a reusable function and improving error handling. The changes address code maintainability while adding test coverage for the new functionality.
Changes:
- Extracted rollapp command execution logic into a new
runRollappCommandfunction - Simplified keyring backend handling by consolidating duplicated code paths
- Added unit tests for the new
runRollappCommandfunction with multiple scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| cmd/rollapp/start/start.go | Refactored command execution logic into runRollappCommand function, removed code duplication for keyring backend handling, improved error messages |
| cmd/rollapp/start/start_test.go | Added new test file with unit tests for runRollappCommand covering success, error, signal, and context cancellation scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| execCmdFollowFunc = func(done chan<- error, ctx context.Context, cmd *exec.Cmd, _ map[string]string) error { | ||
| done <- nil | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Test may be flaky: The mock function sends nil to the done channel and immediately returns nil. This creates a race condition where either signalChan or execDone could be selected first in the runRollappCommand select statement. The test expects success, but if execDone is selected first (receiving the return value of nil), the signalChan value would never be read, potentially causing goroutine leaks in production code. Consider adding a small delay or using a more deterministic approach to test the intended behavior.
| select { | ||
| case err := <-signalChan: | ||
| if err != nil { | ||
| return fmt.Errorf("rollapp process received signal: %w", err) | ||
| } | ||
| return nil | ||
| case err := <-execDone: | ||
| return err | ||
| case <-ctx.Done(): | ||
| return fmt.Errorf("context cancelled: %w", ctx.Err()) | ||
| } |
There was a problem hiding this comment.
Logic error in channel handling: The function receives from signalChan (which is the done channel passed to execCmdFollowFunc) and execDone (which receives the return value from execCmdFollowFunc). However, when a signal is received, bash.ExecCmdFollow sends to signalChan and may still return an error. The current select will only read one value, potentially missing errors. Additionally, if signalChan receives nil (successful signal handling), the function returns nil without waiting for the execCmdFollowFunc to complete, which could lead to resource leaks.
| err := runRollappCommand(ctx, exec.Command("echo"), nil) | ||
| if err == nil { | ||
| t.Fatalf("expected error, got nil") | ||
| } |
There was a problem hiding this comment.
Incomplete error handling test: The test only verifies that an error is returned, but doesn't verify that it's the correct error (matching "boom"). Consider using error message assertion to ensure the error is properly propagated.
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := runRollappCommand(ctx, exec.Command("echo"), nil); err != nil { | ||
| t.Fatalf("expected no error, got %v", err) | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: These lines use spaces for indentation while the rest of the file uses tabs. Go convention is to use tabs for indentation.
| done <- errors.New("SIGTERM") | ||
| return nil | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := runRollappCommand(ctx, exec.Command("echo"), nil); err == nil { | ||
| t.Fatalf("expected signal error") | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: These lines use spaces for indentation while the rest of the file uses tabs. Go convention is to use tabs for indentation.
| t.Cleanup(func() { execCmdFollowFunc = bashExecCmdFollowWrapper }) | ||
| execCmdFollowFunc = func(done chan<- error, ctx context.Context, cmd *exec.Cmd, _ map[string]string) error { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| } | ||
| } | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| if err := runRollappCommand(ctx, exec.Command("echo"), nil); err == nil { | ||
| t.Fatalf("expected context error") | ||
| } | ||
| } | ||
|
|
||
| func bashExecCmdFollowWrapper(done chan<- error, ctx context.Context, cmd *exec.Cmd, prompts map[string]string) error { | ||
| return bash.ExecCmdFollow(done, ctx, cmd, prompts) | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: These lines use spaces for indentation while the rest of the file uses tabs. Go convention is to use tabs for indentation.
| func TestRunRollappCommandSignals(t *testing.T) { | ||
| t.Cleanup(func() { execCmdFollowFunc = bashExecCmdFollowWrapper }) | ||
| execCmdFollowFunc = func(done chan<- error, ctx context.Context, cmd *exec.Cmd, _ map[string]string) error { | ||
| done <- errors.New("SIGTERM") | ||
| return nil | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := runRollappCommand(ctx, exec.Command("echo"), nil); err == nil { | ||
| t.Fatalf("expected signal error") | ||
| } |
There was a problem hiding this comment.
Test may not properly verify signal handling: The mock sends an error to the done channel and returns nil, but in the actual bash.ExecCmdFollow implementation, when a signal is received, it sends to doneChan and then continues to execute cmd.Wait() which may also return an error. The test should verify the actual error message or type to ensure the signal error is being handled correctly, not just any error.
PR Standards
Close #1424
Opening a pull request should be able to meet the following requirements
For Author:
godoccommentsFor Reviewer:
After reviewer approval: