runControllerBuild: Read from stdin only when needed #2196
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
Maybe this is related to docker/cli#4792 ? cc @crazy-max @jedevc help wanted for testing 🙏 |
crazy-max
approved these changes
Jan 16, 2024
Member
crazy-max
left a comment
crazy-max
left a comment
There was a problem hiding this comment.
LGTM thanks @ktock!
Tested on my side with cli 25.0.0-rc.2
BUILDX_EXPERIMENTAL=1 ./build/docker-linux-amd64 version
Client:
Version: 25.0.0-rc.2.m
API version: 1.44
Go version: go1.21.6
Git commit: 1fc6ef9d63
Built: Tue Jan 16 16:19:36 2024
OS/Arch: linux/amd64
Context: default
Server: Docker Desktop
Engine:
Version: 25.0.0-rc.1
API version: 1.44 (minimum version 1.24)
Go version: go1.21.5
Git commit: 9cebefa
Built: Thu Jan 4 16:30:11 2024
OS/Arch: linux/amd64
Experimental: false
containerd:
Version: 1.6.26
GitCommit: 3dd1e886e55dd695541fdcd67420c2888645a495
runc:
Version: 1.1.10
GitCommit: v1.1.10-0-g18a0cb0
docker-init:
Version: 0.19.0
GitCommit: de40ad0
BUILDX_EXPERIMENTAL=1 ./build/docker-linux-amd64 buildx version
github.com/docker/buildx v0.12.0-rc2-77-g0dc15505 0dc1550573e45dbd0a374817cc0b310a9c6ec1d6
BUILDX_EXPERIMENTAL=1 ./build/docker-linux-amd64 build .
#0 building with "default" instance using docker driver
#1 [internal] connecting to local controller
#1 DONE 0.0s
#2 [internal] load build definition from Dockerfile
#2 transferring dockerfile: 4.90kB 0.1s done
#2 DONE 0.1s
#3 [auth] docker/dockerfile:pull token for registry-1.docker.io
#3 DONE 0.0s
#4 resolve image config for docker.io/docker/dockerfile:1
#4 DONE 0.8s
#5 docker-image://docker.io/docker/dockerfile:1@sha256:ac85f380a63b13dfcefa89046420e1781752bab202122f8f50032edf31be0021
#5 CACHED
#6 [internal] load metadata for docker.io/tonistiigi/xx:1.2.1
#6 ...
Current forwarder implementation aggressively reads from the reader registered via SetReader. And in runControllerBuild, stdin is always registered to the forwarder by default. But there are cases where stdin should not be read so this commit fix this behaviour to register the stdin reader only when actual read happens. Signed-off-by: Kohei Tokunaga <[email protected]>
Collaborator
Author
|
Thanks! Added small changes for atomicity. So it seems that reading from stdin isn't allowed in some cases so stdin-aware operations (e.g. reading Dockerfile from stdin and launching (TTY-raw-mode) monitor REPL) won't be supported from docker cli in such cases? |
Member
|
Is this still desired after docker/cli#4792 merge? I don't really see why we should have some protections against reading from stdin. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current forwarder implementation aggressively reads from the reader registered via SetReader. And in runControllerBuild, stdin is always registered to the forwarder by default. But there are cases where stdin should not be read so this commit fix this behaviour to register the stdin to the forwarder only when actual read happens.
Related to docker/cli#4792