-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
remote build: fix streaming and error handling #11067
Conversation
Address a number of issues in the streaming logic in remote build, most importantly an error in using buffered channels on the server side. The pattern below does not guarantee that the channel is entirely read before the context fires. for { select { case <- bufferedChannel: ... case <- ctx.Done(): ... } } Fixes: containers#10154 Signed-off-by: Valentin Rothberg <[email protected]>
@edsantiago @rhatdan @jwhonce PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR, #9887 passes.
To understand what that means, go look at that PR. I'll wait. Scroll up to April 27, when I filed #10154. Look at all the force-pushes I've done since then. All of those failed with multiple flakes. This is the first flake-free run in exactly three months. So, in short, LGTM. Nice work, @vrothberg, and thank you!
is "$output" \ | ||
".*Error: error creating build container: quay.io/libpod/nosuchimage:nosuchtag: image not known" \ | ||
"--pull-never fails with expected error message" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, nice catch. I had forgotten all about this skip.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
/hold cancel |
Address a number of issues in the streaming logic in remote build, most
importantly an error in using buffered channels on the server side.
The pattern below does not guarantee that the channel is entirely read
before the context fires.
Fixes: #10154
Signed-off-by: Valentin Rothberg [email protected]
... had to create a new PR. GitHub did not like to reopen after I pushed to the branch.