- Notifications
You must be signed in to change notification settings - Fork925
fix(agent/agentcontainers): handle race between docker ps and docker inspect#17447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: Ideally I'd like to avoid disconnected err handling (code slipping in betweenerr := andreturn err due to whitespace), but I'll settle for removing the newline.
(Preferable, but not required: return explicitly either nil or err fromif err != nil-block.)
Then it's probably a matter of your Docker configuration, you might have to add |
6a79965 intomainUh oh!
There was an error while loading.Please reload this page.
Fixescoder/internal#586 (comment)