small cleanup + godoc

Signed-off-by: Milas Bowman <[email protected]>

pkg/compose/image_pruner.go

@@ -62,7 +62,6 @@ func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions)
 	} else if opts.Mode != ImagePruneLocal && opts.Mode != ImagePruneAll {
 		return nil, fmt.Errorf("unsupported image prune mode: %s", opts.Mode)
 	}
-
 	var images []string
 
 	if opts.Mode == ImagePruneAll {
@@ -73,28 +72,38 @@ func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions)
 		images = append(images, namedImages...)
 	}
 
-	projectImages, err := p.builtImagesForProject(ctx)
+	projectImages, err := p.labeledLocalImages(ctx)
 	if err != nil {
 		return nil, err
 	}
-
 	for _, img := range projectImages {
 		if len(img.RepoTags) == 0 {
-			// currently, we're only removing the tagged references, but
+			// currently, we're only pruning the tagged references, but
 			// if we start removing the dangling images and grouping by
 			// service, we can remove this (and should rely on `Image::ID`)
 			continue
 		}
 
-		removeImage := opts.RemoveOrphans
-		if !removeImage {
+		var shouldPrune bool
+		if opts.RemoveOrphans {
+			// indiscriminately prune all project images even if they're not
+			// referenced by the current Compose state (e.g. the service was
+			// removed from YAML)
+			shouldPrune = true
+		} else {
+			// only prune the image if it belongs to a known service for the
+			// project AND is either an implicitly-named, locally-built image
+			// or `--rmi=all` has been specified.
+			// TODO(milas): now that Compose labels the images it builds, this
+			// makes less sense; arguably, locally-built but explicitly-named
+			// images should be removed with `--rmi=local` as well.
 			service, err := p.project.GetService(img.Labels[api.ServiceLabel])
 			if err == nil && (opts.Mode == ImagePruneAll || service.Image == "") {
-				removeImage = true
+				shouldPrune = true
 			}
 		}
 
-		if removeImage {
+		if shouldPrune {
 			images = append(images, img.RepoTags[0])
 		}
 	}
@@ -109,7 +118,28 @@ func (p *ImagePruner) ImagesToPrune(ctx context.Context, opts ImagePruneOptions)
 	return images, nil
 }
 
-func (p *ImagePruner) builtImagesForProject(ctx context.Context) ([]moby.ImageSummary, error) {
+// namedImages are those that are explicitly named in the service config.
+//
+// These could be registry-only images (no local build), hybrid (support build
+// as a fallback if cannot pull), or local-only (image does not exist in a
+// registry).
+func (p *ImagePruner) namedImages(ctx context.Context) ([]string, error) {
+	var images []string
+	for _, service := range p.project.Services {
+		if service.Image == "" {
+			continue
+		}
+		images = append(images, service.Image)
+	}
+	return p.filterImagesByExistence(ctx, images)
+}
+
+// labeledLocalImages are images that were locally-built by a current version of
+// Compose (it did not always label built images).
+//
+// The image name could either have been defined by the user or implicitly
+// created from the project + service name.
+func (p *ImagePruner) labeledLocalImages(ctx context.Context) ([]moby.ImageSummary, error) {
 	imageListOpts := moby.ImageListOptions{
 		Filters: filters.NewArgs(
 			projectFilter(p.project.Name),
@@ -129,17 +159,33 @@ func (p *ImagePruner) builtImagesForProject(ctx context.Context) ([]moby.ImageSu
 	return projectImages, nil
 }
 
-func (p *ImagePruner) namedImages(ctx context.Context) ([]string, error) {
+// unlabeledLocalImages are images that match the implicit naming convention
+// for locally-built images but did not get labeled, presumably because they
+// were produced by an older version of Compose.
+//
+// This is transitional to ensure `down` continues to work as expected on
+// projects built/launched by previous versions of Compose. It can safely
+// be removed after some time.
+func (p *ImagePruner) unlabeledLocalImages(ctx context.Context) ([]string, error) {
 	var images []string
 	for _, service := range p.project.Services {
-		if service.Image == "" {
+		if service.Image != "" {
 			continue
 		}
-		images = append(images, service.Image)
+		img := api.GetImageNameOrDefault(service, p.project.Name)
+		images = append(images, img)
 	}
 	return p.filterImagesByExistence(ctx, images)
 }
 
+// filterImagesByExistence returns the subset of images that exist in the
+// engine store.
+//
+// NOTE: Any transient errors communicating with the API will result in an
+// image being returned as "existing", as this method is exclusively used to
+// find images to remove, so the worst case of being conservative here is an
+// attempt to remove an image that doesn't exist, which will cause a warning
+// but is otherwise harmless.
 func (p *ImagePruner) filterImagesByExistence(ctx context.Context, imageNames []string) ([]string, error) {
 	var mu sync.Mutex
 	var ret []string
@@ -168,18 +214,7 @@ func (p *ImagePruner) filterImagesByExistence(ctx context.Context, imageNames []
 	return ret, nil
 }
 
-func (p *ImagePruner) unlabeledLocalImages(ctx context.Context) ([]string, error) {
-	var images []string
-	for _, service := range p.project.Services {
-		if service.Image != "" {
-			continue
-		}
-		img := api.GetImageNameOrDefault(service, p.project.Name)
-		images = append(images, img)
-	}
-	return p.filterImagesByExistence(ctx, images)
-}
-
+// normalizeAndDedupeImages returns the unique set of images after normalization.
 func normalizeAndDedupeImages(images []string) []string {
 	seen := make(map[string]struct{}, len(images))
 	for _, img := range images {
@@ -197,6 +232,7 @@ func normalizeAndDedupeImages(images []string) []string {
 	for v := range seen {
 		ret = append(ret, v)
 	}
+	// ensure a deterministic return result - the actual ordering is not useful
 	sort.Strings(ret)
 	return ret
 }

pkg/e2e/fixtures/build-dependencies/compose.yaml

@@ -10,5 +10,3 @@ services:
     build:
       context: .
       dockerfile: service.dockerfile
-  nginx:
-    image: nginx