From 710cce5ddf4a7e25468711e870e5bd06b5f6c067 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 25 Jul 2019 09:14:50 +0200 Subject: [PATCH] Support recent maven, for Jib on GCB Fix #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 --- pkg/skaffold/build/gcb/cloud_build.go | 17 ++-- pkg/skaffold/build/gcb/desc.go | 54 ++++++++----- pkg/skaffold/build/gcb/desc_test.go | 54 ++++++++----- pkg/skaffold/build/gcb/docker.go | 13 +++- pkg/skaffold/build/gcb/docker_test.go | 28 +++---- pkg/skaffold/build/gcb/jib.go | 19 +++-- pkg/skaffold/build/gcb/jib_test.go | 108 +++++++++++++++----------- pkg/skaffold/constants/constants.go | 2 +- pkg/skaffold/schema/profiles_test.go | 2 +- pkg/skaffold/schema/versions_test.go | 2 +- 10 files changed, 182 insertions(+), 117 deletions(-) diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index d8b900984f5..ac767839b41 100644 --- 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" @@ -100,7 +101,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer return "", errors.Wrap(err, "uploading source tarball") } - call := cbclient.Projects.Builds.Create(projectID, desc) + call := cbclient.Projects.Builds.Create(projectID, &desc) op, err := call.Context(ctx).Do() if err != nil { return "", errors.Wrap(err, "could not create build") @@ -138,7 +139,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") } @@ -174,11 +175,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 could 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 index d48353cfc4b..b3e8dc16b9e 100644 --- a/pkg/skaffold/build/gcb/desc.go +++ b/pkg/skaffold/build/gcb/desc.go @@ -24,15 +24,8 @@ import ( cloudbuild "google.golang.org/api/cloudbuild/v1" ) -func (b *Builder) buildDescription(artifact *latest.Artifact, tag, bucket, object string) (*cloudbuild.Build, error) { - tags := []string{tag} - - steps, err := b.buildSteps(artifact, tag) - if err != nil { - return nil, err - } - - return &cloudbuild.Build{ +func (b *Builder) baseDescription(bucket, object string) cloudbuild.Build { + return cloudbuild.Build{ LogsBucket: bucket, Source: &cloudbuild.Source{ StorageSource: &cloudbuild.StorageSource{ @@ -40,32 +33,55 @@ func (b *Builder) buildDescription(artifact *latest.Artifact, tag, bucket, objec Object: object, }, }, - Steps: steps, - Images: tags, Options: &cloudbuild.BuildOptions{ DiskSizeGb: b.DiskSizeGb, MachineType: b.MachineType, }, Timeout: b.Timeout, - }, nil + } } -func (b *Builder) buildSteps(artifact *latest.Artifact, tag string) ([]*cloudbuild.BuildStep, error) { +func (b *Builder) buildDescription(artifact *latest.Artifact, tag, bucket, object string) (cloudbuild.Build, error) { + base := b.baseDescription(bucket, object) + switch { case artifact.DockerArtifact != nil: - return b.dockerBuildSteps(artifact.DockerArtifact, tag) + return b.dockerBuildDescription(artifact.DockerArtifact, tag, base) case artifact.BazelArtifact != nil: - return nil, errors.New("skaffold can't build a bazel artifact with Google Cloud Build") + return cloudbuild.Build{}, errors.New("skaffold can't build a bazel artifact with Google Cloud Build") - // TODO: build multiple tagged images with jib in GCB (priyawadhwa@) + // TODO: build multiple tagged images with jib in GCB (priyawadhwa@) case artifact.JibMavenArtifact != nil: - return b.jibMavenBuildSteps(artifact.JibMavenArtifact, tag), nil + return b.jibMavenBuildDescription(artifact.JibMavenArtifact, tag, base), nil case artifact.JibGradleArtifact != nil: - return b.jibGradleBuildSteps(artifact.JibGradleArtifact, tag), nil + return b.jibGradleBuildDescription(artifact.JibGradleArtifact, tag, base), nil default: - return nil, fmt.Errorf("undefined artifact type: %+v", artifact.ArtifactType) + return cloudbuild.Build{}, fmt.Errorf("undefined artifact type: %+v", artifact.ArtifactType) } + + // steps, err := b.buildSteps(artifact, tag) + // if err != nil { + // return nil, err + // } + + // return &cloudbuild.Build{ + // LogsBucket: bucket, + // Source: &cloudbuild.Source{ + // StorageSource: &cloudbuild.StorageSource{ + // Bucket: bucket, + // Object: object, + // }, + // }, + // Steps: steps, + // // Images: []string{tag}, + // Options: &cloudbuild.BuildOptions{ + // DiskSizeGb: b.DiskSizeGb, + // MachineType: b.MachineType, + // }, + // Timeout: b.Timeout, + // Tags: []string{"skaffold"}, + // }, nil } diff --git a/pkg/skaffold/build/gcb/desc_test.go b/pkg/skaffold/build/gcb/desc_test.go index 76c0c660456..1a0cdd421b2 100644 --- a/pkg/skaffold/build/gcb/desc_test.go +++ b/pkg/skaffold/build/gcb/desc_test.go @@ -19,32 +19,50 @@ package gcb import ( "testing" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" + runcontext "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/context" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/testutil" ) -func TestBuildBazelDescriptionFail(t *testing.T) { - artifact := &latest.Artifact{ - ArtifactType: latest.ArtifactType{ - BazelArtifact: &latest.BazelArtifact{}, +func TestBuildDescriptionFail(t *testing.T) { + tests := []struct { + description string + artifact *latest.Artifact + }{ + { + description: "bazel", + artifact: &latest.Artifact{ + ArtifactType: latest.ArtifactType{ + BazelArtifact: &latest.BazelArtifact{}, + }, + }, + }, + { + description: "unknown", + artifact: &latest.Artifact{}, }, } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + builder := newBuilder(latest.GoogleCloudBuild{}) - builder := Builder{ - GoogleCloudBuild: &latest.GoogleCloudBuild{}, - } - _, err := builder.buildDescription(artifact, "tag", "bucket", "object") - - testutil.CheckError(t, true, err) -} + _, err := builder.buildDescription(test.artifact, "tag", "bucket", "object") -func TestBuildUnknownDescriptionFail(t *testing.T) { - artifact := &latest.Artifact{} - - builder := Builder{ - GoogleCloudBuild: &latest.GoogleCloudBuild{}, + t.CheckError(true, err) + }) } - _, err := builder.buildDescription(artifact, "tag", "bucket", "object") +} - testutil.CheckError(t, true, err) +func newBuilder(gcb latest.GoogleCloudBuild) *Builder { + return NewBuilder(&runcontext.RunContext{ + Opts: &config.SkaffoldOptions{}, + Cfg: &latest.Pipeline{ + Build: latest.BuildConfig{ + BuildType: latest.BuildType{ + GoogleCloudBuild: &gcb, + }, + }, + }, + }) } diff --git a/pkg/skaffold/build/gcb/docker.go b/pkg/skaffold/build/gcb/docker.go index bcc0a8ad6c6..e359f60c4c5 100644 --- a/pkg/skaffold/build/gcb/docker.go +++ b/pkg/skaffold/build/gcb/docker.go @@ -25,7 +25,7 @@ import ( cloudbuild "google.golang.org/api/cloudbuild/v1" ) -func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string) ([]*cloudbuild.BuildStep, error) { +func (b *Builder) dockerBuildDescription(artifact *latest.DockerArtifact, tag string, base cloudbuild.Build) (cloudbuild.Build, error) { var steps []*cloudbuild.BuildStep for _, cacheFrom := range artifact.CacheFrom { @@ -40,13 +40,18 @@ func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string) ba, err := docker.GetBuildArgs(artifact) if err != nil { - return nil, errors.Wrap(err, "getting docker build args") + return cloudbuild.Build{}, errors.Wrap(err, "getting docker build args") } args = append(args, ba...) args = append(args, ".") - return append(steps, &cloudbuild.BuildStep{ + steps = append(steps, &cloudbuild.BuildStep{ Name: b.DockerImage, Args: args, - }), nil + }) + + base.Steps = steps + base.Images = []string{tag} + + return base, nil } diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go index cda821328cb..57f505b45e9 100644 --- a/pkg/skaffold/build/gcb/docker_test.go +++ b/pkg/skaffold/build/gcb/docker_test.go @@ -38,14 +38,12 @@ func TestDockerBuildDescription(t *testing.T) { }, } - builder := Builder{ - GoogleCloudBuild: &latest.GoogleCloudBuild{ - DockerImage: "docker/docker", - DiskSizeGb: 100, - MachineType: "n1-standard-1", - Timeout: "10m", - }, - } + builder := newBuilder(latest.GoogleCloudBuild{ + DockerImage: "docker/docker", + DiskSizeGb: 100, + MachineType: "n1-standard-1", + Timeout: "10m", + }) desc, err := builder.buildDescription(artifact, "nginx", "bucket", "object") expected := cloudbuild.Build{ @@ -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.dockerBuildDescription(artifact, "nginx2", cloudbuild.Build{}) 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 index 9ac84d9f7d6..b52cefd515b 100644 --- a/pkg/skaffold/build/gcb/jib.go +++ b/pkg/skaffold/build/gcb/jib.go @@ -22,17 +22,22 @@ import ( 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{{ +func (b *Builder) jibMavenBuildDescription(artifact *latest.JibMavenArtifact, tag string, base cloudbuild.Build) cloudbuild.Build { + base.Steps = []*cloudbuild.BuildStep{{ Name: b.MavenImage, - Args: jib.GenerateMavenArgs("dockerBuild", tag, artifact, b.skipTests), + Args: fixHome(jib.GenerateMavenArgs("build", tag, artifact, b.skipTests)), }} + return base } -func (b *Builder) jibGradleBuildSteps(artifact *latest.JibGradleArtifact, tag string) []*cloudbuild.BuildStep { - return []*cloudbuild.BuildStep{{ +func (b *Builder) jibGradleBuildDescription(artifact *latest.JibGradleArtifact, tag string, base cloudbuild.Build) cloudbuild.Build { + base.Steps = []*cloudbuild.BuildStep{{ Name: b.GradleImage, - Args: jib.GenerateGradleArgs("jibDockerBuild", tag, artifact, b.skipTests), + Args: fixHome(jib.GenerateGradleArgs("jib", tag, artifact, b.skipTests)), }} + return base +} + +func fixHome(args []string) []string { + return append([]string{"-Duser.home=/builder/home"}, args...) } diff --git a/pkg/skaffold/build/gcb/jib_test.go b/pkg/skaffold/build/gcb/jib_test.go index e1bf2d91a4d..020498da9eb 100644 --- a/pkg/skaffold/build/gcb/jib_test.go +++ b/pkg/skaffold/build/gcb/jib_test.go @@ -27,68 +27,88 @@ import ( func TestJibMavenBuildSteps(t *testing.T) { tests := []struct { - skipTests bool - args []string + description string + skipTests bool + expectedArgs []string }{ - {false, []string{"-Djib.console=plain", "jib:_skaffold-fail-if-jib-out-of-date", "-Djib.requiredVersion=" + jib.MinimumJibMavenVersion, "--non-recursive", "prepare-package", "jib:dockerBuild", "-Dimage=img"}}, - {true, []string{"-Djib.console=plain", "jib:_skaffold-fail-if-jib-out-of-date", "-Djib.requiredVersion=" + jib.MinimumJibMavenVersion, "--non-recursive", "-DskipTests=true", "prepare-package", "jib:dockerBuild", "-Dimage=img"}}, + { + description: "skip tests", + skipTests: true, + expectedArgs: []string{"-Duser.home=/builder/home", "-Djib.console=plain", "jib:_skaffold-fail-if-jib-out-of-date", "-Djib.requiredVersion=" + jib.MinimumJibMavenVersion, "--non-recursive", "-DskipTests=true", "prepare-package", "jib:build", "-Dimage=img"}, + }, + { + description: "do not skip tests", + skipTests: false, + expectedArgs: []string{"-Duser.home=/builder/home", "-Djib.console=plain", "jib:_skaffold-fail-if-jib-out-of-date", "-Djib.requiredVersion=" + jib.MinimumJibMavenVersion, "--non-recursive", "prepare-package", "jib:build", "-Dimage=img"}, + }, } for _, test := range tests { - artifact := &latest.Artifact{ - ArtifactType: latest.ArtifactType{ - JibMavenArtifact: &latest.JibMavenArtifact{}, - }, - } - - builder := Builder{ - GoogleCloudBuild: &latest.GoogleCloudBuild{ + testutil.Run(t, test.description, func(t *testutil.T) { + artifact := &latest.Artifact{ + ArtifactType: latest.ArtifactType{ + JibMavenArtifact: &latest.JibMavenArtifact{}, + }, + } + + builder := newBuilder(latest.GoogleCloudBuild{ MavenImage: "maven:3.6.0", - }, - skipTests: test.skipTests, - } + }) + builder.skipTests = test.skipTests - steps, err := builder.buildSteps(artifact, "img") - testutil.CheckError(t, false, err) + desc, err := builder.buildDescription(artifact, "img", "bucket", "object") + t.CheckNoError(err) - expected := []*cloudbuild.BuildStep{{ - Name: "maven:3.6.0", - Args: test.args, - }} + expected := []*cloudbuild.BuildStep{{ + Name: "maven:3.6.0", + Args: test.expectedArgs, + }} - testutil.CheckDeepEqual(t, expected, steps) + t.CheckDeepEqual(expected, desc.Steps) + t.CheckDeepEqual(0, len(desc.Images)) + }) } } func TestJibGradleBuildSteps(t *testing.T) { tests := []struct { - skipTests bool - args []string + description string + skipTests bool + expectedArgs []string }{ - {false, []string{"-Djib.console=plain", "_skaffoldFailIfJibOutOfDate", "-Djib.requiredVersion=" + jib.MinimumJibGradleVersion, ":jibDockerBuild", "--image=img"}}, - {true, []string{"-Djib.console=plain", "_skaffoldFailIfJibOutOfDate", "-Djib.requiredVersion=" + jib.MinimumJibGradleVersion, ":jibDockerBuild", "--image=img", "-x", "test"}}, + { + description: "skip tests", + skipTests: true, + expectedArgs: []string{"-Duser.home=/builder/home", "-Djib.console=plain", "_skaffoldFailIfJibOutOfDate", "-Djib.requiredVersion=" + jib.MinimumJibGradleVersion, ":jib", "--image=img", "-x", "test"}, + }, + { + description: "do not skip tests", + skipTests: false, + expectedArgs: []string{"-Duser.home=/builder/home", "-Djib.console=plain", "_skaffoldFailIfJibOutOfDate", "-Djib.requiredVersion=" + jib.MinimumJibGradleVersion, ":jib", "--image=img"}, + }, } for _, test := range tests { - artifact := &latest.Artifact{ - ArtifactType: latest.ArtifactType{ - JibGradleArtifact: &latest.JibGradleArtifact{}, - }, - } - - builder := Builder{ - GoogleCloudBuild: &latest.GoogleCloudBuild{ + testutil.Run(t, test.description, func(t *testutil.T) { + artifact := &latest.Artifact{ + ArtifactType: latest.ArtifactType{ + JibGradleArtifact: &latest.JibGradleArtifact{}, + }, + } + + builder := newBuilder(latest.GoogleCloudBuild{ GradleImage: "gradle:5.1.1", - }, - skipTests: test.skipTests, - } + }) + builder.skipTests = test.skipTests - steps, err := builder.buildSteps(artifact, "img") - testutil.CheckError(t, false, err) + desc, err := builder.buildDescription(artifact, "img", "bucket", "object") + t.CheckNoError(err) - expected := []*cloudbuild.BuildStep{{ - Name: "gradle:5.1.1", - Args: test.args, - }} + expected := []*cloudbuild.BuildStep{{ + Name: "gradle:5.1.1", + Args: test.expectedArgs, + }} - testutil.CheckDeepEqual(t, expected, steps) + t.CheckDeepEqual(expected, desc.Steps) + t.CheckDeepEqual(0, len(desc.Images)) + }) } } diff --git a/pkg/skaffold/constants/constants.go b/pkg/skaffold/constants/constants.go index c50aa4a2272..14e26d10bad 100644 --- a/pkg/skaffold/constants/constants.go +++ b/pkg/skaffold/constants/constants.go @@ -57,7 +57,7 @@ const ( UpdateCheckEnvironmentVariable = "SKAFFOLD_UPDATE_CHECK" DefaultCloudBuildDockerImage = "gcr.io/cloud-builders/docker" - DefaultCloudBuildMavenImage = "gcr.io/cloud-builders/mvn@sha256:0ec283f2ee1ab1d2ac779dcbb24bddaa46275aec7088cc10f2926b4ea0fcac9b" + DefaultCloudBuildMavenImage = "gcr.io/cloud-builders/mvn" DefaultCloudBuildGradleImage = "gcr.io/cloud-builders/gradle" DefaultSkaffoldDir = ".skaffold" diff --git a/pkg/skaffold/schema/profiles_test.go b/pkg/skaffold/schema/profiles_test.go index 5846a645cff..fb69b72d44b 100644 --- a/pkg/skaffold/schema/profiles_test.go +++ b/pkg/skaffold/schema/profiles_test.go @@ -126,7 +126,7 @@ func TestApplyProfiles(t *testing.T) { GoogleCloudBuild: &latest.GoogleCloudBuild{ ProjectID: "my-project", DockerImage: "gcr.io/cloud-builders/docker", - MavenImage: "gcr.io/cloud-builders/mvn@sha256:0ec283f2ee1ab1d2ac779dcbb24bddaa46275aec7088cc10f2926b4ea0fcac9b", + MavenImage: "gcr.io/cloud-builders/mvn", GradleImage: "gcr.io/cloud-builders/gradle", }, }, diff --git a/pkg/skaffold/schema/versions_test.go b/pkg/skaffold/schema/versions_test.go index 9e33c1bc24c..d66158671e4 100644 --- a/pkg/skaffold/schema/versions_test.go +++ b/pkg/skaffold/schema/versions_test.go @@ -245,7 +245,7 @@ func withGoogleCloudBuild(id string, ops ...func(*latest.BuildConfig)) func(*lat b := latest.BuildConfig{BuildType: latest.BuildType{GoogleCloudBuild: &latest.GoogleCloudBuild{ ProjectID: id, DockerImage: "gcr.io/cloud-builders/docker", - MavenImage: "gcr.io/cloud-builders/mvn@sha256:0ec283f2ee1ab1d2ac779dcbb24bddaa46275aec7088cc10f2926b4ea0fcac9b", + MavenImage: "gcr.io/cloud-builders/mvn", GradleImage: "gcr.io/cloud-builders/gradle", }}} for _, op := range ops {