container/exec: Support ConsoleSize#43622
Conversation
thaJeztah
added
area/api
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! Sorry for the delay; I recall I had some thoughts when looking at the changes earlier, but needed to take a bit of time to try to phrase them 😅
Left my comments; happy to hear your (and other's) thoughts on them 👍
vvoland
force-pushed
the
3554-exec-size
branch
2 times, most recently
from
3ec72fd to
6115c65
Compare
|
CI failure is unrelated - flaky test TestSwarmLockUnlockCluster |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
one suggestion, but otherwise looks good
I was writing along while reviewing, so let me push my changes as a PR against your branch
|
Oh! Didn't comment here; I opened vvoland#1 with my suggestion |
| AttachStderr: true | ||
| DetachKeys: "ctrl-p,ctrl-q" | ||
| Tty: false | ||
| ConsoleSize: nil |
There was a problem hiding this comment.
I'm pretty sure that nil is not an alias for null in YAML, so this construct would be equivalent to the JSON {"ConsoleSize": "nil"}. I don't think that's your intention. Maybe omit it entirely from the Tty: false example and provide a separate example with Tty: true and ConsoleSize set to an array, like the ExecStartConfig example below?
There was a problem hiding this comment.
Thanks, good catch! See what Go does to some people... 😄
Unfortunately multiple examples seems to be supported in OpenAPI 3.0 and we use 2.0 here (https://swagger.io/docs/specification/adding-examples/).
So I'll just leave the Tty: false and remove ConsoleSize from here. So we don't repeat the same example as ExecStartConfig.
There was a problem hiding this comment.
Oh, nice catch! I wanted to comment "perhaps remove this one" when I reviewed, but decided not to, and completely missed the nil vs null 😂
As to OpenAPI 3.0 - we should probably move to 3.0 at some point, but I looked into that in the past and automatic conversions weren't great (so still a lot of manual work to do to rewrite it), but also with the tooling we currently use (at least the current version of that) not really working with it, I decided that it was not worth the effort at that point (don't break what works 😅). (also see #42593)
We could of course look into it again; I recently had a discussion with some people from the Docker Hub team, who were planning to change some of their API docs to 3.0.
Now client have the possibility to set the console size of the executed process immediately at the creation. This makes a difference for example when executing commands that output some kind of text user interface which is bounded by the console dimensions. Signed-off-by: Paweł Gronowski <[email protected]>
|
@crazy-max this expected? I see the |
3 participants
Relates to
Now client have the possibility to set the console size of the executed process immediately at the creation. This makes a difference for example when executing commands that output some kind of text user interface which is bounded by the console dimensions.
- What I did
Make exec accept the console size and set it for the created exec
- How I did it
Introduced ConsoleSize parameter which is copied to specs.Process that is passed to containerd.Exec
- How to verify it
docker exec:- Description for the changelog
Exec allows to set the console size for the exec'd process immediately when it's created
- A picture of a cute animal (not mandatory but encouraged)
