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

Refactored container wait and make it public #2384

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

Conversation

KevinLiAWS
Copy link

@KevinLiAWS KevinLiAWS commented Jul 20, 2023

Wait container is useful for external callers.
The current container wait private method only return the error without the container status code, instead, it print the code.
This PR refactored the code to return both the status code and error, and make it public.
From docker API, the status code is int64, so converted the original uint32 to int64, and also use -1 to indicate non container wait error status.

@KevinLiAWS KevinLiAWS marked this pull request as ready for review July 20, 2023 21:23
@AkihiroSuda AkihiroSuda added this to the v1.5.0 milestone Jul 21, 2023
allErr = multierror.Append(allErr, waitErr)
}
}
return allErr
}

func waitContainer(ctx context.Context, w io.Writer, container containerd.Container) error {
func WaitContainer(ctx context.Context, container containerd.Container) (code int64, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@ningziwen ningziwen Jul 22, 2023

Choose a reason for hiding this comment

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

If we want to keep the pattern consistent with other recent refactoring, instead of making methods public, we can also move stdout to cmd from pkg.
#2333
#2342

@AkihiroSuda AkihiroSuda modified the milestones: v1.5.1, v1.5.2 Sep 11, 2023
@AkihiroSuda
Copy link
Member

Needs rebase

@AkihiroSuda AkihiroSuda removed this from the v1.6.1 (tentative) milestone Oct 8, 2023
@apostasie
Copy link
Contributor

Hey @KevinLiAWS

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants