client/container_exec: Separate structs for Start and Attach#51289
client/container_exec: Separate structs for Start and Attach#51289thaJeztah merged 1 commit intomoby:masterfrom
Conversation
| // ExecStart will first check if it's detached | ||
| Detach bool |
There was a problem hiding this comment.
So for ExecAttach - does it make sense to have a Detach option?
| // Terminal size [height, width], unused if Tty == false | ||
| ConsoleSize *[2]uint `json:",omitempty"` |
There was a problem hiding this comment.
We could make this explicit With, Height (could be a struct);
type ConsoleSize struct { Width, Height uint}Could be either a pointer, or just consider the zero-value( 0, 0) being "not set" when we convert it to the API request, WDYT?
There was a problem hiding this comment.
Yeah good idea.
Also thinking if that struct could be combined with the TTY field itself (as the Console size doesnt make sense without TTY).
But:
type TTYConsole struct {
Enabled bool
Size ConsoleSize
}Is not ideal because you could still pass Enabled=false so we don't gain much.
And:
type TTYConsole struct {
Size ConsoleSize
}Would be confusing because the struct being non-nil would be equivalent to the current Tty=true and that would be easy to do "accidentaly".
Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
client-container-exec2
branch
from
397205c to
4aac139
Compare
| ConsoleSize *[2]uint `json:",omitempty"` | ||
| TTY bool | ||
| // Terminal size [height, width], unused if TTY == false | ||
| ConsoleSize ConsoleSize `json:",omitzero"` |
There was a problem hiding this comment.
Nit: we can remove the json: label, because client options are never expected to be marshalled to JSON.
Fine for a follow-up though; there may be more.
| // Terminal size [height, width], unused if TTY == false | ||
| ConsoleSize ConsoleSize `json:",omitzero"` |
| func (cli *Client) ExecStart(ctx context.Context, execID string, options ExecStartOptions) (ExecStartResult, error) { | ||
| req := container.ExecStartRequest{ | ||
| Detach: options.Detach, | ||
| Tty: options.Tty, | ||
| ConsoleSize: options.ConsoleSize, | ||
| Detach: options.Detach, |
There was a problem hiding this comment.
For a follow-up, I'm considering;
- Perhaps we should ditch
ExecAttachaltogether, and just haveExecStart(with options to attach) - Change
DetachtoAttach, so thatExecStartdefaults to just "starting", but optionally allows to attach.
There was a problem hiding this comment.
I think we should just ditch ExecAttach in favor of ExecStart.
There was a problem hiding this comment.
Yup; agreed; added it to the list here;
2 participants
Uh oh!
There was an error while loading. Please reload this page.