Skip to content

install: add progress-fd for install #1431

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmarrero
Copy link
Contributor

Assisted by: Claude Code

This supersedes #982 and is part of #1016

Draft because I have not tested this actually does what we want other than making the tests pass.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces progress reporting for install commands via the --progress-fd option. The implementation correctly passes the ProgressWriter through the necessary functions to provide progress during image pulls. The new functionality is also covered by tests. I've identified one area for improvement concerning an unused function parameter that should be addressed to enhance code clarity.

@jmarrero jmarrero force-pushed the progress-for-install branch from f282db3 to c77a63c Compare July 18, 2025 19:56
Co-authored-by: Colin Walters <[email protected]>
Assisted by: Claude Code
Signed-off-by: Joseph Marrero Corchado <[email protected]>
@cgwalters
Copy link
Collaborator

For tests I'd add to tests-integration/src/install.rs and for verifying that we get JSON out of it at least look at test-image-pushpull-progress.nu. A problem right now is the two test frameworks are totally different.

(tangent but I'm not totally happy with the nushell stuff in the end, it's not bad but it's obscure enough that AI tools IME confabulate a lot. The Rust tests are OK but one general issue there is frameworks like TMT don't make it easy (afaik) to use tests that are compiled, the framework wants to just copy arch-independent scripts/yaml from a git repo)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants