v2 review fixes: lifecycle cleanup and docs#62
Conversation
Ensure pre-start validation failures cancel the derived pipeline context, and include the offending stage name in invalid stream-requirement errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Release any previously configured stdout stream before replacing it with the buffer used by Output, preserving ownership of WithStdoutCloser. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add Function options for declaring stdin and stdout stream requirements, so Function stages can participate in file-preference negotiation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Explain the known deadlock risk for borrowed non-file stdin readers feeding a command that exits without draining stdin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR applies review-driven fixes to the v2 stage pipeline API, focusing on lifecycle cleanup, stream requirement negotiation, and clearer documentation/error reporting.
Changes:
- Ensure the derived pipeline context is canceled on startup failures and improve invalid stream-requirement errors by including the stage name.
- Make
Pipeline.Output()close any previously configuredWithStdoutCloserdestination before switching to an internal capture buffer. - Expose
WithStdinRequirement/WithStdoutRequirementforFunctionstages and document a known stdin limitation with non-file readers.
Show a summary per file
| File | Description |
|---|---|
pipe/pipeline.go |
Adds lifecycle cleanup on pre-start failures, improves requirement validation errors, documents stdin limitation, and adjusts Output() stdout handling. |
pipe/pipeline_test.go |
Adds/updates tests covering Output() closing behavior, Function stream requirements, and updated error messages. |
pipe/function.go |
Exposes FunctionOptions for stdin/stdout stream requirements and refactors forbid options to reuse them. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
mhagger
left a comment
mhagger
left a comment
There was a problem hiding this comment.
I left one change suggestion for you to consider.
| if err := p.stdout.Close(); err != nil { | ||
| return nil, fmt.Errorf("closing previous stdout: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
ISTM that trying to call Output() after having already set stdout would always be a programming error: never a sensible thing to do, likely to happen every time the code is run (i.e., high chance of detecting it in CI), and possibly tricky to figure out if this kind of caller error slipped through. What would you think about making this case panic() instead of silently covering up the mistake?
For that matter, using WithStdout() or WithStderr()/WithStderrCloser() more than once could also panic for the same reason.
There was a problem hiding this comment.
I'd support that, yeah. Making things impossible to misuse is good.
Fixes issues discovered doing a review of the latest
stage-v2-cleanstate./cc @mhagger - these are the changes we discussed.
Copilot Summary
This PR applies small review-driven fixes on top of the v2 stage API branch:
WithStdoutCloserdestination beforeOutput()replaces stdout with its capture buffer.WithStdinRequirementandWithStdoutRequirementsoFunctionstages can participate in stream requirement negotiation without reimplementingStage.