Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new command behavior lacks required integration coverage for successful and failed pulls.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Moves image-pull handling into the shared container engine layer and improves build pull output and failures.
Changes:
- Streams high-level pull status while capturing noisy layer output.
- Reports engine stderr when pulls fail.
File summaries
| File | Description |
|---|---|
contract/build/container.rs |
Uses the shared pull helper. |
container/shared.rs |
Implements streaming and failure capture. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// stderr is captured rather than shown and surfaced only when the pull | ||
| /// fails, as `PullImageFailed`, so a failed pull can report the engine's own | ||
| /// error. A missing engine binary surfaces via `io_error` as `NotFound`. | ||
| pub(crate) async fn pull_image(&self, image: &str, print: &Print) -> Result<(), Error> { |
leighmcculloch
left a comment
leighmcculloch
left a comment
There was a problem hiding this comment.
Couple minor things but lgtm.
| let stream_stdout = async { | ||
| if let Some(stdout) = stdout { | ||
| let mut lines = BufReader::new(stdout).lines(); | ||
| while let Ok(Some(line)) = lines.next_line().await { |
There was a problem hiding this comment.
This loops stops if there's a read error, but the pipe will still be open and the command keeps running and I think this means that the OS pipe buffer behind the pipe has the risk of filling and blocking the command being run from completing. Essentially a deadlock.
| docker | ||
| .pull_image(image, print) | ||
| .await | ||
| .map_err(Error::Engine)?; |
There was a problem hiding this comment.
I don't think the map err is required here with the way the error is defined from I noticed a #[from] above for it anyway.
| pub(crate) async fn pull_image(&self, image: &str, print: &Print) -> Result<(), Error> { | ||
| let mut child = self | ||
| .pull_command(image) | ||
| .stdout(Stdio::piped()) |
There was a problem hiding this comment.
How does this work when --quiet is used?
What
Replaces the container build's image-pull output:
stellar contract build --image <ref> --pullnow streams the engine's high-level status lines (Pulling from,Digest,Status) through the normalprintoutput and captures per-layer stderr, surfacing it only if the pull fails. The pull logic moves to a reusableshared::Args::pull_image, and the container-localpull_image/PullImageFailedare removed in favor ofshared::Error::PullImageFailed { image, stderr }(which already flows throughError::Engine).Why
Split out of #2709 (verifiable builds) at review request: the streaming-pull helper is independent of
--verifiableand improves the existing--imagebuild on its own. Pulling it into its own PR keeps #2709 focused and lets this land separately.Known limitations
N/A