Skip to content
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

refactor: allow custom handlers for processing push/pull status updates #2342

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mharwani
Copy link

@mharwani mharwani commented Jun 30, 2023

Contributing to the refactoring issue #1680

Currently, the way image push/pull commands are refactored, it is difficult to obtain download/upload status updates programmatically. Nerdctl has its own printing logic, but users may want to handle status updates directly via code when importing those subcommands. These changes allow users to define custom handlers for processing status updates during push/pull.

@mharwani mharwani marked this pull request as ready for review June 30, 2023 11:07
@AkihiroSuda AkihiroSuda added this to the v1.5.0 milestone Jul 3, 2023
@mharwani mharwani force-pushed the mharwani/refactor-push-pull branch from 61efde6 to 458f034 Compare July 10, 2023 11:27
@@ -165,14 +169,15 @@ type ImagePushOptions struct {
IpfsAddress string
// Suppress verbose output
Quiet bool
// If non-nil, the Push job will send upload statuses to the handler instead of Stdout
ProgressHandler jobs.StatusHandler
// AllowNondistributableArtifacts allow pushing non-distributable artifacts
AllowNondistributableArtifacts bool
}

// ImagePullOptions specifies options for `nerdctl (image) pull`.
type ImagePullOptions struct {
Stdout io.Writer
Copy link
Contributor

Choose a reason for hiding this comment

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

How about just removing Stdout and pass progressHandler as jobs.PrintProgress(cmd.OutOrStderr()) in CLI layer?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good to me. The push command also prints the image reference to stdout if --quiet flag is set, so I have changed the push/pull method signature to return image reference along with error.

@mharwani mharwani force-pushed the mharwani/refactor-push-pull branch 2 times, most recently from f29ec0e to 2df8955 Compare July 19, 2023 17:14
@mharwani mharwani requested a review from ningziwen July 19, 2023 17:16
Copy link
Contributor

@ningziwen ningziwen left a comment

Choose a reason for hiding this comment

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

LGTM for the refactoring pattern.

Copy link
Contributor

@ningziwen ningziwen left a comment

Choose a reason for hiding this comment

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

The integration tests are failing and please squash the commits to one.

@AkihiroSuda
Copy link
Member

Needs rebase

@AkihiroSuda AkihiroSuda modified the milestones: v1.5.1, v1.5.2 Sep 11, 2023
@AkihiroSuda AkihiroSuda removed this from the v1.6.1 (tentative) milestone Oct 8, 2023
@apostasie
Copy link
Contributor

Hey @mharwani

Do you think you could rebase this?

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.

4 participants