support dry-run for exec command #10252
Conversation
Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
glours
requested review from
a team,
nicksieger,
ndeloof,
StefanScherer,
ulyssessouza,
milas and
laurazard
and removed request for
a team
Codecov ReportBase: 73.89% // Head: 72.79% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## v2 #10252 +/- ##
==========================================
- Coverage 73.89% 72.79% -1.11%
==========================================
Files 2 2
Lines 272 272
==========================================
- Hits 201 198 -3
- Misses 60 62 +2
- Partials 11 12 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
pkg/api/dryrunclient.go
Outdated
| } | ||
|
|
||
| func (d *DryRunClient) ContainerExecStart(ctx context.Context, execID string, config moby.ExecStartCheck) error { | ||
| fmt.Printf("%sExecuting command in detach mode\n", DRYRUN_PREFIX) |
There was a problem hiding this comment.
why "in detach(ed) mode" ?
when starting exec command, you can't tell if client did/will attach or not
There was a problem hiding this comment.
When I tested all the potential cases, I identified that ContainerExecStart was only called when the detached mode is activated
https://github.com/docker/cli/blob/24b4924410dc1db788ce1ee59e2cc7fa8459f8b0/cli/command/container/exec.go#L139
There was a problem hiding this comment.
Do you think it's worth having something like map[string]ExecDetails on the DryRunClient?
That way, ExecCreate could be silent, but here we could log out DRY-RUN Running command "rm -rf /" in app-svc-1.
(Don't feel strongly about this, also seems fine as-is!)
There was a problem hiding this comment.
ContainerExecAttach in the client indeed calls the /start engine endpoint with Detach set but this can be used to distinguish detached mode.
There was a problem hiding this comment.
@milas I've implemented suggested change with execDetails, could you please review ?
pkg/api/dryrunclient.go
Outdated
| func (d *DryRunClient) ContainerExecCreate(ctx context.Context, container string, config moby.ExecConfig) (moby.IDResponse, error) { | ||
| fmt.Printf("%sCreating Exec configuration for container %s with command '%s'\n", DRYRUN_PREFIX, container, strings.Join(config.Cmd, " ")) | ||
| config.Cmd = []string{"true"} | ||
| return d.apiClient.ContainerExecCreate(ctx, container, config) |
There was a problem hiding this comment.
Should this be return nil?
I know we aren't starting the command here, but not sure of any other possible implications here
pkg/api/dryrunclient.go
Outdated
| } | ||
|
|
||
| func (d *DryRunClient) ContainerExecStart(ctx context.Context, execID string, config moby.ExecStartCheck) error { | ||
| fmt.Printf("%sExecuting command in detach mode\n", DRYRUN_PREFIX) |
There was a problem hiding this comment.
Do you think it's worth having something like map[string]ExecDetails on the DryRunClient?
That way, ExecCreate could be silent, but here we could log out DRY-RUN Running command "rm -rf /" in app-svc-1.
(Don't feel strongly about this, also seems fine as-is!)
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
What I did
Add support of dry run mode for
execcommandRelated issue
https://docker.atlassian.net/browse/ENV-55
(not mandatory) A picture of a cute animal, if possible in relation to what you did
