Remove intermediate images and containers from local builds by default #1400
Conversation
There was a problem hiding this comment.
This looks great! I'll test it out as well in a sec, in the meantime:
- What do you think about flipping the flag to become by default
--prune=trueand--prune=falsefor turning it off? - Sorry that this is manual (I'll fix this soon) -- but updating the docs should be easy: if you run
go run cmd/skaffold/man/man.goand replace the contents from### skaffold buildin the docs/content/en/docs/references/cli/_index.md that would be great.
|
After I tried it out, it feels flaky while doing Maybe we should retry/wait somehow until the container exits? |
Codecov Report
@@ Coverage Diff @@
## master #1400 +/- ##
==========================================
- Coverage 51.45% 49.44% -2.02%
==========================================
Files 171 174 +3
Lines 7690 8036 +346
==========================================
+ Hits 3957 3973 +16
- Misses 3357 3686 +329
- Partials 376 377 +1
Continue to review full report at Codecov.
|
I ran through the microservices example 15 times, and I haven't been able to reproduce that issue. I'll go ahead and add a retry loop around it and see if it fixes the issue for you.
I originally had it like this, but I liked |
|
It's a minor thing - we can keep it |
|
@nkubala Could you please rebase? |
|
@balopat @dgageot @priyawadhwa this is RFAL. I reworked it to account for the builder plugins and the artifact cache: when caching, we won't prune all images built by skaffold (so we don't blow away the cache), but we WILL prune on cache eviction. |
nkubala
force-pushed
the
prune
branch
2 times, most recently
from
21e8a59
to
b666c73
Compare
pkg/skaffold/config/options.go
Outdated
| @@ -40,6 +40,8 @@ type SkaffoldOptions struct { | |||
| CacheArtifacts bool | |||
| ExperimentalGUI bool | |||
| EnableRPC bool | |||
| NoPrune bool | |||
| Prune bool // this is a logical combination of NoPrune and CacheArtifacts | |||
There was a problem hiding this comment.
I find this is very confusing - what does it mean that it's a logical combination?
why do we need to store calculations in a "theoretically" read-only struct for user defined opts?
There was a problem hiding this comment.
maybe we could implement an instance function on it?
There was a problem hiding this comment.
by logical combination I mean that opts.Prune == !opts.NoPrune && !opts.CacheArtifacts:
| NoPrune | CacheArtifacts | Prune |
|---|---|---|
| F (default) | F (default) | T (default) |
| F (default) | T | F |
| T | F (default) | F |
| T | T | F |
it's not really storing a calculation, it's just storing a simplified version of it to be used later on. to me this makes more sense than doing opts.Prune(), WDYT?
There was a problem hiding this comment.
I got the simplification - that part is fine - but I think we should introduce the IsPrune() function similar to Labels() instead of storing it in the struct to keep the design consistent i.e. "fields of SkaffoldOptions are user defined flags".
…ect them if they weren't
This code changes the default behavior of the Skaffold runner to remove all intermediate images and containers from the local Docker daemon by default. Users can opt out of this by passing the
--no-pruneflag.The local builder will keep track of all the image IDs it has built over the course of its run, and when the run is ended by the user, use the API client to remove each image. It will also pass the
ForceRemoveoption to the Docker build to remove the intermediate containers from the build, e.g. in multi-stage builds.Fixes #733
cc #1143 and #61