From 0cb12ce81fd2dc037c6367746904eff0f778f063 Mon Sep 17 00:00:00 2001 From: Tad Cordle Date: Tue, 25 Jun 2019 13:51:13 -0400 Subject: [PATCH] Refactor skaffold init for more flexible builder detection (#2274) --- pkg/skaffold/docker/docker_init.go | 99 ++++++++++ pkg/skaffold/docker/docker_init_test.go | 135 ++++++++++++++ pkg/skaffold/docker/validate.go | 50 ----- pkg/skaffold/docker/validate_test.go | 66 ------- pkg/skaffold/initializer/init.go | 234 ++++++++++++++++-------- pkg/skaffold/initializer/init_test.go | 105 +++++++++-- 6 files changed, 476 insertions(+), 213 deletions(-) create mode 100644 pkg/skaffold/docker/docker_init.go create mode 100644 pkg/skaffold/docker/docker_init_test.go delete mode 100644 pkg/skaffold/docker/validate.go delete mode 100644 pkg/skaffold/docker/validate_test.go diff --git a/pkg/skaffold/docker/docker_init.go b/pkg/skaffold/docker/docker_init.go new file mode 100644 index 00000000000..f93f5eb9122 --- /dev/null +++ b/pkg/skaffold/docker/docker_init.go @@ -0,0 +1,99 @@ +/* +Copyright 2019 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package docker + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" + "github.com/moby/buildkit/frontend/dockerfile/command" + "github.com/moby/buildkit/frontend/dockerfile/parser" + "github.com/sirupsen/logrus" +) + +// For testing +var ( + ValidateDockerfileFunc = ValidateDockerfile +) + +// Docker is the path to a dockerfile. Implements the InitBuilder interface. +type Docker string + +// Name returns the name of the builder, "Docker" +func (d Docker) Name() string { + return "Docker" +} + +// Describe returns the initBuilder's string representation, used when prompting the user to choose a builder. +func (d Docker) Describe() string { + return fmt.Sprintf("%s (%s)", d.Name(), d) +} + +// CreateArtifact creates an Artifact to be included in the generated Build Config +func (d Docker) CreateArtifact(manifestImage string) *latest.Artifact { + path := string(d) + workspace := filepath.Dir(path) + a := &latest.Artifact{ImageName: manifestImage} + if workspace != "." { + a.Workspace = workspace + } + if filepath.Base(path) != constants.DefaultDockerfilePath { + a.ArtifactType = latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{DockerfilePath: path}, + } + } + + return a +} + +// ConfiguredImage returns the target image configured by the builder, or an empty string if no image is configured +func (d Docker) ConfiguredImage() string { + // Target image is not configured in dockerfiles + return "" +} + +// Path returns the path to the dockerfile +func (d Docker) Path() string { + return string(d) +} + +// ValidateDockerfile makes sure the given Dockerfile is existing and valid. +func ValidateDockerfile(path string) bool { + f, err := os.Open(path) + if err != nil { + logrus.Warnf("opening file %s: %s", path, err.Error()) + return false + } + + res, err := parser.Parse(f) + if err != nil || res == nil || len(res.AST.Children) == 0 { + return false + } + + // validate each node contains valid dockerfile directive + for _, child := range res.AST.Children { + _, ok := command.Commands[child.Value] + if !ok { + return false + } + } + + return true +} diff --git a/pkg/skaffold/docker/docker_init_test.go b/pkg/skaffold/docker/docker_init_test.go new file mode 100644 index 00000000000..c4578cc46db --- /dev/null +++ b/pkg/skaffold/docker/docker_init_test.go @@ -0,0 +1,135 @@ +/* +Copyright 2019 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package docker + +import ( + "path/filepath" + "testing" + + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/testutil" +) + +func TestValidateDockerfile(t *testing.T) { + var tests = []struct { + description string + content string + fileToValidate string + expectedValid bool + }{ + { + description: "valid", + content: "FROM scratch", + fileToValidate: "Dockerfile", + expectedValid: true, + }, + { + description: "invalid command", + content: "GARBAGE", + fileToValidate: "Dockerfile", + expectedValid: false, + }, + { + description: "not found", + fileToValidate: "Unknown", + expectedValid: false, + }, + { + description: "invalid file", + content: "#escape", + fileToValidate: "Dockerfile", + expectedValid: false, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + tmpDir := t.NewTempDir(). + Write("Dockerfile", test.content) + + valid := ValidateDockerfile(tmpDir.Path(test.fileToValidate)) + + t.CheckDeepEqual(test.expectedValid, valid) + }) + } +} + +func TestDescribe(t *testing.T) { + var tests = []struct { + description string + dockerfile Docker + expectedPrompt string + }{ + { + description: "Dockerfile prompt", + dockerfile: Docker("path/to/Dockerfile"), + expectedPrompt: "Docker (path/to/Dockerfile)", + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + t.CheckDeepEqual(test.expectedPrompt, test.dockerfile.Describe()) + }) + } +} + +func TestCreateArtifact(t *testing.T) { + var tests = []struct { + description string + dockerfile Docker + manifestImage string + expectedArtifact latest.Artifact + expectedImage string + }{ + { + description: "default filename", + dockerfile: Docker(filepath.Join("path", "to", "Dockerfile")), + manifestImage: "image", + expectedArtifact: latest.Artifact{ + ImageName: "image", + Workspace: filepath.Join("path", "to"), + ArtifactType: latest.ArtifactType{}, + }, + }, + { + description: "non-default filename", + dockerfile: Docker(filepath.Join("path", "to", "Dockerfile1")), + manifestImage: "image", + expectedArtifact: latest.Artifact{ + ImageName: "image", + Workspace: filepath.Join("path", "to"), + ArtifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{DockerfilePath: filepath.Join("path", "to", "Dockerfile1")}, + }, + }, + }, + { + description: "ignore workspace", + dockerfile: Docker("Dockerfile"), + manifestImage: "image", + expectedArtifact: latest.Artifact{ + ImageName: "image", + ArtifactType: latest.ArtifactType{}, + }, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + artifact := test.dockerfile.CreateArtifact(test.manifestImage) + t.CheckDeepEqual(test.expectedArtifact, *artifact) + }) + } +} diff --git a/pkg/skaffold/docker/validate.go b/pkg/skaffold/docker/validate.go deleted file mode 100644 index 039ae8561e3..00000000000 --- a/pkg/skaffold/docker/validate.go +++ /dev/null @@ -1,50 +0,0 @@ -/* -Copyright 2019 The Skaffold Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package docker - -import ( - "os" - - "github.com/moby/buildkit/frontend/dockerfile/command" - "github.com/moby/buildkit/frontend/dockerfile/parser" - "github.com/sirupsen/logrus" -) - -// ValidateDockerfile makes sure the given Dockerfile is -// existing and valid. -func ValidateDockerfile(path string) bool { - f, err := os.Open(path) - if err != nil { - logrus.Warnf("opening file %s: %s", path, err.Error()) - return false - } - - res, err := parser.Parse(f) - if err != nil || res == nil || len(res.AST.Children) == 0 { - return false - } - - // validate each node contains valid dockerfile directive - for _, child := range res.AST.Children { - _, ok := command.Commands[child.Value] - if !ok { - return false - } - } - - return true -} diff --git a/pkg/skaffold/docker/validate_test.go b/pkg/skaffold/docker/validate_test.go deleted file mode 100644 index c4637780801..00000000000 --- a/pkg/skaffold/docker/validate_test.go +++ /dev/null @@ -1,66 +0,0 @@ -/* -Copyright 2019 The Skaffold Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package docker - -import ( - "testing" - - "github.com/GoogleContainerTools/skaffold/testutil" -) - -func TestValidateDockerfile(t *testing.T) { - var tests = []struct { - description string - content string - fileToValidate string - expectedValid bool - }{ - { - description: "valid", - content: "FROM scratch", - fileToValidate: "Dockerfile", - expectedValid: true, - }, - { - description: "invalid command", - content: "GARBAGE", - fileToValidate: "Dockerfile", - expectedValid: false, - }, - { - description: "not found", - fileToValidate: "Unknown", - expectedValid: false, - }, - { - description: "invalid file", - content: "#escape", - fileToValidate: "Dockerfile", - expectedValid: false, - }, - } - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - tmpDir := t.NewTempDir(). - Write("Dockerfile", test.content) - - valid := ValidateDockerfile(tmpDir.Path(test.fileToValidate)) - - t.CheckDeepEqual(test.expectedValid, valid) - }) - } -} diff --git a/pkg/skaffold/initializer/init.go b/pkg/skaffold/initializer/init.go index da108a9b4ff..e8e94d117b6 100644 --- a/pkg/skaffold/initializer/init.go +++ b/pkg/skaffold/initializer/init.go @@ -25,11 +25,11 @@ import ( "os" "os/exec" "path/filepath" + "sort" "strings" "github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/tips" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/kubectl" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/defaults" @@ -41,9 +41,14 @@ import ( yaml "gopkg.in/yaml.v2" ) -// NoDockerfile allows users to specify they don't want to build +// For testing +var ( + promptUserForBuildConfigFunc = promptUserForBuildConfig +) + +// NoBuilder allows users to specify they don't want to build // an image we parse out from a kubernetes manifest -const NoDockerfile = "None (image not built from these sources)" +const NoBuilder = "None (image not built from these sources)" // Initializer is the Init API of skaffold and responsible for generating // skaffold configuration file. @@ -54,6 +59,22 @@ type Initializer interface { GetImages() []string } +// InitBuilder represents a builder that can be chosen by skaffold init. +type InitBuilder interface { + // Name returns the name of the builder + Name() string + // Describe returns the initBuilder's string representation, used when prompting the user to choose a builder. + // Must be unique between artifacts. + Describe() string + // CreateArtifact creates an Artifact to be included in the generated Build Config + CreateArtifact(image string) *latest.Artifact + // ConfiguredImage returns the target image configured by the builder, or an empty string if no image is configured. + // This should be a cheap operation. + ConfiguredImage() string + // Path returns the path to the build file + Path() string +} + // Config defines the Initializer Config for Init API of skaffold. type Config struct { ComposeFile string @@ -64,6 +85,12 @@ type Config struct { Opts *config.SkaffoldOptions } +// builderImagePair defines a builder and the image it builds +type builderImagePair struct { + Builder InitBuilder + ImageName string +} + // DoInit executes the `skaffold init` flow. func DoInit(out io.Writer, c Config) error { rootDir := "." @@ -77,7 +104,7 @@ func DoInit(out io.Writer, c Config) error { } } - potentialConfigs, dockerfiles, err := walk(rootDir, c.Force, docker.ValidateDockerfile) + potentialConfigs, builderConfigs, err := walk(rootDir, c.Force, detectBuilders) if err != nil { return err } @@ -88,22 +115,27 @@ func DoInit(out io.Writer, c Config) error { } images := k.GetImages() if c.Analyze { - return printAnalyzeJSON(out, c.SkipBuild, dockerfiles, images) + return printAnalyzeJSON(out, c.SkipBuild, builderConfigs, images) } - var pairs []dockerfilePair + // conditionally generate build artifacts + var pairs []builderImagePair if !c.SkipBuild { - if len(dockerfiles) == 0 { - return errors.New("one or more valid Dockerfiles must be present to build images with skaffold; please provide at least one Dockerfile and try again or run `skaffold init --skip-build`") + if len(builderConfigs) == 0 { + return errors.New("one or more valid Dockerfiles must be present to build images with skaffold; please provide at least Dockerfile and try again or run `skaffold init --skip-build`") } + var unresolvedImages []string + pairs, builderConfigs, unresolvedImages = autoSelectBuilders(builderConfigs, images) + if c.CliArtifacts != nil { - pairs, err = processCliArtifacts(c.CliArtifacts) + newPairs, err := processCliArtifacts(c.CliArtifacts) if err != nil { return errors.Wrap(err, "processing cli artifacts") } + pairs = append(pairs, newPairs...) } else { - pairs = resolveDockerfileImages(dockerfiles, images) + pairs = append(pairs, resolveBuilderImages(builderConfigs, unresolvedImages)...) } } @@ -150,94 +182,136 @@ func DoInit(out io.Writer, c Config) error { return nil } -func processCliArtifacts(artifacts []string) ([]dockerfilePair, error) { - var pairs []dockerfilePair +// autoSelectBuilders 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 autoSelectBuilders(builderConfigs []InitBuilder, images []string) ([]builderImagePair, []InitBuilder, []string) { + var pairs []builderImagePair + var unresolvedImages []string + for _, image := range images { + matchingConfigIndex := -1 + for i, config := range builderConfigs { + if image != config.ConfiguredImage() { + continue + } + + // Found more than one match; can't auto-select. + if matchingConfigIndex != -1 { + matchingConfigIndex = -1 + break + } + matchingConfigIndex = i + } + + if matchingConfigIndex != -1 { + // Exactly one pair found; save the pair and remove from remaining build configs + pairs = append(pairs, builderImagePair{ImageName: image, Builder: builderConfigs[matchingConfigIndex]}) + builderConfigs = append(builderConfigs[:matchingConfigIndex], builderConfigs[matchingConfigIndex+1:]...) + } else { + // No definite pair found, add to images list + unresolvedImages = append(unresolvedImages, image) + } + } + return pairs, builderConfigs, unresolvedImages +} + +func detectBuilders(path string) ([]InitBuilder, error) { + // Check for Dockerfile + if docker.ValidateDockerfileFunc(path) { + results := []InitBuilder{docker.Docker(path)} + return results, nil + } + + // TODO: Check for more builders + + return nil, nil +} + +func processCliArtifacts(artifacts []string) ([]builderImagePair, error) { + var pairs []builderImagePair for _, artifact := range artifacts { parts := strings.Split(artifact, "=") if len(parts) != 2 { return nil, fmt.Errorf("malformed artifact provided: %s", artifact) } - pairs = append(pairs, dockerfilePair{ - Dockerfile: parts[0], - ImageName: parts[1], + + pairs = append(pairs, builderImagePair{ + Builder: docker.Docker(parts[0]), + ImageName: parts[1], }) } return pairs, nil } -// For each image parsed from all k8s manifests, prompt the user for -// the dockerfile that builds the referenced image -func resolveDockerfileImages(dockerfiles []string, images []string) []dockerfilePair { - // if we only have 1 image and 1 dockerfile, don't bother prompting - if len(images) == 1 && len(dockerfiles) == 1 { - return []dockerfilePair{{ - Dockerfile: dockerfiles[0], - ImageName: images[0], +// 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) []builderImagePair { + // If nothing to choose, don't bother prompting + if len(images) == 0 || len(builderConfigs) == 0 { + return []builderImagePair{} + } + + // 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], }} } - pairs := []dockerfilePair{} + + // 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] - pair := promptUserForDockerfile(image, dockerfiles) - if pair.Dockerfile != NoDockerfile { - pairs = append(pairs, pair) - dockerfiles = util.RemoveFromSlice(dockerfiles, pair.Dockerfile) + choice := promptUserForBuildConfigFunc(image, choices) + if choice != NoBuilder { + pairs = append(pairs, builderImagePair{Builder: choiceMap[choice], ImageName: image}) + choices = util.RemoveFromSlice(choices, choice) } - images = util.RemoveFromSlice(images, pair.ImageName) + images = util.RemoveFromSlice(images, image) } - if len(dockerfiles) > 0 { - logrus.Warnf("unused dockerfiles found in repository: %v", dockerfiles) + if len(builderConfigs) > 0 { + logrus.Warnf("unused builder configs found in repository: %v", builderConfigs) } return pairs } -func promptUserForDockerfile(image string, dockerfiles []string) dockerfilePair { - var selectedDockerfile string - options := append(dockerfiles, NoDockerfile) +func promptUserForBuildConfig(image string, choices []string) string { + var selectedBuildConfig string + options := append(choices, NoBuilder) prompt := &survey.Select{ - Message: fmt.Sprintf("Choose the dockerfile to build image %s", image), + Message: fmt.Sprintf("Choose the builder to build image %s", image), Options: options, PageSize: 15, } - survey.AskOne(prompt, &selectedDockerfile, nil) - return dockerfilePair{ - Dockerfile: selectedDockerfile, - ImageName: image, - } + survey.AskOne(prompt, &selectedBuildConfig, nil) + return selectedBuildConfig } -func processBuildArtifacts(pairs []dockerfilePair) latest.BuildConfig { +func processBuildArtifacts(pairs []builderImagePair) latest.BuildConfig { var config latest.BuildConfig - if len(pairs) > 0 { - var artifacts []*latest.Artifact - for _, pair := range pairs { - workspace := filepath.Dir(pair.Dockerfile) - dockerfilePath := filepath.Base(pair.Dockerfile) - a := &latest.Artifact{ - ImageName: pair.ImageName, - } - if workspace != "." { - a.Workspace = workspace - } - if dockerfilePath != constants.DefaultDockerfilePath { - a.ArtifactType = latest.ArtifactType{ - DockerArtifact: &latest.DockerArtifact{ - DockerfilePath: dockerfilePath, - }, - } - } - artifacts = append(artifacts, a) + config.Artifacts = make([]*latest.Artifact, len(pairs)) + for i, pair := range pairs { + config.Artifacts[i] = pair.Builder.CreateArtifact(pair.ImageName) } - config.Artifacts = artifacts } return config } -func generateSkaffoldConfig(k Initializer, dockerfilePairs []dockerfilePair) ([]byte, error) { +func generateSkaffoldConfig(k Initializer, buildConfigPairs []builderImagePair) ([]byte, error) { // if we're here, the user has no skaffold yaml so we need to generate one // if the user doesn't have any k8s yamls, generate one for each dockerfile logrus.Info("generating skaffold config") @@ -250,7 +324,7 @@ func generateSkaffoldConfig(k Initializer, dockerfilePairs []dockerfilePair) ([] return nil, errors.Wrap(err, "generating default pipeline") } - cfg.Build = processBuildArtifacts(dockerfilePairs) + cfg.Build = processBuildArtifacts(buildConfigPairs) cfg.Deploy = k.GenerateDeployConfig() pipelineStr, err := yaml.Marshal(cfg) @@ -261,7 +335,8 @@ func generateSkaffoldConfig(k Initializer, dockerfilePairs []dockerfilePair) ([] return pipelineStr, nil } -func printAnalyzeJSON(out io.Writer, skipBuild bool, dockerfiles, images []string) error { +// TODO: make more flexible for non-docker builders +func printAnalyzeJSON(out io.Writer, skipBuild bool, dockerfiles []InitBuilder, images []string) error { if !skipBuild && len(dockerfiles) == 0 { return errors.New("one or more valid Dockerfiles must be present to build images with skaffold; please provide at least one Dockerfile and try again or run `skaffold init --skip-build`") } @@ -269,9 +344,13 @@ func printAnalyzeJSON(out io.Writer, skipBuild bool, dockerfiles, images []strin Dockerfiles []string `json:"dockerfiles,omitempty"` Images []string `json:"images,omitempty"` }{ - Dockerfiles: dockerfiles, - Images: images, + Images: images, + } + a.Dockerfiles = make([]string, len(dockerfiles)) + for i, dockerfile := range dockerfiles { + a.Dockerfiles[i] = dockerfile.Path() } + contents, err := json.Marshal(a) if err != nil { return errors.Wrap(err, "marshalling contents") @@ -280,13 +359,9 @@ func printAnalyzeJSON(out io.Writer, skipBuild bool, dockerfiles, images []strin return err } -type dockerfilePair struct { - Dockerfile string - ImageName string -} - -func walk(dir string, force bool, validateDockerfile func(string) bool) ([]string, []string, error) { - var dockerfiles, potentialConfigs []string +func walk(dir string, force bool, validateBuildFile func(string) ([]InitBuilder, error)) ([]string, []InitBuilder, error) { + var potentialConfigs []string + var foundBuilders []InitBuilder err := filepath.Walk(dir, func(path string, f os.FileInfo, e error) error { if f.IsDir() && util.IsHiddenDir(f.Name()) { logrus.Debugf("skip walking hidden dir %s", f.Name()) @@ -306,15 +381,18 @@ func walk(dir string, force bool, validateDockerfile func(string) bool) ([]strin potentialConfigs = append(potentialConfigs, path) return nil } - // try and parse dockerfile - if validateDockerfile(path) { - logrus.Infof("existing dockerfile found: %s", path) - dockerfiles = append(dockerfiles, path) + // try and parse build file + if builderConfigs, err := validateBuildFile(path); builderConfigs != nil { + for _, buildConfig := range builderConfigs { + logrus.Infof("existing builder found: %s", buildConfig.Describe()) + foundBuilders = append(foundBuilders, buildConfig) + } + return err } return nil }) if err != nil { return nil, nil, err } - return potentialConfigs, dockerfiles, nil + return potentialConfigs, foundBuilders, nil } diff --git a/pkg/skaffold/initializer/init_test.go b/pkg/skaffold/initializer/init_test.go index 2416529e044..02242dd39ec 100644 --- a/pkg/skaffold/initializer/init_test.go +++ b/pkg/skaffold/initializer/init_test.go @@ -21,13 +21,14 @@ import ( "strings" "testing" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/testutil" ) func TestPrintAnalyzeJSON(t *testing.T) { tests := []struct { description string - dockerfiles []string + dockerfiles []InitBuilder images []string skipBuild bool shouldErr bool @@ -35,9 +36,9 @@ func TestPrintAnalyzeJSON(t *testing.T) { }{ { description: "dockerfile and image", - dockerfiles: []string{"Dockerfile", "Dockerfile_2"}, + dockerfiles: []InitBuilder{docker.Docker("Dockerfile1"), docker.Docker("Dockerfile2")}, images: []string{"image1", "image2"}, - expected: "{\"dockerfiles\":[\"Dockerfile\",\"Dockerfile_2\"],\"images\":[\"image1\",\"image2\"]}", + expected: "{\"dockerfiles\":[\"Dockerfile1\",\"Dockerfile2\"],\"images\":[\"image1\",\"image2\"]}", }, { description: "no dockerfile, skip build", @@ -68,15 +69,15 @@ func TestPrintAnalyzeJSON(t *testing.T) { func TestWalk(t *testing.T) { emptyFile := "" tests := []struct { - description string - filesWithContents map[string]string - expectedConfigs []string - expectedDockerfiles []string - force bool - shouldErr bool + description string + filesWithContents map[string]string + expectedConfigs []string + expectedPaths []string + force bool + shouldErr bool }{ { - description: "should return correct k8 configs and dockerfiles", + description: "should return correct k8 configs and build files", filesWithContents: map[string]string{ "config/test.yaml": emptyFile, "k8pod.yml": emptyFile, @@ -89,7 +90,7 @@ func TestWalk(t *testing.T) { "config/test.yaml", "k8pod.yml", }, - expectedDockerfiles: []string{ + expectedPaths: []string{ "Dockerfile", "deploy/Dockerfile", }, @@ -108,7 +109,7 @@ func TestWalk(t *testing.T) { expectedConfigs: []string{ "k8pod.yml", }, - expectedDockerfiles: []string{ + expectedPaths: []string{ "Dockerfile", }, shouldErr: false, @@ -131,7 +132,7 @@ deploy: "config/test.yaml", "k8pod.yml", }, - expectedDockerfiles: []string{ + expectedPaths: []string{ "Dockerfile", "deploy/Dockerfile", }, @@ -150,10 +151,10 @@ kind: Config deploy: kustomize: {}`, }, - force: false, - expectedConfigs: nil, - expectedDockerfiles: nil, - shouldErr: true, + force: false, + expectedConfigs: nil, + expectedPaths: nil, + shouldErr: true, }, } for _, test := range tests { @@ -161,11 +162,16 @@ deploy: tmpDir := t.NewTempDir(). WriteFiles(test.filesWithContents) - potentialConfigs, dockerfiles, err := walk(tmpDir.Root(), test.force, testValidDocker) + t.Override(&docker.ValidateDockerfileFunc, testValidDocker) + + potentialConfigs, builders, err := walk(tmpDir.Root(), test.force, detectBuilders) t.CheckError(test.shouldErr, err) t.CheckDeepEqual(tmpDir.Paths(test.expectedConfigs...), potentialConfigs) - t.CheckDeepEqual(tmpDir.Paths(test.expectedDockerfiles...), dockerfiles) + t.CheckDeepEqual(len(test.expectedPaths), len(builders)) + for i := range builders { + t.CheckDeepEqual(tmpDir.Path(test.expectedPaths[i]), builders[i].Path()) + } }) } } @@ -173,3 +179,64 @@ deploy: func testValidDocker(path string) bool { return strings.HasSuffix(path, "Dockerfile") } + +func TestResolveBuilderImages(t *testing.T) { + tests := []struct { + description string + buildConfigs []InitBuilder + images []string + shouldMakeChoice bool + expectedPairs []builderImagePair + }{ + { + description: "nothing to choose from", + buildConfigs: []InitBuilder{}, + images: []string{}, + shouldMakeChoice: false, + expectedPairs: []builderImagePair{}, + }, + { + description: "don't prompt for single dockerfile and image", + buildConfigs: []InitBuilder{docker.Docker("Dockerfile1")}, + images: []string{"image1"}, + shouldMakeChoice: false, + expectedPairs: []builderImagePair{ + { + Builder: docker.Docker("Dockerfile1"), + ImageName: "image1", + }, + }, + }, + { + description: "prompt for multiple builders and images", + buildConfigs: []InitBuilder{docker.Docker("Dockerfile1"), docker.Docker("Dockerfile2")}, + images: []string{"image1", "image2"}, + shouldMakeChoice: true, + expectedPairs: []builderImagePair{ + { + Builder: docker.Docker("Dockerfile1"), + ImageName: "image1", + }, + { + Builder: docker.Docker("Dockerfile2"), + ImageName: "image2", + }, + }, + }, + } + 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 + t.Override(&promptUserForBuildConfigFunc, func(image string, choices []string) string { + if !test.shouldMakeChoice { + t.FailNow() + } + return choices[0] + }) + + pairs := resolveBuilderImages(test.buildConfigs, test.images) + + t.CheckDeepEqual(test.expectedPairs, pairs) + }) + } +}