Add user-friendly validation of builder/artifact compatibility

pkg/skaffold/build/dependencies.go

@@ -24,6 +24,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/buildpacks"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/custom"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/jib"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
@@ -56,7 +57,7 @@ func DependenciesForArtifact(ctx context.Context, a *latest.Artifact, insecureRe
 		paths, err = buildpacks.GetDependencies(ctx, a.Workspace, a.BuildpackArtifact)
 
 	default:
-		return nil, fmt.Errorf("undefined artifact type: %+v", a.ArtifactType)
+		return nil, fmt.Errorf("undefined type %q for artifact:\n%s", misc.ArtifactType(a), misc.FormatArtifact(a))
 	}
 
 	if err != nil {

pkg/skaffold/build/gcb/spec.go

@@ -17,11 +17,11 @@ limitations under the License.
 package gcb
 
 import (
-	"errors"
 	"fmt"
 
 	cloudbuild "google.golang.org/api/cloudbuild/v1"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 )
 
@@ -52,27 +52,24 @@ func (b *Builder) buildSpec(artifact *latest.Artifact, tag, bucket, object strin
 	return buildSpec, nil
 }
 
-func (b *Builder) buildSpecForArtifact(artifact *latest.Artifact, tag string) (cloudbuild.Build, error) {
+func (b *Builder) buildSpecForArtifact(a *latest.Artifact, tag string) (cloudbuild.Build, error) {
 	switch {
-	case artifact.KanikoArtifact != nil:
-		return b.kanikoBuildSpec(artifact.KanikoArtifact, tag)
+	case a.KanikoArtifact != nil:
+		return b.kanikoBuildSpec(a.KanikoArtifact, tag)
 
-	case artifact.DockerArtifact != nil:
-		return b.dockerBuildSpec(artifact.DockerArtifact, tag)
+	case a.DockerArtifact != nil:
+		return b.dockerBuildSpec(a.DockerArtifact, tag)
 
-	case artifact.JibArtifact != nil:
-		return b.jibBuildSpec(artifact, tag)
+	case a.JibArtifact != nil:
+		return b.jibBuildSpec(a, tag)
 
-	case artifact.BazelArtifact != nil:
-		return cloudbuild.Build{}, errors.New("skaffold can't build a bazel artifact with Google Cloud Build")
+	case a.BazelArtifact != nil, a.CustomArtifact != nil:
+		return cloudbuild.Build{}, fmt.Errorf("Found a '%s' artifact, which is incompatible with the 'gcb' builder:\n\n%s\n\nTo use the '%s' builder, remove the 'googleCloudBuild' stanza from the 'build' section of your configuration. For information, see https://skaffold.dev/docs/pipeline-stages/builders/", misc.ArtifactType(a), misc.FormatArtifact(a), misc.ArtifactType(a))
 
-	case artifact.CustomArtifact != nil:
-		return cloudbuild.Build{}, errors.New("skaffold can't build a custom artifact with Google Cloud Build")
-
-	case artifact.BuildpackArtifact != nil:
-		return b.buildpackBuildSpec(artifact.BuildpackArtifact, tag)
+	case a.BuildpackArtifact != nil:
+		return b.buildpackBuildSpec(a.BuildpackArtifact, tag)
 
 	default:
-		return cloudbuild.Build{}, fmt.Errorf("undefined artifact type: %+v", artifact.ArtifactType)
+		return cloudbuild.Build{}, fmt.Errorf("undefined type %q for artifact:\n%s", misc.ArtifactType(a), misc.FormatArtifact(a))
 	}
 }

pkg/skaffold/build/local/local.go

@@ -28,6 +28,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/buildpacks"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/custom"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/jib"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
@@ -44,16 +45,16 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags,
 	return build.InParallel(ctx, out, tags, artifacts, b.buildArtifact, *b.cfg.Concurrency)
 }
 
-func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) {
-	digestOrImageID, err := b.runBuildForArtifact(ctx, out, artifact, tag)
+func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) {
+	digestOrImageID, err := b.runBuildForArtifact(ctx, out, a, tag)
 	if err != nil {
 		return "", err
 	}
 
 	if b.pushImages {
 		// only track images for pruning when building with docker
 		// if we're pushing a bazel image, it was built directly to the registry
-		if artifact.DockerArtifact != nil {
+		if a.DockerArtifact != nil {
 			imageID, err := b.getImageIDForTag(ctx, tag)
 			if err != nil {
 				logrus.Warnf("unable to inspect image: built images may not be cleaned up correctly by skaffold")
@@ -72,7 +73,7 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *la
 	return build.TagWithImageID(ctx, tag, imageID, b.localDocker)
 }
 
-func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) {
+func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) {
 	if !b.pushImages {
 		// All of the builders will rely on a local Docker:
 		// + Either to build the image,
@@ -84,23 +85,26 @@ func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, artifa
 	}
 
 	switch {
-	case artifact.DockerArtifact != nil:
-		return b.buildDocker(ctx, out, artifact, tag)
+	case a.DockerArtifact != nil:
+		return b.buildDocker(ctx, out, a, tag)
 
-	case artifact.BazelArtifact != nil:
-		return bazel.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages).Build(ctx, out, artifact, tag)
+	case a.BazelArtifact != nil:
+		return bazel.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages).Build(ctx, out, a, tag)
 
-	case artifact.JibArtifact != nil:
-		return jib.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.skipTests).Build(ctx, out, artifact, tag)
+	case a.JibArtifact != nil:
+		return jib.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.skipTests).Build(ctx, out, a, tag)
 
-	case artifact.CustomArtifact != nil:
-		return custom.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.retrieveExtraEnv()).Build(ctx, out, artifact, tag)
+	case a.CustomArtifact != nil:
+		return custom.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.retrieveExtraEnv()).Build(ctx, out, a, tag)
 
