add --force-rm to clean up after a failed build #5839
Conversation
|
Great! Would be happy to test when you have this working. |
| @@ -110,6 +110,7 @@ func (cli *DockerCli) CmdBuild(args ...string) error { | |||
| suppressOutput := cmd.Bool([]string{"q", "-quiet"}, false, "Suppress the verbose output generated by the containers") | |||
| noCache := cmd.Bool([]string{"#no-cache", "-no-cache"}, false, "Do not use cache when building the image") | |||
| rm := cmd.Bool([]string{"#rm", "-rm"}, true, "Remove intermediate containers after a successful build") | |||
| forceRm := cmd.Bool([]string{"#force-rm", "-force-rm"}, false, "Always remove intermediate containers, even after unsuccessful builds") | |||
There was a problem hiding this comment.
I don't think we need to add the deprecated flag. We can stick with the '--' version
|
I've made the required changes to the docs, I've added a test and one more commit to add back @philips You can try it out already, it only needed docs and tests. ping @vieux @creack @crosbymichael |
|
I've made the remaining changes and I've added one more batch of cli integration tests. |
|
Coud you update Otherwise LGTM |
This adds back the rm query parameter to the remote api docs for api v1.10 and v1.11. Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)
This adds a `--force-rm` flag to docker build which makes the Docker daemon clean up all containers, even when the build has failed. This new flag requires that we bump the remote API, so we also bump the remote API version. Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)
Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)
Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)
This makes the remote API version 1.12 and newer default to automatically deleting intermediate containers when the build has succeedeed. Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)
Docker-DCO-1.1-Signed-off-by: Cristian Staretu <cristian.staretu@gmail.com> (github: unclejack)
|
@vieux Could you take another look, please? |
|
LGTM |
|
Docs LGTM |
|
@unclejack @vieux would it be better to have this as the default or is the flag the right way? |
|
@crosbymichael We're defaulting |
|
@unclejack yes, but why don't we force-rm as the default for --rm? It seems more ppl want these removed anyways and don't care |
|
@crosbymichael It just seems like it might be unexpected to see all the containers deleted without notice upon failure. For example, if you're doing an expensive build (bandwidth, computation and disk io) and there's nothing left behind to debug, it might be annoying to see the container is gone and you have to repeat the build with |
|
ok, sounds good. I just wanted to double check to see what you thought |
| @@ -903,12 +903,20 @@ func postBuild(eng *engine.Engine, version version.Version, w http.ResponseWrite | |||
| } else { | |||
| job.Stdout.Add(utils.NewWriteFlusher(w)) | |||
| } | |||
|
|
|||
| if r.FormValue("forcerm") == "1" && version.GreaterThanOrEqualTo("1.12") { | |||
There was a problem hiding this comment.
Shouldn't you use the getBoolParam and not just check for 1 on these?
There was a problem hiding this comment.
No, this is ok. The code is doing the same thing for rm below because we need to check for specific values explicitly. This makes it easier to have tri-state values (missing, false, true) for this in the future, just like I've changed --rm to be tri-state (missing with default to true, false and true).
|
LGTM |
add --force-rm to clean up after a failed build
This PR is ready for review.
This adds a
--force-rmflag to docker build which makes the Docker daemon clean up all containers, even when the build has failed.