Use check in params so we don't ignore errors#29224
Conversation
AkihiroSuda
added
the
status/failing-ci
vdemeester
force-pushed
the
check-no-errors
branch
from
c9d7129 to
270351f
Compare
There was a problem hiding this comment.
plugin remove doesn't answer http.StatusNoContent (maybe it should but it does not)
There was a problem hiding this comment.
I think we should fix that if it doesn't return content /cc @anusha-ragunathan
There was a problem hiding this comment.
Also, best practice could be to check for "any" 2xx status code
There was a problem hiding this comment.
This make sure it will also delete image like imageName:<none> which are generated on some DockerRegistrySuite tests. There was some left overs, and as we ignored the errors, we didn't even know it…
vdemeester
removed
the
status/failing-ci
|
It showed some errors that weren't catched… updates should fix them 👼 |
Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
check-no-errors
branch
from
270351f to
bcad3d5
Compare
| deleteAllImages(c) | ||
| deleteAllVolumes(c) | ||
| deleteAllNetworks(c) | ||
| if daemonPlatform == "linux" { |
There was a problem hiding this comment.
Plugins aren't supported by windows (and possibly other daemon platform), so we should try to clean them on those.
vdemeester
force-pushed
the
check-no-errors
branch
2 times, most recently
from
eca1039 to
5580052
Compare
vdemeester
added
the
status/failing-ci
vdemeester
force-pushed
the
check-no-errors
branch
from
5580052 to
0eca882
Compare
Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
check-no-errors
branch
from
0eca882 to
58ad917
Compare
vdemeester
removed
the
status/failing-ci
| fmt.Printf("ERROR: couldn't resolve full path to the Docker binary (%v)\n", err) | ||
| os.Exit(1) | ||
| } | ||
| if registryImage := os.Getenv("REGISTRY_IMAGE"); registryImage != "" { |
There was a problem hiding this comment.
Do we know why this was there?
There was a problem hiding this comment.
Not really, but it's dead code 😅
| c.Assert(out, checker.Contains, id[:12], check.Commentf("container should be restarted instead of removed: %v", out)) | ||
|
|
||
| // Kill the container to make sure it will be removed | ||
| dockerCmd(c, "kill", id) |
There was a problem hiding this comment.
So, this tests is making sure a container started with --rm will not be removed when doing a restart one it. This means, when TearDown gets there, it's failing with Removal in progress. By killing it before, I make sure it's getting cleaned before.
There was a problem hiding this comment.
Thanks. Ideally docker rm should just wait until the removal finishes instead of erroring, but that's a different issue.
|
LGTM |
|
|
||
| func deleteAllImages() error { | ||
| cmd := exec.Command(dockerBinary, "images") | ||
| func deleteAllImages(c *check.C) { |
There was a problem hiding this comment.
As a follow up, we should use docker image prune -af 😄
5 participants
Small improvements mainly on the
TearDownTestfor the suite(s) so we make sure to not ignore some errors./cc @cpuguy83 @dnephin @icecrime @thaJeztah @AkihiroSuda @vieux @tiborvass
🐻
Signed-off-by: Vincent Demeester [email protected]