Support recent maven, for Jib on GCB

Fix GoogleContainerTools#2546 Also took the opportunity to improve tests a bit. Should give builds that are slightly faster since they will by default use a builder image that's already cached on GCB. Signed-off-by: David Gageot <[email protected]>
dgageot · Jul 30, 2019 · e0e3eb8 · e0e3eb8
1 parent bf847ea
commit e0e3eb8
Show file tree

Hide file tree

Showing 16 changed files with 291 additions and 220 deletions.
diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go
@@ -29,6 +29,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/gcp"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/sources"
@@ -79,11 +80,6 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer
 		return "", errors.Wrap(err, "checking bucket is in correct project")
 	}
 
-	desc, err := b.buildDescription(artifact, tag, cbBucket, buildObject)
-	if err != nil {
-		return "", errors.Wrap(err, "could not create build description")
-	}
-
 	dependencies, err := b.DependenciesForArtifact(ctx, artifact)
 	if err != nil {
 		return "", errors.Wrapf(err, "getting dependencies for %s", artifact.ImageName)
@@ -94,7 +90,12 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer
 		return "", errors.Wrap(err, "uploading source tarball")
 	}
 
-	call := cbclient.Projects.Builds.Create(projectID, desc)
+	buildSpec, err := b.buildSpec(artifact, tag, cbBucket, buildObject)
+	if err != nil {
+		return "", errors.Wrap(err, "could not create build description")
+	}
+
+	call := cbclient.Projects.Builds.Create(projectID, &buildSpec)
 	op, err := call.Context(ctx).Do()
 	if err != nil {
 		return "", errors.Wrap(err, "could not create build")
@@ -132,7 +133,7 @@ watch:
 		switch cb.Status {
 		case StatusQueued, StatusWorking, StatusUnknown:
 		case StatusSuccess:
-			digest, err = getDigest(cb)
+			digest, err = getDigest(cb, tag)
 			if err != nil {
 				return "", errors.Wrap(err, "getting image id from finished build")
 			}
@@ -168,11 +169,15 @@ func getBuildID(op *cloudbuild.Operation) (string, error) {
 	return buildMeta.Build.Id, nil
 }
 
-func getDigest(b *cloudbuild.Build) (string, error) {
-	if b.Results == nil || len(b.Results.Images) == 0 {
-		return "", errors.New("build failed")
+func getDigest(b *cloudbuild.Build, defaultToTag string) (string, error) {
+	if b.Results != nil && len(b.Results.Images) == 1 {
+		return b.Results.Images[0].Digest, nil
 	}
-	return b.Results.Images[0].Digest, nil
+
+	// The build steps pushed the image directly like when we use Jib.
+	// Retrieve the digest for that tag.
+	// TODO(dgageot): I don't think GCB can push to an insecure registry.
+	return docker.RemoteDigest(defaultToTag, nil)
 }
 
 func (b *Builder) getLogs(ctx context.Context, offset int64, bucket, objectName string) (io.ReadCloser, error) {

diff --git a/pkg/skaffold/build/gcb/desc.go b/pkg/skaffold/build/gcb/desc.go
diff --git a/pkg/skaffold/build/gcb/desc_test.go b/pkg/skaffold/build/gcb/desc_test.go
diff --git a/pkg/skaffold/build/gcb/docker.go b/pkg/skaffold/build/gcb/docker.go
@@ -25,7 +25,27 @@ import (
 	cloudbuild "google.golang.org/api/cloudbuild/v1"
 )
 
-func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string) ([]*cloudbuild.BuildStep, error) {
+// dockerBuildSpec lists the build steps required to build a docker image.
+func (b *Builder) dockerBuildSpec(artifact *latest.DockerArtifact, tag string) (cloudbuild.Build, error) {
+	args, err := b.dockerBuildArgs(artifact, tag)
+	if err != nil {
+		return cloudbuild.Build{}, err
+	}
+
+	steps := b.cacheFromSteps(artifact)
+	steps = append(steps, &cloudbuild.BuildStep{
+		Name: b.DockerImage,
+		Args: args,
+	})
+
+	return cloudbuild.Build{
+		Steps:  steps,
+		Images: []string{tag},
+	}, nil
+}
+
+// cacheFromSteps pulls images used by `--cache-from`.
+func (b *Builder) cacheFromSteps(artifact *latest.DockerArtifact) []*cloudbuild.BuildStep {
 	var steps []*cloudbuild.BuildStep
 
 	for _, cacheFrom := range artifact.CacheFrom {
@@ -36,17 +56,19 @@ func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string)
 		})
 	}
 
-	args := []string{"build", "--tag", tag, "-f", artifact.DockerfilePath}
+	return steps
+}
 
+// dockerBuildArgs lists the arguments passed to `docker` to build a given image.
+func (b *Builder) dockerBuildArgs(artifact *latest.DockerArtifact, tag string) ([]string, error) {
 	ba, err := docker.GetBuildArgs(artifact)
 	if err != nil {
 		return nil, errors.Wrap(err, "getting docker build args")
 	}
+
+	args := []string{"build", "--tag", tag, "-f", artifact.DockerfilePath}
 	args = append(args, ba...)
 	args = append(args, ".")
 
-	return append(steps, &cloudbuild.BuildStep{
-		Name: b.DockerImage,
-		Args: args,
-	}), nil
+	return args, nil
 }
diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go
@@ -25,7 +25,7 @@ import (
 	cloudbuild "google.golang.org/api/cloudbuild/v1"
 )
 
-func TestDockerBuildDescription(t *testing.T) {
+func TestDockerBuildSpec(t *testing.T) {
 	artifact := &latest.Artifact{
 		ArtifactType: latest.ArtifactType{
 			DockerArtifact: &latest.DockerArtifact{
@@ -38,15 +38,13 @@ func TestDockerBuildDescription(t *testing.T) {
 		},
 	}
 
-	builder := Builder{
-		GoogleCloudBuild: &latest.GoogleCloudBuild{
-			DockerImage: "docker/docker",
-			DiskSizeGb:  100,
-			MachineType: "n1-standard-1",
-			Timeout:     "10m",
-		},
-	}
-	desc, err := builder.buildDescription(artifact, "nginx", "bucket", "object")
+	builder := newBuilder(latest.GoogleCloudBuild{
+		DockerImage: "docker/docker",
+		DiskSizeGb:  100,
+		MachineType: "n1-standard-1",
+		Timeout:     "10m",
+	})
+	desc, err := builder.buildSpec(artifact, "nginx", "bucket", "object")
 
 	expected := cloudbuild.Build{
 		LogsBucket: "bucket",
@@ -68,7 +66,7 @@ func TestDockerBuildDescription(t *testing.T) {
 		Timeout: "10m",
 	}
 
-	testutil.CheckErrorAndDeepEqual(t, false, err, expected, *desc)
+	testutil.CheckErrorAndDeepEqual(t, false, err, expected, desc)
 }
 
 func TestPullCacheFrom(t *testing.T) {
@@ -77,12 +75,10 @@ func TestPullCacheFrom(t *testing.T) {
 		CacheFrom:      []string{"from/image1", "from/image2"},
 	}
 
-	builder := Builder{
-		GoogleCloudBuild: &latest.GoogleCloudBuild{
-			DockerImage: "docker/docker",
-		},
-	}
-	steps, err := builder.dockerBuildSteps(artifact, "nginx2")
+	builder := newBuilder(latest.GoogleCloudBuild{
+		DockerImage: "docker/docker",
+	})
+	desc, err := builder.dockerBuildSpec(artifact, "nginx2")
 
 	expected := []*cloudbuild.BuildStep{{
 		Name:       "docker/docker",
@@ -97,5 +93,5 @@ func TestPullCacheFrom(t *testing.T) {
 		Args: []string{"build", "--tag", "nginx2", "-f", "Dockerfile", "--cache-from", "from/image1", "--cache-from", "from/image2", "."},
 	}}
 
-	testutil.CheckErrorAndDeepEqual(t, false, err, expected, steps)
+	testutil.CheckErrorAndDeepEqual(t, false, err, expected, desc.Steps)
 }
diff --git a/pkg/skaffold/build/gcb/jib.go b/pkg/skaffold/build/gcb/jib.go
@@ -17,22 +17,33 @@ limitations under the License.
 package gcb
 
 import (
+	"strings"
+
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/jib"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	cloudbuild "google.golang.org/api/cloudbuild/v1"
 )
 
-// TODO(dgageot): check that `package` is bound to `jib:build`
-func (b *Builder) jibMavenBuildSteps(artifact *latest.JibMavenArtifact, tag string) []*cloudbuild.BuildStep {
-	return []*cloudbuild.BuildStep{{
-		Name: b.MavenImage,
-		Args: jib.GenerateMavenArgs("dockerBuild", tag, artifact, b.skipTests),
-	}}
+func (b *Builder) jibMavenBuildSpec(artifact *latest.JibMavenArtifact, tag string) cloudbuild.Build {
+	return cloudbuild.Build{
+		Steps: []*cloudbuild.BuildStep{{
+			Name:       b.MavenImage,
+			Entrypoint: "sh",
+			Args:       fixHome("mvn", jib.GenerateMavenArgs("build", tag, artifact, b.skipTests)),
+		}},
+	}
+}
+
+func (b *Builder) jibGradleBuildSpec(artifact *latest.JibGradleArtifact, tag string) cloudbuild.Build {
+	return cloudbuild.Build{
+		Steps: []*cloudbuild.BuildStep{{
+			Name:       b.GradleImage,
+			Entrypoint: "sh",
+			Args:       fixHome("gradle", jib.GenerateGradleArgs("jib", tag, artifact, b.skipTests)),
+		}},
+	}
 }
 
-func (b *Builder) jibGradleBuildSteps(artifact *latest.JibGradleArtifact, tag string) []*cloudbuild.BuildStep {
-	return []*cloudbuild.BuildStep{{
-		Name: b.GradleImage,
-		Args: jib.GenerateGradleArgs("jibDockerBuild", tag, artifact, b.skipTests),
-	}}
+func fixHome(command string, args []string) []string {
+	return []string{"-c", command + " -Duser.home=$$HOME " + strings.Join(args, " ")}
 }