-	case artifact.BuildpackArtifact != nil:
-		return buildpacks.NewArtifactBuilder(b.localDocker, b.pushImages, b.devMode).Build(ctx, out, artifact, tag)
+	case a.BuildpackArtifact != nil:
+		return buildpacks.NewArtifactBuilder(b.localDocker, b.pushImages, b.devMode).Build(ctx, out, a, tag)
+
+	case a.KanikoArtifact != nil:
+		return "", fmt.Errorf("Found a '%s' artifact, which is incompatible with the 'local' builder:\n\n%s\n\nTo use the '%s' builder, add the 'cluster' stanza to the 'build' section of your configuration. For information, see https://skaffold.dev/docs/pipeline-stages/builders/", misc.ArtifactType(a), misc.FormatArtifact(a), misc.ArtifactType(a))
 
 	default:
-		return "", fmt.Errorf("undefined artifact type: %+v", artifact.ArtifactType)
+		return "", fmt.Errorf("undefined type %q for artifact:\n%s", misc.ArtifactType(a), misc.FormatArtifact(a))
 	}
 }
 

pkg/skaffold/build/misc/artifact_type.go

@@ -0,0 +1,55 @@
+/*
+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 misc
+
+import (
+	"fmt"
+	"strings"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/yaml"
+)
+
+// ArtifactType returns a string representing the type found in an artifact. Used for error messages.
+// (this would normally be implemented as a String() method on the type, but types are versioned)
+func ArtifactType(a *latest.Artifact) string {
+	switch {
+	case a.DockerArtifact != nil:
+		return "docker"
+	case a.KanikoArtifact != nil:
+		return "kaniko"
+	case a.BazelArtifact != nil:
+		return "bazel"
+	case a.JibArtifact != nil:
+		return "jib"
+	case a.CustomArtifact != nil:
+		return "custom"
+	case a.BuildpackArtifact != nil:
+		return "buildpack"
+	default:
+		return ""
+	}
+}
+
+
+// FormatArtifact returns a string representation of an artifact for usage in error messages
+func FormatArtifact(a *latest.Artifact) string {
+	buf, err := yaml.Marshal(a)
+	if err != nil {
+		return fmt.Sprintf("%+v", a)
+	}
+	return strings.TrimSpace(string(buf))
+}

pkg/skaffold/build/misc/artifact_type_test.go

@@ -0,0 +1,84 @@
+/*
+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 misc
+
+import (
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/testutil"
+)
+
+func TestArtifactType(t *testing.T) {
+	tests := []struct {
+		description string
+		want        string
+		artifact    *latest.Artifact
+	}{
+		{"docker", "docker", &latest.Artifact{
+			ArtifactType: latest.ArtifactType{
+				DockerArtifact: &latest.DockerArtifact{},
+			},
+		}},
+		{"kaniko", "kaniko", &latest.Artifact{
+			ArtifactType: latest.ArtifactType{
+				KanikoArtifact: &latest.KanikoArtifact{},
+			},
+		}},
+		{"docker+kaniko", "docker", &latest.Artifact{
+			ArtifactType: latest.ArtifactType{
+				DockerArtifact: &latest.DockerArtifact{},
+				KanikoArtifact: &latest.KanikoArtifact{},
+			},
+		}},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			got := ArtifactType(test.artifact)
+			if got != test.want {
+				t.Errorf("ArtifactType(%+v) = %q; want %q", test.artifact, got, test.want)
+			}
+		})
+	}
+}
+
+func TestFormatArtifact(t *testing.T) {
+	tests := []struct {
+		description string
+		want        string
+		artifact    *latest.Artifact
+	}{
+		{"docker", "docker: {}", &latest.Artifact{
+			ArtifactType: latest.ArtifactType{
+				DockerArtifact: &latest.DockerArtifact{},
+			},
+		}},
+		{"kaniko", "kaniko: {}", &latest.Artifact{
+			ArtifactType: latest.ArtifactType{
+				KanikoArtifact: &latest.KanikoArtifact{},
+			},
+		}},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			got := FormatArtifact(test.artifact)
+			if got != test.want {
+				t.Errorf("FormatArtifact(%+v) = %q; want %q", test.artifact, got, test.want)
+			}
+		})
+	}
+}