[refactor] make DoInit() a proper controller

pkg/skaffold/initializer/build/builders.go

@@ -17,27 +17,22 @@ limitations under the License.
 package build
 
 import (
-	"encoding/json"
-	"errors"
-	"fmt"
-	"sort"
-	"strings"
-
-	"github.com/sirupsen/logrus"
-
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/buildpacks"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/jib"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/prompt"
+	"io"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings"
 )
 
 // NoBuilder allows users to specify they don't want to build
 // an image we parse out from a Kubernetes manifest
 const NoBuilder = "None (image not built from these sources)"
 
+type Error string
+
+func (e Error) Error() string { return string(e) }
+
+const ErrorNoBuilder = Error("one or more valid builder configuration (Dockerfile or Jib configuration) must be present to build images with skaffold; please provide at least one build config and try again or run `skaffold init --skip-build`")
+
 // InitBuilder represents a builder that can be chosen by skaffold init.
 type InitBuilder interface {
 	// Name returns the name of the builder
@@ -60,183 +55,50 @@ type BuilderImagePair struct {
 	ImageName string
 }
 
-// MatchBuildersToImages takes a list of builders and images, checks if any of the builders' configured target
-// images match an image in the image list, and returns a list of the matching builder/image pairs. Also
-// separately returns the builder configs and images that didn't have any matches.
-func MatchBuildersToImages(builderConfigs []InitBuilder, images []string) ([]BuilderImagePair, []InitBuilder, []string) {
-	var pairs []BuilderImagePair
-	var unresolvedImages = make(sortedSet)
-	for _, image := range images {
-		builderIdx := findExactlyOnceMatchingBuilder(builderConfigs, image)
-
-		// exactly one builder found for the image
-		if builderIdx != -1 {
-			// save the pair
-			pairs = append(pairs, BuilderImagePair{ImageName: image, Builder: builderConfigs[builderIdx]})
-			// remove matched builder from builderConfigs
-			builderConfigs = append(builderConfigs[:builderIdx], builderConfigs[builderIdx+1:]...)
-		} else {
-			// No definite pair found, add to images list
-			unresolvedImages.add(image)
-		}
-	}
-	return pairs, builderConfigs, unresolvedImages.values()
+type Initializer interface {
+	ProcessImages([]string) error
+	BuildConfig() latest.BuildConfig
+	BuilderImagePairs() []BuilderImagePair
+	PrintAnalysis(io.Writer) error
 }
 
-func findExactlyOnceMatchingBuilder(builderConfigs []InitBuilder, image string) int {
-	matchingConfigIndex := -1
-	for i, config := range builderConfigs {
-		if image != config.ConfiguredImage() {
-			continue
-		}
-		// Found more than one match;
-		if matchingConfigIndex != -1 {
-			return -1
-		}
-		matchingConfigIndex = i
-	}
-	return matchingConfigIndex
+type emptyBuildInitializer struct {
 }
 
-// TODO(nkubala): make these private again once DoInit() relinquishes control of the builder/image processing
-func ProcessCliArtifacts(artifacts []string) ([]BuilderImagePair, error) {
-	var pairs []BuilderImagePair
-	for _, artifact := range artifacts {
-		// Parses JSON in the form of: {"builder":"Name of Builder","payload":{...},"image":"image.name"}.
-		// The builder field is parsed first to determine the builder type, and the payload is parsed
-		// afterwards once the type is determined.
-		a := struct {
-			Name  string `json:"builder"`
-			Image string `json:"image"`
-		}{}
-		if err := json.Unmarshal([]byte(artifact), &a); err != nil {
-			// Not JSON, use backwards compatible method
-			parts := strings.Split(artifact, "=")
-			if len(parts) != 2 {
-				return nil, fmt.Errorf("malformed artifact provided: %s", artifact)
-			}
-			pairs = append(pairs, BuilderImagePair{
-				Builder:   docker.ArtifactConfig{File: parts[0]},
-				ImageName: parts[1],
-			})
-			continue
-		}
-
-		// Use builder type to parse payload
-		switch a.Name {
-		case docker.Name:
-			parsed := struct {
-				Payload docker.ArtifactConfig `json:"payload"`
-			}{}
-			if err := json.Unmarshal([]byte(artifact), &parsed); err != nil {
-				return nil, err
-			}
-			pair := BuilderImagePair{Builder: parsed.Payload, ImageName: a.Image}
-			pairs = append(pairs, pair)
-
-		// FIXME: shouldn't use a human-readable name?
-		case jib.PluginName(jib.JibGradle), jib.PluginName(jib.JibMaven):
-			parsed := struct {
-				Payload jib.ArtifactConfig `json:"payload"`
-			}{}
-			if err := json.Unmarshal([]byte(artifact), &parsed); err != nil {
-				return nil, err
-			}
-			parsed.Payload.BuilderName = a.Name
-			pair := BuilderImagePair{Builder: parsed.Payload, ImageName: a.Image}
-			pairs = append(pairs, pair)
-
-		case buildpacks.Name:
-			parsed := struct {
-				Payload buildpacks.ArtifactConfig `json:"payload"`
-			}{}
-			if err := json.Unmarshal([]byte(artifact), &parsed); err != nil {
-				return nil, err
-			}
-			pair := BuilderImagePair{Builder: parsed.Payload, ImageName: a.Image}
-			pairs = append(pairs, pair)
-
-		default:
-			return nil, fmt.Errorf("unknown builder type in CLI artifacts: %q", a.Name)
-		}
-	}
-	return pairs, nil
+func (e *emptyBuildInitializer) ProcessImages([]string) error {
+	return nil
 }
 
-// For each image parsed from all k8s manifests, prompt the user for the builder that builds the referenced image
-func ResolveBuilderImages(builderConfigs []InitBuilder, images []string, force bool) ([]BuilderImagePair, error) {
-	// If nothing to choose, don't bother prompting
-	if len(images) == 0 || len(builderConfigs) == 0 {
-		return []BuilderImagePair{}, nil
-	}
-
-	// if we only have 1 image and 1 build config, don't bother prompting
-	if len(images) == 1 && len(builderConfigs) == 1 {
-		return []BuilderImagePair{{
-			Builder:   builderConfigs[0],
-			ImageName: images[0],
-		}}, nil
-	}
-
-	if force {
-		return nil, errors.New("unable to automatically resolve builder/image pairs; run `skaffold init` without `--force` to manually resolve ambiguities")
-	}
-
-	return resolveBuilderImagesInteractively(builderConfigs, images)
+func (e *emptyBuildInitializer) BuildConfig() latest.BuildConfig {
+	return latest.BuildConfig{}
 }
 
-func resolveBuilderImagesInteractively(builderConfigs []InitBuilder, images []string) ([]BuilderImagePair, error) {
-	// Build map from choice string to builder config struct
-	choices := make([]string, len(builderConfigs))
-	choiceMap := make(map[string]InitBuilder, len(builderConfigs))
-	for i, buildConfig := range builderConfigs {
-		choice := buildConfig.Describe()
-		choices[i] = choice
-		choiceMap[choice] = buildConfig
-	}
-	sort.Strings(choices)
-
-	// For each choice, use prompt string to pair builder config with k8s image
-	pairs := []BuilderImagePair{}
-	for {
-		if len(images) == 0 {
-			break
-		}
-
-		image := images[0]
-		choice, err := prompt.BuildConfigFunc(image, append(choices, NoBuilder))
-		if err != nil {
-			return nil, err
-		}
+func (e *emptyBuildInitializer) BuilderImagePairs() []BuilderImagePair {
+	return nil
+}
 
-		if choice != NoBuilder {
-			pairs = append(pairs, BuilderImagePair{Builder: choiceMap[choice], ImageName: image})
-			choices = util.RemoveFromSlice(choices, choice)
-		}
-		images = util.RemoveFromSlice(images, image)
-	}
-	if len(choices) > 0 {
-		logrus.Warnf("unused builder configs found in repository: %v", choices)
-	}
-	return pairs, nil
+func (e *emptyBuildInitializer) PrintAnalysis(io.Writer) error {
+	return nil
 }
 
-func StripTags(taggedImages []string) []string {
-	// Remove tags from image names
-	var images []string
-	for _, image := range taggedImages {
-		parsed, err := docker.ParseReference(image)
-		if err != nil {
-			// It's possible that it's a templatized name that can't be parsed as is.
-			warnings.Printf("Couldn't parse image [%s]: %s", image, err.Error())
-			continue
+func NewInitializer(builders []InitBuilder, c config.Config) Initializer {
+	switch {
+	case c.SkipBuild:
+		return &emptyBuildInitializer{}
+	case c.CliArtifacts != nil:
+		return &cliBuildInitializer{
+			cliArtifacts:    c.CliArtifacts,
+			builders:        builders,
+			skipBuild:       c.SkipBuild,
+			enableNewFormat: c.EnableNewInitFormat,
 		}
-		if parsed.Digest != "" {
-			warnings.Printf("Ignoring image referenced by digest: [%s]", image)
-			continue
+	default:
+		return &defaultBuildInitializer{
+			builders:        builders,
+			skipBuild:       c.SkipBuild,
+			force:           c.Force,
+			enableNewFormat: c.EnableNewInitFormat,
+			resolveImages:   !c.Analyze,
 		}
-
-		images = append(images, parsed.BaseName)
 	}
-	return images
 }

pkg/skaffold/initializer/build/builders_test.go

@@ -43,7 +43,7 @@ func TestResolveBuilderImages(t *testing.T) {
 			buildConfigs:     []InitBuilder{},
 			images:           []string{},
 			shouldMakeChoice: false,
-			expectedPairs:    []BuilderImagePair{},
+			expectedPairs:    nil,
 		},
 		{
 			description:      "don't prompt for single dockerfile and image",
@@ -97,17 +97,21 @@ func TestResolveBuilderImages(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			// Overrides promptUserForBuildConfig to choose first option rather than using the interactive menu
+			// Overrides prompt.BuildConfigFunc to choose first option rather than using the interactive menu
 			t.Override(&prompt.BuildConfigFunc, func(image string, choices []string) (string, error) {
 				if !test.shouldMakeChoice {
 					t.FailNow()
 				}
 				return choices[0], nil
 			})
 
-			pairs, err := ResolveBuilderImages(test.buildConfigs, test.images, test.force)
-
-			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedPairs, pairs)
+			initializer := &defaultBuildInitializer{
+				builders:         test.buildConfigs,
+				force:            test.force,
+				unresolvedImages: test.images,
+			}
+			err := initializer.resolveBuilderImages()
+			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedPairs, initializer.BuilderImagePairs())
 		})
 	}
 }
@@ -184,7 +188,7 @@ func TestAutoSelectBuilders(t *testing.T) {
 
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			pairs, builderConfigs, unresolvedImages := MatchBuildersToImages(test.builderConfigs, test.images)
+			pairs, builderConfigs, unresolvedImages := matchBuildersToImages(test.builderConfigs, test.images)
 
 			t.CheckDeepEqual(test.expectedPairs, pairs)
 			t.CheckDeepEqual(test.expectedBuildersLeft, builderConfigs)
@@ -258,7 +262,7 @@ func TestProcessCliArtifacts(t *testing.T) {
 
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			pairs, err := ProcessCliArtifacts(test.artifacts)
+			pairs, err := processCliArtifacts(test.artifacts)
 
 			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedPairs, pairs)
 		})
@@ -321,7 +325,7 @@ func TestStripImageTags(t *testing.T) {
 		testutil.Run(t, tc.name, func(t *testutil.T) {
 			fakeWarner := &warnings.Collect{}
 			t.Override(&warnings.Printf, fakeWarner.Warnf)
-			images := StripTags(tc.taggedImages)
+			images := stripTags(tc.taggedImages)
 
 			t.CheckDeepEqual(tc.expectedImages, images)
 			t.CheckDeepEqual(tc.expectedWarnings, fakeWarner.Warnings)