fix(agent/agentcontainers): handle race between docker ps and docker inspect #17447
Conversation
| // DockerCLILister is a ContainerLister that lists containers using the docker CLI | ||
| type DockerCLILister struct { | ||
| execer agentexec.Execer | ||
| } | ||
|
|
||
| var _ Lister = &DockerCLILister{} | ||
|
|
||
| func NewDocker(execer agentexec.Execer, opts ...func(*DockerCLILister)) Lister { | ||
| return &DockerCLILister{ | ||
| execer: agentexec.DefaultExecer, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
review: just moved down a bit closer to the method definitions
| if runtime.GOOS != "linux" { | ||
| t.Skip("Skipping on non-Linux OS") | ||
| } | ||
| if _, err := exec.LookPath("devcontainer"); err != nil { | ||
| t.Fatal("this test requires the devcontainer CLI: npm install -g @devcontainers/cli") | ||
| } |
There was a problem hiding this comment.
review: I didn't look too closely into what was actually going on here, but I suspect it's related to path differences between the host and the Docker VM on MacOS/Windows.
There was a problem hiding this comment.
I've run this on macOS as well, so IMO we could allow that, but agree that we need to verify the devcontainer CLI here, good add 👍🏻
| if runtime.GOOS != "linux" { | ||
| t.Skip("Skipping on non-Linux OS") | ||
| } | ||
| if _, err := exec.LookPath("devcontainer"); err != nil { | ||
| t.Fatal("this test requires the devcontainer CLI: npm install -g @devcontainers/cli") | ||
| } |
There was a problem hiding this comment.
I've run this on macOS as well, so IMO we could allow that, but agree that we need to verify the devcontainer CLI here, good add 👍🏻
Interesting, so "works on my machine" 🤔 I tried to figure out what the root cause was on my end, but the error returned by |
|
@johnstcn interesting, here's me on macOS: Perhaps it's a Nix issue? |
It happens outside the nix environment as well, strangely enough 🤷 |
| return stdout, stderr, err | ||
| if err != nil && bytes.Contains(stderr, []byte("No such object:")) { | ||
| // This can happen if a container is deleted between the time we check for its existence and the time we inspect it. | ||
| return stdout, stderr, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: Ideally I'd like to avoid disconnected err handling (code slipping in between err := and return err due to whitespace), but I'll settle for removing the newline.
(Preferable, but not required: return explicitly either nil or err from if err != nil-block.)
Then it's probably a matter of your Docker configuration, you might have to add |
Fixes coder/internal#586 (comment)