Use a consistent approach for producing warnings, but add a TODO for moving
this warning to the daemon, which can make a better call if it will work
or not (depending on networking mode).
This warning was originally added in [moby@afa92a9], before integration with
libnetwork, and this warning may be incorrect in many scenarios.
While updating, also removing the custom regular expression used to
detect if the IP is a loopback address, and using go's netip package
instead.
[moby@afa92a9]: afa92a9af0
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This warning was originally added in [moby@3aa70c1], and moved to be printed
on both `run` and `create` in commit 7c514a31c9.
However, [moby@57f1305] (docker 19.03, API 1.40) moved such warnings to
the daemon side. The patch mentioned this issue:
> This patch will have one side-effect; docker cli's that also perform this check
> client-side will print the warning twice; this can be addressed by disabling
> the cli-side check for newer API versions, but will generate a bit of extra
> noise when using an older CLI.
The CLI does not take this into account currently, and still prints warnings
twice; even in cases where the option is not supported by the daemon, and
discarded:
On a host without OomKillDisable support:
docker create --oom-kill-disable alpine
WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.
WARNING: Your kernel does not support OomKillDisable. OomKillDisable discarded.
On a host that supports it:
docker create --oom-kill-disable alpine
WARNING: Disabling the OOM killer on containers without setting a '-m/--memory' limit may be dangerous.
WARNING: OOM killer is disabled for the container, but no memory limit is set, this can result in the system running out of resources.
This patch removes the client-side warning, leaving it to the daemon to
report if any warnings should produced (and the client to print them).
With this patch applied:
On a host without OomKillDisable support:
docker create --oom-kill-disable alpine
WARNING: Your kernel does not support OomKillDisable. OomKillDisable discarded.
On a host that supports it:
docker create --oom-kill-disable alpine
WARNING: OOM killer is disabled for the container, but no memory limit is set, this can result in the system running out of resources.
[moby@3aa70c1]: 3aa70c1948
[moby@57f1305]: 57f1305e74
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was shared between "image" and "container" packages,
all of which needed the trust package, so move it there instead.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
cli/command/container/cp.go:206:3: naked return in func `resolveLocalPath` with 5 lines of code (nakedret)
return
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
cli/command/container/client_test.go:78:7: unused-receiver: method receiver 'f' is not referenced in method's body, consider removing or renaming it as _ (revive)
func (f *fakeClient) ContainerExecStart(context.Context, string, container.ExecStartOptions) error {
^
cli/command/container/create_test.go:383:7: unused-receiver: method receiver 'f' is not referenced in method's body, consider removing or renaming it as _ (revive)
func (f fakeNotFound) NotFound() {}
^
cli/command/container/create_test.go:384:7: unused-receiver: method receiver 'f' is not referenced in method's body, consider removing or renaming it as _ (revive)
func (f fakeNotFound) Error() string { return "error fake not found" }
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Previously, multiple suggestions were provided when completing
commands like `run`, `history` and `push`. This change
limits completion to a single suggestion for the above and 2 suggestions for `tag`
Signed-off-by: Mohammed Aminu Futa <mohammedfuta2000@gmail.com>
Without breaking API compatibility, this patch allows us to know whether
a returned `cli/StatusError` was caused by a context cancellation or
not, which we can use to provide a nicer UX and not print the Go
"context canceled" error message if this is the cause.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
- use apiClient instead of client for the API client to
prevent shadowing imports.
- use dockerCLI with Go's standard camelCase casing.
- suppress some errors to make my IDE and linters happier
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- use Println to print newline instead of custom format
- suppress some errors to make my IDE and linters happier
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
While there may be reasons to keep pkg/errors in production code,
we don't need them for these tests.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The '--link' option should only be migrated to an endpoint
option if the network is user-defined ... there was already
an exception for network "default", but not for "bridge".
Signed-off-by: Rob Murray <rob.murray@docker.com>
Allows for the `jsonmessage.DisplayJSONMessagesStream` function
to correctly return when the context is cancelled with the appropriate
reason (`ctx.Error()`) instead of just a nil error.
Follow-up to 30a73ff19c
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Co-authored-by: Paweł Gronowski <pawel.gronowski@docker.com>
Currently the cp will tar from the same directory it will untar into
simultaneously. There is a race between reading the file and truncating
the file for write, however, the race will not show up with a large
enough buffer on the tar side if buffered before the copy begins.
Also removes the unnecessary deferred removal, the removal is handled by
cleanup and respects the no cleanup env.
Signed-off-by: Derek McGowan <derek@mcg.dev>
If STDOUT or STDERR are attached and the container exits, the streams
will be closed by the daemon while the container is exiting, causing
the streamer to return an error
61b02e636d/cli/command/container/hijack.go (L53)
that gets sent
61b02e636d/cli/command/container/run.go (L278)
and received
61b02e636d/cli/command/container/run.go (L225)
on `errCh`.
However, if only STDIN is attached, it's not closed (since this is
attached to the user's TTY) when the container exits, so the streamer
doesn't exit and nothing gets sent on `errCh`, meaning the CLI execution
hangs receiving on `errCh` on L231.
Change the logic to receive on both `errCh` and `statusChan` – this way,
if the container exits, we get notified on `statusChan` (even if only
STDIN is attached), and can cancel the streamer and exit.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
Now, if running in "detached" mode, we early exit at L222.
Similarly, if `attachContainer` errors out, it returns an error that
gets handled on L190.
As such, `errCh` can never be nil on L231. Remove the nil check.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
Since everything else after the `apiClient.ContainerStart` block is
under an `if attach` conditional, we can move the "detached" early exit
up.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
During a `docker run`, the CLI has some different behavior/output
depending on whether the run is "detached" or not.
In some cases, the CLI is checking whether either `stdin`, `stdout` or
`stderr` are attached, but in other cases we're only checking `stdout`
and `stderr`, which leads to some inconsistencies:
```
$ docker run -a stdout --rm --name test alpine top
[docker kill test]
exit status 137
$ docker run -a stderr --rm --name test alpine top
[docker kill test]
exit status 137
$ docker run -a stdin --rm --name test alpine top
56820d94a89b96889478241ae68920323332c6d4cf9b51ba9340cba01e9e0565
[docker kill test]
[no exit code]
```
Since we're not checking for whether `stdin` is attached when deciding
whether to early exit without receiving on `statusChan`, the `docker run
-a stdin` is falling into the "detached mode" logic, which simply prints
the container ID and doesn't print/return the exit code.
This patch makes the "attached" checks consistent.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
This patch fixes the context cancellation
behaviour for the `runContainer` function,
specifically the `createContainer` function
introduced in this commit 991b1303da.
It delays stripping the `cancel` from the context
passed into the `runContainer` function so that
the `createContainer` function can be cancelled
gracefully by a SIGTERM/SIGINT.
This is especially true when the requested image
does not exist and `docker run` needs to `pull`
the image before creating the container.
Although this patch does gracefully cancel
the `runContainer` function it does not address
the root cause. Some functions in the call path
are not context aware, such as `pullImage`.
Future work would still be necessary to ensure
a consistent behaviour in the CLI.
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
Instead of clearing the whole screen and then writing the new stats,
we now write the new stats on top of the old text, and then clear
the remaining text.
This is a more efficient way to update the stats, as it avoids the
flickering that happens when the screen is cleared and rewritten.
Signed-off-by: Giedrius Jonikas <giedriusj1@gmail.com>
make shell
make -C ./internal/gocompat/
GO111MODULE=on go test -v
# github.com/docker/cli/cli/command/container
../../cli/command/container/completion.go:37:28: implicit function instantiation requires go1.18 or later (-lang was set to go1.16; check go.mod)
../../cli/command/container/completion.go:82:25: implicit function instantiation requires go1.18 or later (-lang was set to go1.16; check go.mod)
../../cli/command/container/completion.go:92:27: implicit function instantiation requires go1.18 or later (-lang was set to go1.16; check go.mod)
FAIL gocompat [build failed]
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
commit 4a7b04d412 configured golangci-lint
to use go1.23 semantics, which enabled the copyloopvar linter.
go1.22 now creates a copy of variables when assigned in a loop; make sure we
don't have files that may downgrade semantics to go1.21 in case that also means
disabling that feature; https://go.dev/ref/spec#Go_1.22
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
