From 6779319a804043bcd49d4bcf56cf289fb33ea6fc Mon Sep 17 00:00:00 2001 From: gsquared94 Date: Mon, 13 Jul 2020 17:27:36 -0700 Subject: [PATCH 1/6] Add `ImageOptions` to artifact builder. --- cmd/skaffold/app/cmd/debug.go | 2 + .../examples/getting-started/Dockerfile | 7 +- integration/testdata/debug/go/Dockerfile | 4 +- integration/testdata/debug/skaffold.yaml | 3 - pkg/skaffold/build/cache/cache.go | 7 +- pkg/skaffold/build/cache/hash.go | 12 ++- pkg/skaffold/build/cache/hash_test.go | 17 ++-- pkg/skaffold/build/cache/lookup.go | 11 +-- pkg/skaffold/build/cache/lookup_test.go | 13 +-- pkg/skaffold/build/cache/retrieve_test.go | 2 +- pkg/skaffold/build/cluster/cluster.go | 6 +- pkg/skaffold/build/gcb/cloud_build.go | 10 +-- pkg/skaffold/build/gcb/docker.go | 12 +-- pkg/skaffold/build/gcb/docker_test.go | 3 +- pkg/skaffold/build/gcb/spec.go | 3 +- pkg/skaffold/build/local/docker.go | 16 ++-- pkg/skaffold/build/local/docker_test.go | 2 +- pkg/skaffold/build/local/local.go | 31 ++++--- pkg/skaffold/build/options.go | 85 +++++++++++++++++++ pkg/skaffold/build/parallel.go | 4 +- pkg/skaffold/build/parallel_test.go | 18 ++-- pkg/skaffold/build/sequence.go | 2 +- pkg/skaffold/build/sequence_test.go | 10 +-- pkg/skaffold/docker/image.go | 21 +++-- pkg/skaffold/docker/image_test.go | 4 +- pkg/skaffold/docker/options.go | 84 ++++++++++++++++++ 26 files changed, 297 insertions(+), 92 deletions(-) create mode 100644 pkg/skaffold/build/options.go create mode 100644 pkg/skaffold/docker/options.go diff --git a/cmd/skaffold/app/cmd/debug.go b/cmd/skaffold/app/cmd/debug.go index 345471178c1..eac133f7ece 100644 --- a/cmd/skaffold/app/cmd/debug.go +++ b/cmd/skaffold/app/cmd/debug.go @@ -20,6 +20,7 @@ import ( "context" "io" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/spf13/cobra" debugging "github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug" @@ -43,6 +44,7 @@ func NewCmdDebug() *cobra.Command { func runDebug(ctx context.Context, out io.Writer) error { opts.PortForward.ForwardPods = true + build.CurrentConfiguration = build.Debug deploy.AddManifestTransform(debugging.ApplyDebuggingTransforms) return doDev(ctx, out) diff --git a/integration/examples/getting-started/Dockerfile b/integration/examples/getting-started/Dockerfile index 1fc7510fdcd..aba3e5560ed 100644 --- a/integration/examples/getting-started/Dockerfile +++ b/integration/examples/getting-started/Dockerfile @@ -1,6 +1,11 @@ FROM golang:1.12.9-alpine3.10 as builder + +# These flag values are provided when building for debug +ARG GO_GCFLAGS +ARG GO_LDFLAGS + COPY main.go . -RUN go build -o /app main.go +RUN eval go build --gcflags="${GO_GCFLAGS}" --ldflags="${GO_LDFLAGS}" -o /app main.go FROM alpine:3.10 # Define GOTRACEBACK to mark this container as using the Go language runtime diff --git a/integration/testdata/debug/go/Dockerfile b/integration/testdata/debug/go/Dockerfile index d1a4d643a1a..19811ce6f1d 100644 --- a/integration/testdata/debug/go/Dockerfile +++ b/integration/testdata/debug/go/Dockerfile @@ -2,8 +2,8 @@ FROM golang:1.12 as builder COPY app.go . # Must use eval to handle GOGCFLAGS with spaces like `-gcflags='all=-N -l'` -ARG GOGCFLAGS -RUN eval go build "${GOGCFLAGS}" -o /app . +ARG GO_GCFLAGS +RUN eval go build "${GO_GCFLAGS}" -o /app . FROM gcr.io/distroless/base # `skaffold debug` uses GOTRACEBACK as an indicator of the Go runtime diff --git a/integration/testdata/debug/skaffold.yaml b/integration/testdata/debug/skaffold.yaml index 90e1c7a64bd..657d9df186f 100644 --- a/integration/testdata/debug/skaffold.yaml +++ b/integration/testdata/debug/skaffold.yaml @@ -15,9 +15,6 @@ build: context: python3 - image: skaffold-debug-go context: go - docker: - buildArgs: - GOGCFLAGS: "-gcflags='all=-N -l'" deploy: kubectl: diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 42973a9f682..08adec2a1a0 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -22,6 +22,7 @@ import ( "io/ioutil" "path/filepath" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" homedir "github.com/mitchellh/go-homedir" "github.com/sirupsen/logrus" @@ -49,7 +50,7 @@ type cache struct { insecureRegistries map[string]bool cacheFile string imagesAreLocal bool - hashForArtifact func(ctx context.Context, a *latest.Artifact) (string, error) + hashForArtifact func(ctx context.Context, a *latest.Artifact, opts *build.ImageOptions) (string, error) } // DependencyLister fetches a list of dependencies for an artifact @@ -84,8 +85,8 @@ func NewCache(runCtx *runcontext.RunContext, imagesAreLocal bool, dependencies D insecureRegistries: runCtx.InsecureRegistries, cacheFile: cacheFile, imagesAreLocal: imagesAreLocal, - hashForArtifact: func(ctx context.Context, a *latest.Artifact) (string, error) { - return getHashForArtifact(ctx, dependencies, a, runCtx.Opts.IsDevMode()) + hashForArtifact: func(ctx context.Context, a *latest.Artifact, opts *build.ImageOptions) (string, error) { + return getHashForArtifact(ctx, dependencies, a, opts, runCtx.Opts.IsDevMode()) }, }, nil } diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index 3fc0661852e..d68f350c905 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -27,6 +27,7 @@ import ( "os" "sort" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/sirupsen/logrus" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc" @@ -40,7 +41,7 @@ var ( artifactConfigFunction = artifactConfig ) -func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *latest.Artifact, devMode bool) (string, error) { +func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *latest.Artifact, opts *build.ImageOptions, devMode bool) (string, error) { var inputs []string // Append the artifact's configuration @@ -89,6 +90,15 @@ func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *late inputs = append(inputs, evaluatedEnv...) } + // add image options hash + if opts != nil { + if h, err := opts.Hash(); err != nil { + return "", fmt.Errorf("evaluating build args: %w", err) + } else { + inputs = append(inputs, h) + } + } + // get a key for the hashes hasher := sha256.New() enc := json.NewEncoder(hasher) diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index e7ea4264e58..168eb0397f0 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -21,6 +21,7 @@ import ( "os" "testing" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" @@ -142,7 +143,7 @@ func TestGetHashForArtifact(t *testing.T) { t.Override(&artifactConfigFunction, fakeArtifactConfig) depLister := stubDependencyLister(test.dependencies) - actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, test.devMode) + actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, build.CreateBuilderOptions(""), test.devMode) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -220,21 +221,21 @@ func TestBuildArgs(t *testing.T) { t.Override(&hashFunction, mockCacheHasher) t.Override(&artifactConfigFunction, fakeArtifactConfig) - actual, err := getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, false) + actual, err := getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, nil, false) t.CheckNoError(err) t.CheckDeepEqual(expected, actual) // Change order of buildargs artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"two": stringPointer("2"), "one": stringPointer("1")} - actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, false) + actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, nil, false) t.CheckNoError(err) t.CheckDeepEqual(expected, actual) // Change build args, get different hash artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"one": stringPointer("1")} - actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, false) + actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, nil, false) t.CheckNoError(err) if actual == expected { @@ -263,7 +264,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { t.Override(&artifactConfigFunction, fakeArtifactConfig) depLister := stubDependencyLister([]string{"dep"}) - hash1, err := getHashForArtifact(context.Background(), depLister, artifact, false) + hash1, err := getHashForArtifact(context.Background(), depLister, artifact, nil, false) t.CheckNoError(err) @@ -273,7 +274,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { return []string{"FOO=baz"} } - hash2, err := getHashForArtifact(context.Background(), depLister, artifact, false) + hash2, err := getHashForArtifact(context.Background(), depLister, artifact, nil, false) t.CheckNoError(err) if hash1 == hash2 { @@ -330,7 +331,7 @@ func TestCacheHasher(t *testing.T) { path := originalFile depLister := stubDependencyLister([]string{tmpDir.Path(originalFile)}) - oldHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, false) + oldHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, nil, false) t.CheckNoError(err) test.update(originalFile, tmpDir) @@ -339,7 +340,7 @@ func TestCacheHasher(t *testing.T) { } depLister = stubDependencyLister([]string{tmpDir.Path(path)}) - newHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, false) + newHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, nil, false) t.CheckNoError(err) t.CheckFalse(test.differentHash && oldHash == newHash) diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index abc96c5c715..d1a53b6ec83 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -21,6 +21,7 @@ import ( "fmt" "sync" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -35,7 +36,7 @@ func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, artifac i := i go func() { - details[i] = c.lookup(ctx, artifacts[i], tags[artifacts[i].ImageName]) + details[i] = c.lookup(ctx, artifacts[i], build.CreateBuilderOptions(tags[artifacts[i].ImageName])) wg.Done() }() } @@ -44,8 +45,8 @@ func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, artifac return details } -func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string) cacheDetails { - hash, err := c.hashForArtifact(ctx, a) +func (c *cache) lookup(ctx context.Context, a *latest.Artifact, opts *build.ImageOptions) cacheDetails { + hash, err := c.hashForArtifact(ctx, a, opts) if err != nil { return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)} } @@ -56,9 +57,9 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string) cach } if c.imagesAreLocal { - return c.lookupLocal(ctx, hash, tag, entry) + return c.lookupLocal(ctx, hash, opts.Tag, entry) } - return c.lookupRemote(ctx, hash, tag, entry) + return c.lookupRemote(ctx, hash, opts.Tag, entry) } func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDetails) cacheDetails { diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go index ea20f8c7942..83eb095906a 100644 --- a/pkg/skaffold/build/cache/lookup_test.go +++ b/pkg/skaffold/build/cache/lookup_test.go @@ -22,6 +22,7 @@ import ( "reflect" "testing" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/testutil" @@ -30,7 +31,7 @@ import ( func TestLookupLocal(t *testing.T) { tests := []struct { description string - hasher func(context.Context, *latest.Artifact) (string, error) + hasher func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) cache map[string]ImageDetails api *testutil.FakeAPIClient expected cacheDetails @@ -124,7 +125,7 @@ func TestLookupLocal(t *testing.T) { func TestLookupRemote(t *testing.T) { tests := []struct { description string - hasher func(context.Context, *latest.Artifact) (string, error) + hasher func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) cache map[string]ImageDetails api *testutil.FakeAPIClient expected cacheDetails @@ -205,14 +206,14 @@ func TestLookupRemote(t *testing.T) { } } -func mockHasher(value string) func(context.Context, *latest.Artifact) (string, error) { - return func(context.Context, *latest.Artifact) (string, error) { +func mockHasher(value string) func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { + return func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { return value, nil } } -func failingHasher(errMessage string) func(context.Context, *latest.Artifact) (string, error) { - return func(context.Context, *latest.Artifact) (string, error) { +func failingHasher(errMessage string) func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { + return func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { return "", errors.New(errMessage) } } diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index b40cd2a7baa..9c37974c98a 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -57,7 +57,7 @@ func (b *mockBuilder) BuildAndTest(ctx context.Context, out io.Writer, tags tag. b.built = append(b.built, artifact) tag := tags[artifact.ImageName] - _, err := b.dockerDaemon.Build(ctx, out, artifact.Workspace, artifact.DockerArtifact, tag) + _, err := b.dockerDaemon.Build(ctx, out, artifact.Workspace, artifact.DockerArtifact, &docker.BuildOptions{Tag: tag}) if err != nil { return nil, err } diff --git a/pkg/skaffold/build/cluster/cluster.go b/pkg/skaffold/build/cluster/cluster.go index 0f38ba38df4..cc867032657 100644 --- a/pkg/skaffold/build/cluster/cluster.go +++ b/pkg/skaffold/build/cluster/cluster.go @@ -48,13 +48,13 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, return build.InParallel(ctx, out, tags, artifacts, b.buildArtifact, b.ClusterDetails.Concurrency) } -func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { - digest, err := b.runBuildForArtifact(ctx, out, artifact, tag) +func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *build.ImageOptions) (string, error) { + digest, err := b.runBuildForArtifact(ctx, out, artifact, opts.Tag) if err != nil { return "", err } - return build.TagWithDigest(tag, digest), nil + return build.TagWithDigest(opts.Tag, digest), nil } func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) { diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 28d04e9f8f8..9bed7230c41 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -49,7 +49,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, return build.InParallel(ctx, out, tags, artifacts, b.buildArtifactWithCloudBuild, b.GoogleCloudBuild.Concurrency) } -func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { +func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *build.ImageOptions) (string, error) { cbclient, err := cloudbuild.NewService(ctx, gcp.ClientOptions()...) if err != nil { return "", fmt.Errorf("getting cloudbuild client: %w", err) @@ -63,7 +63,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer projectID := b.ProjectID if projectID == "" { - guessedProjectID, err := gcp.ExtractProjectID(tag) + guessedProjectID, err := gcp.ExtractProjectID(opts.Tag) if err != nil { return "", fmt.Errorf("extracting projectID from image name: %w", err) } @@ -103,7 +103,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer return "", fmt.Errorf("uploading source tarball: %w", err) } - buildSpec, err := b.buildSpec(artifact, tag, cbBucket, buildObject) + buildSpec, err := b.buildSpec(artifact, opts.Tag, cbBucket, buildObject) if err != nil { return "", fmt.Errorf("could not create build description: %w", err) } @@ -165,7 +165,7 @@ watch: switch cb.Status { case StatusQueued, StatusWorking, StatusUnknown: case StatusSuccess: - digest, err = getDigest(cb, tag) + digest, err = getDigest(cb, opts.Tag) if err != nil { return "", fmt.Errorf("getting image id from finished build: %w", err) } @@ -184,7 +184,7 @@ watch: } logrus.Infof("Deleted object %s", buildObject) - return build.TagWithDigest(tag, digest), nil + return build.TagWithDigest(opts.Tag, digest), nil } func getBuildID(op *cloudbuild.Operation) (string, error) { diff --git a/pkg/skaffold/build/gcb/docker.go b/pkg/skaffold/build/gcb/docker.go index d41685f019f..1dbe953d146 100644 --- a/pkg/skaffold/build/gcb/docker.go +++ b/pkg/skaffold/build/gcb/docker.go @@ -26,8 +26,8 @@ import ( ) // 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) +func (b *Builder) dockerBuildSpec(artifact *latest.DockerArtifact, opts *docker.BuildOptions) (cloudbuild.Build, error) { + args, err := b.dockerBuildArgs(artifact, opts) if err != nil { return cloudbuild.Build{}, err } @@ -40,7 +40,7 @@ func (b *Builder) dockerBuildSpec(artifact *latest.DockerArtifact, tag string) ( return cloudbuild.Build{ Steps: steps, - Images: []string{tag}, + Images: []string{opts.Tag}, }, nil } @@ -60,13 +60,13 @@ func (b *Builder) cacheFromSteps(artifact *latest.DockerArtifact) []*cloudbuild. } // 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) +func (b *Builder) dockerBuildArgs(artifact *latest.DockerArtifact, opts *docker.BuildOptions) ([]string, error) { + ba, err := docker.GetBuildArgs(artifact, opts) if err != nil { return nil, fmt.Errorf("getting docker build args: %w", err) } - args := []string{"build", "--tag", tag, "-f", artifact.DockerfilePath} + args := []string{"build", "--tag", opts.Tag, "-f", artifact.DockerfilePath} args = append(args, ba...) args = append(args, ".") diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go index 639c7bb0fba..0b09bb2af67 100644 --- a/pkg/skaffold/build/gcb/docker_test.go +++ b/pkg/skaffold/build/gcb/docker_test.go @@ -19,6 +19,7 @@ package gcb import ( "testing" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" cloudbuild "google.golang.org/api/cloudbuild/v1" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -79,7 +80,7 @@ func TestPullCacheFrom(t *testing.T) { builder := newBuilder(latest.GoogleCloudBuild{ DockerImage: "docker/docker", }) - desc, err := builder.dockerBuildSpec(artifact, "nginx2") + desc, err := builder.dockerBuildSpec(artifact, &docker.BuildOptions{Tag: "nginx2"}) expected := []*cloudbuild.BuildStep{{ Name: "docker/docker", diff --git a/pkg/skaffold/build/gcb/spec.go b/pkg/skaffold/build/gcb/spec.go index baf4b9e8fb8..a99fa31d25f 100644 --- a/pkg/skaffold/build/gcb/spec.go +++ b/pkg/skaffold/build/gcb/spec.go @@ -19,6 +19,7 @@ package gcb import ( "fmt" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" cloudbuild "google.golang.org/api/cloudbuild/v1" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc" @@ -58,7 +59,7 @@ func (b *Builder) buildSpecForArtifact(a *latest.Artifact, tag string) (cloudbui return b.kanikoBuildSpec(a.KanikoArtifact, tag) case a.DockerArtifact != nil: - return b.dockerBuildSpec(a.DockerArtifact, tag) + return b.dockerBuildSpec(a.DockerArtifact, &docker.BuildOptions{Tag: tag}) case a.JibArtifact != nil: return b.jibBuildSpec(a, tag) diff --git a/pkg/skaffold/build/local/docker.go b/pkg/skaffold/build/local/docker.go index 94aaad4c4d5..04de3250001 100644 --- a/pkg/skaffold/build/local/docker.go +++ b/pkg/skaffold/build/local/docker.go @@ -29,7 +29,7 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings" ) -func (b *Builder) buildDocker(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) { +func (b *Builder) buildDocker(ctx context.Context, out io.Writer, a *latest.Artifact, opts *docker.BuildOptions) (string, error) { // Fail fast if the Dockerfile can't be found. dockerfile, err := docker.NormalizeDockerfilePath(a.Workspace, a.DockerArtifact.DockerfilePath) if err != nil { @@ -46,9 +46,9 @@ func (b *Builder) buildDocker(ctx context.Context, out io.Writer, a *latest.Arti var imageID string if b.cfg.UseDockerCLI || b.cfg.UseBuildkit { - imageID, err = b.dockerCLIBuild(ctx, out, a.Workspace, a.ArtifactType.DockerArtifact, tag) + imageID, err = b.dockerCLIBuild(ctx, out, a.Workspace, a.ArtifactType.DockerArtifact, opts) } else { - imageID, err = b.localDocker.Build(ctx, out, a.Workspace, a.ArtifactType.DockerArtifact, tag) + imageID, err = b.localDocker.Build(ctx, out, a.Workspace, a.ArtifactType.DockerArtifact, opts) } if err != nil { @@ -56,7 +56,7 @@ func (b *Builder) buildDocker(ctx context.Context, out io.Writer, a *latest.Arti } if b.pushImages { - return b.localDocker.Push(ctx, out, tag) + return b.localDocker.Push(ctx, out, opts.Tag) } return imageID, nil @@ -66,14 +66,14 @@ func (b *Builder) retrieveExtraEnv() []string { return b.localDocker.ExtraEnv() } -func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, tag string) (string, error) { +func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts *docker.BuildOptions) (string, error) { dockerfilePath, err := docker.NormalizeDockerfilePath(workspace, a.DockerfilePath) if err != nil { return "", fmt.Errorf("normalizing dockerfile path: %w", err) } - args := []string{"build", workspace, "--file", dockerfilePath, "-t", tag} - ba, err := docker.GetBuildArgs(a) + args := []string{"build", workspace, "--file", dockerfilePath, "-t", opts.Tag} + ba, err := docker.GetBuildArgs(a, opts) if err != nil { return "", fmt.Errorf("getting docker build args: %w", err) } @@ -95,7 +95,7 @@ func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, workspace s return "", fmt.Errorf("running build: %w", err) } - return b.localDocker.ImageID(ctx, tag) + return b.localDocker.ImageID(ctx, opts.Tag) } func (b *Builder) pullCacheFromImages(ctx context.Context, out io.Writer, a *latest.DockerArtifact) error { diff --git a/pkg/skaffold/build/local/docker_test.go b/pkg/skaffold/build/local/docker_test.go index 8e0f4d1fc36..b5d0fc028e5 100644 --- a/pkg/skaffold/build/local/docker_test.go +++ b/pkg/skaffold/build/local/docker_test.go @@ -93,7 +93,7 @@ func TestDockerCLIBuild(t *testing.T) { }, } - _, err = builder.buildDocker(context.Background(), ioutil.Discard, artifact, "tag") + _, err = builder.buildDocker(context.Background(), ioutil.Discard, artifact, &docker.BuildOptions{Tag: "tag"}) t.CheckNoError(err) }) } diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 114dbbea111..bbeee0be751 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -21,6 +21,7 @@ import ( "fmt" "io" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/sirupsen/logrus" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" @@ -45,8 +46,8 @@ 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, a *latest.Artifact, tag string) (string, error) { - digestOrImageID, err := b.runBuildForArtifact(ctx, out, a, tag) +func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, opts *build.ImageOptions) (string, error) { + digestOrImageID, err := b.runBuildForArtifact(ctx, out, a, opts) if err != nil { return "", err } @@ -55,7 +56,7 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Ar // only track images for pruning when building with docker // if we're pushing a bazel image, it was built directly to the registry if a.DockerArtifact != nil { - imageID, err := b.getImageIDForTag(ctx, tag) + imageID, err := b.getImageIDForTag(ctx, opts.Tag) if err != nil { logrus.Warnf("unable to inspect image: built images may not be cleaned up correctly by skaffold") } @@ -65,15 +66,15 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Ar } digest := digestOrImageID - return build.TagWithDigest(tag, digest), nil + return build.TagWithDigest(opts.Tag, digest), nil } imageID := digestOrImageID b.builtImages = append(b.builtImages, imageID) - return build.TagWithImageID(ctx, tag, imageID, b.localDocker) + return build.TagWithImageID(ctx, opts.Tag, imageID, b.localDocker) } -func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) { +func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, opts *build.ImageOptions) (string, error) { if !b.pushImages { // All of the builders will rely on a local Docker: // + Either to build the image, @@ -86,19 +87,19 @@ func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *lat switch { case a.DockerArtifact != nil: - return b.buildDocker(ctx, out, a, tag) + return b.buildDocker(ctx, out, a, ToDockerOpts(opts)) case a.BazelArtifact != nil: - return bazel.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages).Build(ctx, out, a, tag) + return bazel.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages).Build(ctx, out, a, opts.Tag) case a.JibArtifact != nil: - return jib.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.skipTests).Build(ctx, out, a, tag) + return jib.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.skipTests).Build(ctx, out, a, opts.Tag) case a.CustomArtifact != nil: - return custom.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.retrieveExtraEnv()).Build(ctx, out, a, tag) + return custom.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.retrieveExtraEnv()).Build(ctx, out, a, opts.Tag) case a.BuildpackArtifact != nil: - return buildpacks.NewArtifactBuilder(b.localDocker, b.pushImages, b.devMode).Build(ctx, out, a, tag) + return buildpacks.NewArtifactBuilder(b.localDocker, b.pushImages, b.devMode).Build(ctx, out, a, opts.Tag) default: return "", fmt.Errorf("unexpected type %q for local artifact:\n%s", misc.ArtifactType(a), misc.FormatArtifact(a)) @@ -112,3 +113,11 @@ func (b *Builder) getImageIDForTag(ctx context.Context, tag string) (string, err } return insp.ID, nil } + +func ToDockerOpts(opts *build.ImageOptions) *docker.BuildOptions { + d := &docker.BuildOptions{Tag: opts.Tag} + if opts.Configuration == build.Debug { + return d.AddModifier(docker.OptimizeBuildForDebug) + } + return d.AddModifier(docker.OptimizeBuildForRelease) +} diff --git a/pkg/skaffold/build/options.go b/pkg/skaffold/build/options.go new file mode 100644 index 00000000000..c8fd2e852b6 --- /dev/null +++ b/pkg/skaffold/build/options.go @@ -0,0 +1,85 @@ +/* +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 build + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "sort" + "strconv" +) + +// Configuration denotes build configuration for the artifact builder as either Release or Debug +type Configuration int + +const ( + Release = iota + Debug +) + +// ImageOptions provides options for the artifact builder +type ImageOptions struct { + // FIXME: Tag should be []string but we don't support multiple tags yet + Tag string // image tag + Configuration Configuration // build image for release or debug + Args map[string]*string // additional builder specific args +} + +var CurrentConfiguration Configuration + +func CreateBuilderOptions(tag string) *ImageOptions { + return &ImageOptions{ + Tag: tag, + Configuration: CurrentConfiguration, + } +} + +// Hash returns the hash of given image option, useful for image caching +func (opts *ImageOptions) Hash() (string, error) { + var inputs []string + if opts == nil { + return "", nil + } + + inputs = append(inputs, strconv.Itoa(int(opts.Configuration))) + if opts.Args != nil { + inputs = append(inputs, convertBuildArgsToStringArray(opts.Args)...) + } + + hasher := sha256.New() + enc := json.NewEncoder(hasher) + if err := enc.Encode(inputs); err != nil { + return "", err + } + + return hex.EncodeToString(hasher.Sum(nil)), nil +} + +func convertBuildArgsToStringArray(buildArgs map[string]*string) []string { + var args []string + for k, v := range buildArgs { + if v == nil { + args = append(args, k) + continue + } + args = append(args, fmt.Sprintf("%s=%s", k, *v)) + } + sort.Strings(args) + return args +} diff --git a/pkg/skaffold/build/parallel.go b/pkg/skaffold/build/parallel.go index 6dfae0ab021..ab33b583c56 100644 --- a/pkg/skaffold/build/parallel.go +++ b/pkg/skaffold/build/parallel.go @@ -31,7 +31,7 @@ import ( const bufferedLinesPerArtifact = 10000 -type artifactBuilder func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) +type artifactBuilder func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) // For testing var ( @@ -116,7 +116,7 @@ func getBuildResult(ctx context.Context, cw io.Writer, tags tag.ImageTags, artif if !present { return "", fmt.Errorf("unable to find tag for image %s", artifact.ImageName) } - return build(ctx, cw, artifact, tag) + return build(ctx, cw, artifact, CreateBuilderOptions(tag)) } func collectResults(out io.Writer, artifacts []*latest.Artifact, results *sync.Map, outputs []chan string) ([]Artifact, error) { diff --git a/pkg/skaffold/build/parallel_test.go b/pkg/skaffold/build/parallel_test.go index 93c6e26f867..d1a0e135dde 100644 --- a/pkg/skaffold/build/parallel_test.go +++ b/pkg/skaffold/build/parallel_test.go @@ -43,9 +43,9 @@ func TestGetBuild(t *testing.T) { }{ { description: "build succeeds", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { out.Write([]byte("build succeeds")) - return fmt.Sprintf("%s@sha256:abac", tag), nil + return fmt.Sprintf("%s@sha256:abac", opts.Tag), nil }, tags: tag.ImageTags{ "skaffold/image1": "skaffold/image1:v0.0.1", @@ -56,7 +56,7 @@ func TestGetBuild(t *testing.T) { }, { description: "build fails", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { return "", fmt.Errorf("build fails") }, tags: tag.ImageTags{ @@ -203,7 +203,7 @@ func TestInParallel(t *testing.T) { { description: "short and nice build log", expected: "Building [skaffold/image1]...\nshort\nBuilding [skaffold/image2]...\nshort\n", - buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { + buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { out.Write([]byte("short")) return fmt.Sprintf("%s:tag", artifact.ImageName), nil }, @@ -217,7 +217,7 @@ Building [skaffold/image2]... This is a long string more than 10 bytes. And new lines `, - buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { + buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { out.Write([]byte("This is a long string more than 10 bytes.\nAnd new lines")) return fmt.Sprintf("%s:tag", artifact.ImageName), nil }, @@ -280,14 +280,14 @@ func TestInParallelConcurrency(t *testing.T) { var actualConcurrency int32 - builder := func(_ context.Context, _ io.Writer, _ *latest.Artifact, tag string) (string, error) { + builder := func(_ context.Context, _ io.Writer, _ *latest.Artifact, opts *ImageOptions) (string, error) { if atomic.AddInt32(&actualConcurrency, 1) > int32(test.maxConcurrency) { return "", fmt.Errorf("only %d build can run at a time", test.maxConcurrency) } time.Sleep(5 * time.Millisecond) atomic.AddInt32(&actualConcurrency, -1) - return tag, nil + return opts.Tag, nil } initializeEvents() @@ -317,8 +317,8 @@ func TestInParallelForArgs(t *testing.T) { }, { description: "runs in parallel for 2 artifacts", - buildArtifact: func(_ context.Context, _ io.Writer, _ *latest.Artifact, tag string) (string, error) { - return tag, nil + buildArtifact: func(_ context.Context, _ io.Writer, _ *latest.Artifact, opts *ImageOptions) (string, error) { + return opts.Tag, nil }, artifactLen: 2, expected: []Artifact{ diff --git a/pkg/skaffold/build/sequence.go b/pkg/skaffold/build/sequence.go index ff22ccdb268..609e2f08931 100644 --- a/pkg/skaffold/build/sequence.go +++ b/pkg/skaffold/build/sequence.go @@ -41,7 +41,7 @@ func InSequence(ctx context.Context, out io.Writer, tags tag.ImageTags, artifact return nil, fmt.Errorf("unable to find tag for image %s", artifact.ImageName) } - finalTag, err := buildArtifact(ctx, out, artifact, tag) + finalTag, err := buildArtifact(ctx, out, artifact, CreateBuilderOptions(tag)) if err != nil { event.BuildFailed(artifact.ImageName, err) return nil, fmt.Errorf("couldn't build %q: %w", artifact.ImageName, err) diff --git a/pkg/skaffold/build/sequence_test.go b/pkg/skaffold/build/sequence_test.go index 48413a98455..fcb7cdd80e5 100644 --- a/pkg/skaffold/build/sequence_test.go +++ b/pkg/skaffold/build/sequence_test.go @@ -41,8 +41,8 @@ func TestInSequence(t *testing.T) { }{ { description: "build succeeds", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { - return fmt.Sprintf("%s@sha256:abac", tag), nil + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + return fmt.Sprintf("%s@sha256:abac", opts.Tag), nil }, tags: tag.ImageTags{ "skaffold/image1": "skaffold/image1:v0.0.1", @@ -56,7 +56,7 @@ func TestInSequence(t *testing.T) { }, { description: "build fails", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { return "", fmt.Errorf("build fails") }, tags: tag.ImageTags{ @@ -135,8 +135,8 @@ type concatTagger struct { // doBuild calculate the tag based by concatinating the tag values for artifact // builds seen so far. It mimics artifact dependency where the next build result // depends on the previous build result. -func (t *concatTagger) doBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { - t.tag += tag +func (t *concatTagger) doBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + t.tag += opts.Tag return fmt.Sprintf("%s:%s", artifact.ImageName, t.tag), nil } diff --git a/pkg/skaffold/docker/image.go b/pkg/skaffold/docker/image.go index b942363b666..7a4af777227 100644 --- a/pkg/skaffold/docker/image.go +++ b/pkg/skaffold/docker/image.go @@ -61,7 +61,7 @@ type LocalDaemon interface { ExtraEnv() []string ServerVersion(ctx context.Context) (types.Version, error) ConfigFile(ctx context.Context, image string) (*v1.ConfigFile, error) - Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, ref string) (string, error) + Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts *BuildOptions) (string, error) Push(ctx context.Context, out io.Writer, ref string) (string, error) Pull(ctx context.Context, out io.Writer, ref string) error Load(ctx context.Context, out io.Writer, input io.Reader, ref string) (string, error) @@ -155,7 +155,7 @@ func (l *localDaemon) ConfigFile(ctx context.Context, image string) (*v1.ConfigF } // Build performs a docker build and returns the imageID. -func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, ref string) (string, error) { +func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts *BuildOptions) (string, error) { logrus.Debugf("Running docker build: context: %s, dockerfile: %s", workspace, a.DockerfilePath) // Like `docker build`, we ignore the errors @@ -180,8 +180,8 @@ func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string progressOutput := streamformatter.NewProgressOutput(out) body := progress.NewProgressReader(buildCtx, progressOutput, 0, "", "Sending build context to Docker daemon") - resp, err := l.apiClient.ImageBuild(ctx, body, types.ImageBuildOptions{ - Tags: []string{ref}, + imageOpts := &types.ImageBuildOptions{ + Tags: []string{opts.Tag}, Dockerfile: a.DockerfilePath, BuildArgs: buildArgs, CacheFrom: a.CacheFrom, @@ -190,7 +190,10 @@ func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string ForceRemove: l.forceRemove, NetworkMode: strings.ToLower(a.NetworkMode), NoCache: a.NoCache, - }) + } + + imageOpts = opts.ApplyModifiers(imageOpts) + resp, err := l.apiClient.ImageBuild(ctx, body, *imageOpts) if err != nil { return "", fmt.Errorf("docker build: %w", err) } @@ -217,7 +220,7 @@ func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string if imageID == "" { // Maybe this version of Docker doesn't return the digest of the image // that has been built. - imageID, err = l.ImageID(ctx, ref) + imageID, err = l.ImageID(ctx, opts.Tag) if err != nil { return "", fmt.Errorf("getting digest: %w", err) } @@ -404,7 +407,7 @@ func (l *localDaemon) ImageRemove(ctx context.Context, image string, opts types. } // GetBuildArgs gives the build args flags for docker build. -func GetBuildArgs(a *latest.DockerArtifact) ([]string, error) { +func GetBuildArgs(a *latest.DockerArtifact, opts *BuildOptions) ([]string, error) { var args []string buildArgs, err := EvaluateBuildArgs(a.BuildArgs) @@ -412,6 +415,10 @@ func GetBuildArgs(a *latest.DockerArtifact) ([]string, error) { return nil, fmt.Errorf("unable to evaluate build args: %w", err) } + buildArgs = opts.ApplyModifiers(&types.ImageBuildOptions{ + BuildArgs: buildArgs, + }).BuildArgs + var keys []string for k := range buildArgs { keys = append(keys, k) diff --git a/pkg/skaffold/docker/image_test.go b/pkg/skaffold/docker/image_test.go index 03e8aaa72ae..2968b9423a4 100644 --- a/pkg/skaffold/docker/image_test.go +++ b/pkg/skaffold/docker/image_test.go @@ -190,7 +190,7 @@ func TestBuild(t *testing.T) { t.SetEnvs(test.env) localDocker := NewLocalDaemon(test.api, nil, false, nil) - _, err := localDocker.Build(context.Background(), ioutil.Discard, test.workspace, test.artifact, "finalimage") + _, err := localDocker.Build(context.Background(), ioutil.Discard, test.workspace, test.artifact, &BuildOptions{Tag: "finalimage"}) if test.shouldErr { t.CheckErrorContains(test.expectedError, err) @@ -322,7 +322,7 @@ func TestGetBuildArgs(t *testing.T) { testutil.Run(t, test.description, func(t *testutil.T) { t.Override(&util.OSEnviron, func() []string { return test.env }) - result, err := GetBuildArgs(test.artifact) + result, err := GetBuildArgs(test.artifact, &BuildOptions{Tag: ""}) t.CheckError(test.shouldErr, err) if !test.shouldErr { diff --git a/pkg/skaffold/docker/options.go b/pkg/skaffold/docker/options.go new file mode 100644 index 00000000000..f65e4f0f8d9 --- /dev/null +++ b/pkg/skaffold/docker/options.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 docker + +import "github.com/docker/docker/api/types" + +var buildArgsForDebug = map[string]string{ + "GO_GCFLAGS": "'all=-N -l'", // disable build optimization for Golang + // TODO: Add for other languages +} + +var buildArgsForRelease = map[string]string{ + "GO_LDFLAGS": "-w", // omit debug information in build output for Golang + // TODO: Add for other languages +} + +type BuildOptionsModifier func(*types.ImageBuildOptions) *types.ImageBuildOptions + +type BuildOptions struct { + Tag string + modifiers []BuildOptionsModifier +} + +func (b *BuildOptions) AddModifier(m BuildOptionsModifier) *BuildOptions { + b.modifiers = append(b.modifiers, m) + return b +} + +func (b *BuildOptions) ApplyModifiers(opts *types.ImageBuildOptions) *types.ImageBuildOptions { + for _, modifier := range b.modifiers { + opts = modifier(opts) + } + return opts +} + +func OptimizeBuildForDebug(opts *types.ImageBuildOptions) *types.ImageBuildOptions { + if opts == nil { + opts = &types.ImageBuildOptions{} + } + + if opts.BuildArgs == nil { + opts.BuildArgs = make(map[string]*string) + } + + for k, v := range buildArgsForDebug { + if opts.BuildArgs[k] == nil { + opts.BuildArgs[k] = &v + } + } + + return opts +} + +func OptimizeBuildForRelease(opts *types.ImageBuildOptions) *types.ImageBuildOptions { + if opts == nil { + opts = &types.ImageBuildOptions{} + } + + if opts.BuildArgs == nil { + opts.BuildArgs = make(map[string]*string) + } + + for k, v := range buildArgsForRelease { + if opts.BuildArgs[k] == nil { + opts.BuildArgs[k] = &v + } + } + + return opts +} From 8845485e490eb2472e02c6ac98855a61e2936fb8 Mon Sep 17 00:00:00 2001 From: gsquared94 Date: Wed, 15 Jul 2020 10:30:36 -0700 Subject: [PATCH 2/6] Fix test and linters --- cmd/skaffold/app/cmd/debug.go | 2 +- integration/testdata/debug/go/Dockerfile | 2 +- pkg/skaffold/build/cache/cache.go | 2 +- pkg/skaffold/build/cache/hash.go | 8 ++++---- pkg/skaffold/build/cache/hash_test.go | 3 +-- pkg/skaffold/build/gcb/docker_test.go | 2 +- pkg/skaffold/build/gcb/spec.go | 2 +- pkg/skaffold/build/local/local.go | 2 +- 8 files changed, 11 insertions(+), 12 deletions(-) diff --git a/cmd/skaffold/app/cmd/debug.go b/cmd/skaffold/app/cmd/debug.go index eac133f7ece..ded050eee69 100644 --- a/cmd/skaffold/app/cmd/debug.go +++ b/cmd/skaffold/app/cmd/debug.go @@ -20,9 +20,9 @@ import ( "context" "io" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/spf13/cobra" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" debugging "github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy" ) diff --git a/integration/testdata/debug/go/Dockerfile b/integration/testdata/debug/go/Dockerfile index 19811ce6f1d..5d73ef0c995 100644 --- a/integration/testdata/debug/go/Dockerfile +++ b/integration/testdata/debug/go/Dockerfile @@ -3,7 +3,7 @@ FROM golang:1.12 as builder COPY app.go . # Must use eval to handle GOGCFLAGS with spaces like `-gcflags='all=-N -l'` ARG GO_GCFLAGS -RUN eval go build "${GO_GCFLAGS}" -o /app . +RUN eval go build --gcflags="${GO_GCFLAGS}" -o /app . FROM gcr.io/distroless/base # `skaffold debug` uses GOTRACEBACK as an indicator of the Go runtime diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 08adec2a1a0..2063e1a06aa 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -22,10 +22,10 @@ import ( "io/ioutil" "path/filepath" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" homedir "github.com/mitchellh/go-homedir" "github.com/sirupsen/logrus" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext" diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index d68f350c905..81817ac1200 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -27,9 +27,9 @@ import ( "os" "sort" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/sirupsen/logrus" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -92,11 +92,11 @@ func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *late // add image options hash if opts != nil { - if h, err := opts.Hash(); err != nil { + h, err := opts.Hash() + if err != nil { return "", fmt.Errorf("evaluating build args: %w", err) - } else { - inputs = append(inputs, h) } + inputs = append(inputs, h) } // get a key for the hashes diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index 168eb0397f0..be9d888838d 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -21,7 +21,6 @@ import ( "os" "testing" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" @@ -143,7 +142,7 @@ func TestGetHashForArtifact(t *testing.T) { t.Override(&artifactConfigFunction, fakeArtifactConfig) depLister := stubDependencyLister(test.dependencies) - actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, build.CreateBuilderOptions(""), test.devMode) + actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, nil, test.devMode) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go index 0b09bb2af67..ce25aa0aa97 100644 --- a/pkg/skaffold/build/gcb/docker_test.go +++ b/pkg/skaffold/build/gcb/docker_test.go @@ -19,9 +19,9 @@ package gcb import ( "testing" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" cloudbuild "google.golang.org/api/cloudbuild/v1" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" diff --git a/pkg/skaffold/build/gcb/spec.go b/pkg/skaffold/build/gcb/spec.go index a99fa31d25f..ae3bb351a10 100644 --- a/pkg/skaffold/build/gcb/spec.go +++ b/pkg/skaffold/build/gcb/spec.go @@ -19,10 +19,10 @@ package gcb import ( "fmt" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" cloudbuild "google.golang.org/api/cloudbuild/v1" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index bbeee0be751..bf5b437f77b 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -21,7 +21,6 @@ import ( "fmt" "io" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/sirupsen/logrus" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" @@ -32,6 +31,7 @@ import ( "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/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) From 7ec76c6ece96dd388ef5b17796c0088e41a1b478 Mon Sep 17 00:00:00 2001 From: gsquared94 Date: Thu, 16 Jul 2020 15:27:42 -0700 Subject: [PATCH 3/6] Remove global state; simplify build arguments; fix test and linters --- cmd/skaffold/app/cmd/debug.go | 2 - integration/debug_test.go | 14 +-- .../examples/getting-started/Dockerfile | 6 +- integration/testdata/debug/go/Dockerfile | 7 +- pkg/skaffold/build/build.go | 3 +- pkg/skaffold/build/cache/cache.go | 4 +- pkg/skaffold/build/cache/hash.go | 12 ++- pkg/skaffold/build/cache/hash_test.go | 37 ++++---- pkg/skaffold/build/cache/lookup.go | 7 +- pkg/skaffold/build/cache/lookup_test.go | 20 +++-- pkg/skaffold/build/cache/retrieve.go | 9 +- pkg/skaffold/build/cache/retrieve_test.go | 37 ++++---- pkg/skaffold/build/cache/types.go | 9 +- pkg/skaffold/build/cluster/cluster.go | 7 +- pkg/skaffold/build/gcb/cloud_build.go | 7 +- pkg/skaffold/build/local/local.go | 13 ++- pkg/skaffold/build/local/local_test.go | 85 ++++++++++++++----- pkg/skaffold/build/options.go | 35 +++----- pkg/skaffold/build/parallel.go | 21 ++--- pkg/skaffold/build/parallel_test.go | 62 ++++++-------- pkg/skaffold/build/sequence.go | 12 +-- pkg/skaffold/build/sequence_test.go | 33 +++---- pkg/skaffold/docker/image.go | 6 ++ pkg/skaffold/docker/options.go | 15 ++-- pkg/skaffold/runner/build_deploy.go | 16 +++- pkg/skaffold/runner/runner_test.go | 3 +- pkg/skaffold/runner/timings.go | 5 +- pkg/skaffold/runner/timings_test.go | 3 +- 28 files changed, 248 insertions(+), 242 deletions(-) diff --git a/cmd/skaffold/app/cmd/debug.go b/cmd/skaffold/app/cmd/debug.go index ded050eee69..345471178c1 100644 --- a/cmd/skaffold/app/cmd/debug.go +++ b/cmd/skaffold/app/cmd/debug.go @@ -22,7 +22,6 @@ import ( "github.com/spf13/cobra" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" debugging "github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy" ) @@ -44,7 +43,6 @@ func NewCmdDebug() *cobra.Command { func runDebug(ctx context.Context, out io.Writer) error { opts.PortForward.ForwardPods = true - build.CurrentConfiguration = build.Debug deploy.AddManifestTransform(debugging.ApplyDebuggingTransforms) return doDev(ctx, out) diff --git a/integration/debug_test.go b/integration/debug_test.go index c38923a9a7d..846109acf1d 100644 --- a/integration/debug_test.go +++ b/integration/debug_test.go @@ -39,19 +39,7 @@ func TestDebug(t *testing.T) { { description: "kubectl", deployments: []string{"java"}, - pods: []string{"nodejs", "npm", "python3", "go"}, - }, - { - description: "kustomize", - args: []string{"--profile", "kustomize"}, - deployments: []string{"java"}, - pods: []string{"nodejs", "npm", "python3", "go"}, - }, - { - description: "buildpacks", - args: []string{"--profile", "buildpacks"}, - deployments: []string{"java"}, - pods: []string{"nodejs", "npm", "python3", "go"}, + pods: []string{"go"}, }, } for _, test := range tests { diff --git a/integration/examples/getting-started/Dockerfile b/integration/examples/getting-started/Dockerfile index aba3e5560ed..6fe0141a931 100644 --- a/integration/examples/getting-started/Dockerfile +++ b/integration/examples/getting-started/Dockerfile @@ -1,11 +1,11 @@ FROM golang:1.12.9-alpine3.10 as builder # These flag values are provided when building for debug -ARG GO_GCFLAGS -ARG GO_LDFLAGS +ARG SKAFFOLD_GO_GCFLAGS +ARG SKAFFOLD_GO_LDFLAGS COPY main.go . -RUN eval go build --gcflags="${GO_GCFLAGS}" --ldflags="${GO_LDFLAGS}" -o /app main.go +RUN eval go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -ldflags="${SKAFFOLD_GO_LDFLAGS}" -o /app main.go FROM alpine:3.10 # Define GOTRACEBACK to mark this container as using the Go language runtime diff --git a/integration/testdata/debug/go/Dockerfile b/integration/testdata/debug/go/Dockerfile index 5d73ef0c995..014fdb27ddf 100644 --- a/integration/testdata/debug/go/Dockerfile +++ b/integration/testdata/debug/go/Dockerfile @@ -1,9 +1,10 @@ FROM golang:1.12 as builder +ARG SKAFFOLD_GO_GCFLAGS COPY app.go . -# Must use eval to handle GOGCFLAGS with spaces like `-gcflags='all=-N -l'` -ARG GO_GCFLAGS -RUN eval go build --gcflags="${GO_GCFLAGS}" -o /app . +RUN echo "SKAFFOLD_GO_GCFLAGS: ${SKAFFOLD_GO_GCFLAGS}" +# Must use eval to handle GOGCFLAGS with spaces like `all=-N -l` +RUN eval go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -o /app . FROM gcr.io/distroless/base # `skaffold debug` uses GOTRACEBACK as an indicator of the Go runtime diff --git a/pkg/skaffold/build/build.go b/pkg/skaffold/build/build.go index 75d00909a10..6e3161bcab3 100644 --- a/pkg/skaffold/build/build.go +++ b/pkg/skaffold/build/build.go @@ -20,7 +20,6 @@ import ( "context" "io" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) @@ -37,7 +36,7 @@ type Artifact struct { type Builder interface { Labels() map[string]string - Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]Artifact, error) + Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []BuilderOptions) ([]Artifact, error) // Prune removes images built with Skaffold Prune(context.Context, io.Writer) error diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 2063e1a06aa..f5b39f35e12 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -50,7 +50,7 @@ type cache struct { insecureRegistries map[string]bool cacheFile string imagesAreLocal bool - hashForArtifact func(ctx context.Context, a *latest.Artifact, opts *build.ImageOptions) (string, error) + hashForArtifact func(ctx context.Context, a *latest.Artifact, opts build.BuilderOptions) (string, error) } // DependencyLister fetches a list of dependencies for an artifact @@ -85,7 +85,7 @@ func NewCache(runCtx *runcontext.RunContext, imagesAreLocal bool, dependencies D insecureRegistries: runCtx.InsecureRegistries, cacheFile: cacheFile, imagesAreLocal: imagesAreLocal, - hashForArtifact: func(ctx context.Context, a *latest.Artifact, opts *build.ImageOptions) (string, error) { + hashForArtifact: func(ctx context.Context, a *latest.Artifact, opts build.BuilderOptions) (string, error) { return getHashForArtifact(ctx, dependencies, a, opts, runCtx.Opts.IsDevMode()) }, }, nil diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index 81817ac1200..c92249aad20 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -41,7 +41,7 @@ var ( artifactConfigFunction = artifactConfig ) -func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *latest.Artifact, opts *build.ImageOptions, devMode bool) (string, error) { +func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *latest.Artifact, opts build.BuilderOptions, devMode bool) (string, error) { var inputs []string // Append the artifact's configuration @@ -91,13 +91,11 @@ func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *late } // add image options hash - if opts != nil { - h, err := opts.Hash() - if err != nil { - return "", fmt.Errorf("evaluating build args: %w", err) - } - inputs = append(inputs, h) + h, err := opts.Hash() + if err != nil { + return "", fmt.Errorf("evaluating build args: %w", err) } + inputs = append(inputs, h) // get a key for the hashes hasher := sha256.New() diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index be9d888838d..a1aa60cbb4c 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -21,6 +21,7 @@ import ( "os" "testing" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" @@ -61,24 +62,24 @@ func TestGetHashForArtifact(t *testing.T) { description: "hash for artifact", dependencies: []string{"a", "b"}, artifact: &latest.Artifact{}, - expected: "1caa15f7ce87536bddbac30a39768e8e3b212bf591f9b64926fa50c40b614c66", + expected: "448cef891dcc63fe03414342751e30b9f591c3a9dbf488d785e55269591cc26a", }, { description: "ignore file not found", dependencies: []string{"a", "b", "not-found"}, artifact: &latest.Artifact{}, - expected: "1caa15f7ce87536bddbac30a39768e8e3b212bf591f9b64926fa50c40b614c66", + expected: "448cef891dcc63fe03414342751e30b9f591c3a9dbf488d785e55269591cc26a", }, { description: "dependencies in different orders", dependencies: []string{"b", "a"}, artifact: &latest.Artifact{}, - expected: "1caa15f7ce87536bddbac30a39768e8e3b212bf591f9b64926fa50c40b614c66", + expected: "448cef891dcc63fe03414342751e30b9f591c3a9dbf488d785e55269591cc26a", }, { description: "no dependencies", artifact: &latest.Artifact{}, - expected: "53ebd85adc9b03923a7dacfe6002879af526ef6067d441419d6e62fb9bf608ab", + expected: "5c1047030c6cccf3b28e879b588526e146c8d671d4d9b7fbb5ecb922af0bf9a5", }, { description: "docker target", @@ -89,7 +90,7 @@ func TestGetHashForArtifact(t *testing.T) { }, }, }, - expected: "f947b5aad32734914aa2dea0ec95bceff257037e6c2a529007183c3f21547eae", + expected: "2bf7c8b1399d0c6a5c1b39e8fdb60dbfdcf90f5322de6f98d3378ea532c3248a", }, { description: "different docker target", @@ -100,7 +101,7 @@ func TestGetHashForArtifact(t *testing.T) { }, }, }, - expected: "09b366c764d0e39f942283cc081d5522b9dde52e725376661808054e3ed0177f", + expected: "7eb5019be5af98ce5d62a302b23408a278b307a89cdc32ee49ab8e66dfcaac78", }, { description: "build args", @@ -114,7 +115,7 @@ func TestGetHashForArtifact(t *testing.T) { }, }, }, - expected: "f3f710a4ec1d1bfb2a9b8ef2b4b7cc5f254102d17095a71872821b396953a4ce", + expected: "4b04663930accc2e32680ae57b4f9981d4e673d393f6dd1da4bcb03cc59639a9", }, { description: "env variables", @@ -126,14 +127,14 @@ func TestGetHashForArtifact(t *testing.T) { }, }, }, - expected: "a2e225e66c5932e41b0026164bf204533d59974b42fbb645da2855dc9d432cb9", + expected: "3df767d39ea17545165f5cc9fd4ad20b02dc40e7cb24bb3188a42d34161101ee", }, { description: "devmode", dependencies: []string{"a", "b"}, artifact: &latest.Artifact{}, devMode: true, - expected: "f019dda9d0c38fea4aab1685c7da54f7009aba1cb47e3cb4c6c1ce5b10fa5c32", + expected: "d18a2202185518cd139dbdff9be2e4fb972a5de7f2f2448c41b05cb310d1e875", }, } for _, test := range tests { @@ -142,7 +143,7 @@ func TestGetHashForArtifact(t *testing.T) { t.Override(&artifactConfigFunction, fakeArtifactConfig) depLister := stubDependencyLister(test.dependencies) - actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, nil, test.devMode) + actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, build.BuilderOptions{}, test.devMode) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -207,7 +208,7 @@ func TestArtifactConfigDevMode(t *testing.T) { func TestBuildArgs(t *testing.T) { testutil.Run(t, "", func(t *testutil.T) { - expected := "f5b610f4fea07461411b2ea0e2cddfd2ffc28d1baed49180f5d3acee5a18f9e7" + expected := "8f39eede184d46ce6122895c7c9c2bf93cb8f69c9322fe710d1d8384c351e657" artifact := &latest.Artifact{ ArtifactType: latest.ArtifactType{ @@ -220,21 +221,21 @@ func TestBuildArgs(t *testing.T) { t.Override(&hashFunction, mockCacheHasher) t.Override(&artifactConfigFunction, fakeArtifactConfig) - actual, err := getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, nil, false) + actual, err := getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, build.BuilderOptions{}, false) t.CheckNoError(err) t.CheckDeepEqual(expected, actual) // Change order of buildargs artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"two": stringPointer("2"), "one": stringPointer("1")} - actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, nil, false) + actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, build.BuilderOptions{}, false) t.CheckNoError(err) t.CheckDeepEqual(expected, actual) // Change build args, get different hash artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"one": stringPointer("1")} - actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, nil, false) + actual, err = getHashForArtifact(context.Background(), stubDependencyLister(nil), artifact, build.BuilderOptions{}, false) t.CheckNoError(err) if actual == expected { @@ -263,7 +264,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { t.Override(&artifactConfigFunction, fakeArtifactConfig) depLister := stubDependencyLister([]string{"dep"}) - hash1, err := getHashForArtifact(context.Background(), depLister, artifact, nil, false) + hash1, err := getHashForArtifact(context.Background(), depLister, artifact, build.BuilderOptions{}, false) t.CheckNoError(err) @@ -273,7 +274,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { return []string{"FOO=baz"} } - hash2, err := getHashForArtifact(context.Background(), depLister, artifact, nil, false) + hash2, err := getHashForArtifact(context.Background(), depLister, artifact, build.BuilderOptions{}, false) t.CheckNoError(err) if hash1 == hash2 { @@ -330,7 +331,7 @@ func TestCacheHasher(t *testing.T) { path := originalFile depLister := stubDependencyLister([]string{tmpDir.Path(originalFile)}) - oldHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, nil, false) + oldHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, build.BuilderOptions{}, false) t.CheckNoError(err) test.update(originalFile, tmpDir) @@ -339,7 +340,7 @@ func TestCacheHasher(t *testing.T) { } depLister = stubDependencyLister([]string{tmpDir.Path(path)}) - newHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, nil, false) + newHash, err := getHashForArtifact(context.Background(), depLister, &latest.Artifact{}, build.BuilderOptions{}, false) t.CheckNoError(err) t.CheckFalse(test.differentHash && oldHash == newHash) diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index d1a53b6ec83..6300caf92d7 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -22,12 +22,11 @@ import ( "sync" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) -func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, artifacts []*latest.Artifact) []cacheDetails { +func (c *cache) lookupArtifacts(ctx context.Context, artifacts []*latest.Artifact, options []build.BuilderOptions) []cacheDetails { details := make([]cacheDetails, len(artifacts)) var wg sync.WaitGroup @@ -36,7 +35,7 @@ func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, artifac i := i go func() { - details[i] = c.lookup(ctx, artifacts[i], build.CreateBuilderOptions(tags[artifacts[i].ImageName])) + details[i] = c.lookup(ctx, artifacts[i], options[i]) wg.Done() }() } @@ -45,7 +44,7 @@ func (c *cache) lookupArtifacts(ctx context.Context, tags tag.ImageTags, artifac return details } -func (c *cache) lookup(ctx context.Context, a *latest.Artifact, opts *build.ImageOptions) cacheDetails { +func (c *cache) lookup(ctx context.Context, a *latest.Artifact, opts build.BuilderOptions) cacheDetails { hash, err := c.hashForArtifact(ctx, a, opts) if err != nil { return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)} diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go index 83eb095906a..308d95f1587 100644 --- a/pkg/skaffold/build/cache/lookup_test.go +++ b/pkg/skaffold/build/cache/lookup_test.go @@ -31,7 +31,7 @@ import ( func TestLookupLocal(t *testing.T) { tests := []struct { description string - hasher func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) + hasher func(context.Context, *latest.Artifact, build.BuilderOptions) (string, error) cache map[string]ImageDetails api *testutil.FakeAPIClient expected cacheDetails @@ -110,8 +110,10 @@ func TestLookupLocal(t *testing.T) { client: docker.NewLocalDaemon(test.api, nil, false, nil), hashForArtifact: test.hasher, } - details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": "tag"}, []*latest.Artifact{{ + details := cache.lookupArtifacts(context.Background(), []*latest.Artifact{{ ImageName: "artifact", + }}, []build.BuilderOptions{{ + Tag: "tag", }}) // cmp.Diff cannot access unexported fields in *exec.Cmd, so use reflect.DeepEqual here directly @@ -125,7 +127,7 @@ func TestLookupLocal(t *testing.T) { func TestLookupRemote(t *testing.T) { tests := []struct { description string - hasher func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) + hasher func(context.Context, *latest.Artifact, build.BuilderOptions) (string, error) cache map[string]ImageDetails api *testutil.FakeAPIClient expected cacheDetails @@ -194,8 +196,10 @@ func TestLookupRemote(t *testing.T) { client: docker.NewLocalDaemon(test.api, nil, false, nil), hashForArtifact: test.hasher, } - details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": "tag"}, []*latest.Artifact{{ + details := cache.lookupArtifacts(context.Background(), []*latest.Artifact{{ ImageName: "artifact", + }}, []build.BuilderOptions{{ + Tag: "tag", }}) // cmp.Diff cannot access unexported fields in *exec.Cmd, so use reflect.DeepEqual here directly @@ -206,14 +210,14 @@ func TestLookupRemote(t *testing.T) { } } -func mockHasher(value string) func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { - return func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { +func mockHasher(value string) func(context.Context, *latest.Artifact, build.BuilderOptions) (string, error) { + return func(context.Context, *latest.Artifact, build.BuilderOptions) (string, error) { return value, nil } } -func failingHasher(errMessage string) func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { - return func(context.Context, *latest.Artifact, *build.ImageOptions) (string, error) { +func failingHasher(errMessage string) func(context.Context, *latest.Artifact, build.BuilderOptions) (string, error) { + return func(context.Context, *latest.Artifact, build.BuilderOptions) (string, error) { return "", errors.New(errMessage) } } diff --git a/pkg/skaffold/build/cache/retrieve.go b/pkg/skaffold/build/cache/retrieve.go index 25546c73090..70a27a2d4a6 100644 --- a/pkg/skaffold/build/cache/retrieve.go +++ b/pkg/skaffold/build/cache/retrieve.go @@ -25,14 +25,13 @@ import ( "github.com/sirupsen/logrus" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" sErrors "github.com/GoogleContainerTools/skaffold/pkg/skaffold/errors" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) -func (c *cache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildAndTest BuildAndTestFn) ([]build.Artifact, error) { +func (c *cache) Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions, buildAndTest BuildAndTestFn) ([]build.Artifact, error) { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -41,7 +40,7 @@ func (c *cache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, ar color.Default.Fprintln(out, "Checking cache...") lookup := make(chan []cacheDetails) - go func() { lookup <- c.lookupArtifacts(ctx, tags, artifacts) }() + go func() { lookup <- c.lookupArtifacts(ctx, artifacts, options) }() var results []cacheDetails select { @@ -90,7 +89,7 @@ func (c *cache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, ar // Image is already built entry := c.artifactCache[result.Hash()] - tag := tags[artifact.ImageName] + tag := options[i].Tag var uniqueTag string if c.imagesAreLocal { @@ -111,7 +110,7 @@ func (c *cache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, ar logrus.Infoln("Cache check complete in", time.Since(start)) - bRes, err := buildAndTest(ctx, out, tags, needToBuild) + bRes, err := buildAndTest(ctx, out, needToBuild, options) if err != nil { return nil, err } diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index 9c37974c98a..dfd9e0782bf 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -26,7 +26,6 @@ import ( "github.com/docker/docker/api/types" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext" @@ -50,12 +49,12 @@ type mockBuilder struct { dockerDaemon docker.LocalDaemon } -func (b *mockBuilder) BuildAndTest(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { +func (b *mockBuilder) BuildAndTest(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions) ([]build.Artifact, error) { var built []build.Artifact - for _, artifact := range artifacts { + for i, artifact := range artifacts { b.built = append(b.built, artifact) - tag := tags[artifact.ImageName] + tag := options[i].Tag _, err := b.dockerDaemon.Build(ctx, out, artifact.Workspace, artifact.DockerArtifact, &docker.BuildOptions{Tag: tag}) if err != nil { @@ -102,10 +101,12 @@ func TestCacheBuildLocal(t *testing.T) { CacheFile: tmpDir.Path("cache"), }, } - tags := map[string]string{ - "artifact1": "artifact1:tag1", - "artifact2": "artifact2:tag2", + + options := []build.BuilderOptions{ + {Tag: "artifact1:tag1"}, + {Tag: "artifact2:tag2"}, } + artifacts := []*latest.Artifact{ {ImageName: "artifact1", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, {ImageName: "artifact2", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, @@ -128,7 +129,7 @@ func TestCacheBuildLocal(t *testing.T) { // First build: Need to build both artifacts builder := &mockBuilder{dockerDaemon: dockerDaemon, push: false} - bRes, err := artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + bRes, err := artifactCache.Build(context.Background(), ioutil.Discard, artifacts, options, builder.BuildAndTest) t.CheckNoError(err) t.CheckDeepEqual(2, len(builder.built)) @@ -137,7 +138,7 @@ func TestCacheBuildLocal(t *testing.T) { // Second build: both artifacts are read from cache // Artifacts should always be returned in their original order builder = &mockBuilder{dockerDaemon: dockerDaemon, push: false} - bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, artifacts, options, builder.BuildAndTest) t.CheckNoError(err) t.CheckEmpty(builder.built) @@ -149,7 +150,7 @@ func TestCacheBuildLocal(t *testing.T) { // Artifacts should always be returned in their original order tmpDir.Write("dep1", "new content") builder = &mockBuilder{dockerDaemon: dockerDaemon, push: false} - bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, artifacts, options, builder.BuildAndTest) t.CheckNoError(err) t.CheckDeepEqual(1, len(builder.built)) @@ -161,7 +162,7 @@ func TestCacheBuildLocal(t *testing.T) { // Artifacts should always be returned in their original order tmpDir.Write("dep3", "new content") builder = &mockBuilder{dockerDaemon: dockerDaemon, push: false} - bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, artifacts, options, builder.BuildAndTest) t.CheckNoError(err) t.CheckDeepEqual(1, len(builder.built)) @@ -185,10 +186,12 @@ func TestCacheBuildRemote(t *testing.T) { CacheFile: tmpDir.Path("cache"), }, } - tags := map[string]string{ - "artifact1": "artifact1:tag1", - "artifact2": "artifact2:tag2", + + options := []build.BuilderOptions{ + {Tag: "artifact1:tag1"}, + {Tag: "artifact2:tag2"}, } + artifacts := []*latest.Artifact{ {ImageName: "artifact1", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, {ImageName: "artifact2", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, @@ -221,7 +224,7 @@ func TestCacheBuildRemote(t *testing.T) { // First build: Need to build both artifacts builder := &mockBuilder{dockerDaemon: dockerDaemon, push: true} - bRes, err := artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + bRes, err := artifactCache.Build(context.Background(), ioutil.Discard, artifacts, options, builder.BuildAndTest) t.CheckNoError(err) t.CheckDeepEqual(2, len(builder.built)) @@ -232,7 +235,7 @@ func TestCacheBuildRemote(t *testing.T) { // Second build: both artifacts are read from cache builder = &mockBuilder{dockerDaemon: dockerDaemon, push: true} - bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, artifacts, options, builder.BuildAndTest) t.CheckNoError(err) t.CheckEmpty(builder.built) @@ -243,7 +246,7 @@ func TestCacheBuildRemote(t *testing.T) { // Third build: change one artifact's dependencies tmpDir.Write("dep1", "new content") builder = &mockBuilder{dockerDaemon: dockerDaemon, push: true} - bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + bRes, err = artifactCache.Build(context.Background(), ioutil.Discard, artifacts, options, builder.BuildAndTest) t.CheckNoError(err) t.CheckDeepEqual(1, len(builder.built)) diff --git a/pkg/skaffold/build/cache/types.go b/pkg/skaffold/build/cache/types.go index 43a45fe817d..875cbaa739d 100644 --- a/pkg/skaffold/build/cache/types.go +++ b/pkg/skaffold/build/cache/types.go @@ -21,18 +21,17 @@ import ( "io" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) -type BuildAndTestFn func(context.Context, io.Writer, tag.ImageTags, []*latest.Artifact) ([]build.Artifact, error) +type BuildAndTestFn func(context.Context, io.Writer, []*latest.Artifact, []build.BuilderOptions) ([]build.Artifact, error) type Cache interface { - Build(context.Context, io.Writer, tag.ImageTags, []*latest.Artifact, BuildAndTestFn) ([]build.Artifact, error) + Build(context.Context, io.Writer, []*latest.Artifact, []build.BuilderOptions, BuildAndTestFn) ([]build.Artifact, error) } type noCache struct{} -func (n *noCache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildAndTest BuildAndTestFn) ([]build.Artifact, error) { - return buildAndTest(ctx, out, tags, artifacts) +func (n *noCache) Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions, buildAndTest BuildAndTestFn) ([]build.Artifact, error) { + return buildAndTest(ctx, out, artifacts, options) } diff --git a/pkg/skaffold/build/cluster/cluster.go b/pkg/skaffold/build/cluster/cluster.go index cc867032657..5a6672bd1c3 100644 --- a/pkg/skaffold/build/cluster/cluster.go +++ b/pkg/skaffold/build/cluster/cluster.go @@ -24,13 +24,12 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/custom" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/misc" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) // Build builds a list of artifacts with Kaniko. -func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { +func (b *Builder) Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions) ([]build.Artifact, error) { teardownPullSecret, err := b.setupPullSecret(out) if err != nil { return nil, fmt.Errorf("setting up pull secret: %w", err) @@ -45,10 +44,10 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, defer teardownDockerConfigSecret() } - return build.InParallel(ctx, out, tags, artifacts, b.buildArtifact, b.ClusterDetails.Concurrency) + return build.InParallel(ctx, out, artifacts, options, b.buildArtifact, b.ClusterDetails.Concurrency) } -func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *build.ImageOptions) (string, error) { +func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts build.BuilderOptions) (string, error) { digest, err := b.runBuildForArtifact(ctx, out, artifact, opts.Tag) if err != nil { return "", err diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 9bed7230c41..45fab8e4184 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -34,7 +34,6 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "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" @@ -45,11 +44,11 @@ import ( ) // Build builds a list of artifacts with Google Cloud Build. -func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { - return build.InParallel(ctx, out, tags, artifacts, b.buildArtifactWithCloudBuild, b.GoogleCloudBuild.Concurrency) +func (b *Builder) Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions) ([]build.Artifact, error) { + return build.InParallel(ctx, out, artifacts, options, b.buildArtifactWithCloudBuild, b.GoogleCloudBuild.Concurrency) } -func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *build.ImageOptions) (string, error) { +func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts build.BuilderOptions) (string, error) { cbclient, err := cloudbuild.NewService(ctx, gcp.ClientOptions()...) if err != nil { return "", fmt.Errorf("getting cloudbuild client: %w", err) diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index bf5b437f77b..3f700ee6968 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -29,7 +29,6 @@ import ( "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/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -37,16 +36,16 @@ import ( // Build runs a docker build on the host and tags the resulting image with // its checksum. It streams build progress to the writer argument. -func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { +func (b *Builder) Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions) ([]build.Artifact, error) { if b.localCluster { color.Default.Fprintf(out, "Found [%s] context, using local docker daemon.\n", b.kubeContext) } defer b.localDocker.Close() - return build.InParallel(ctx, out, tags, artifacts, b.buildArtifact, *b.cfg.Concurrency) + return build.InParallel(ctx, out, artifacts, options, b.buildArtifact, *b.cfg.Concurrency) } -func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, opts *build.ImageOptions) (string, error) { +func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, opts build.BuilderOptions) (string, error) { digestOrImageID, err := b.runBuildForArtifact(ctx, out, a, opts) if err != nil { return "", err @@ -74,7 +73,7 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Ar return build.TagWithImageID(ctx, opts.Tag, imageID, b.localDocker) } -func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, opts *build.ImageOptions) (string, error) { +func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, opts build.BuilderOptions) (string, error) { if !b.pushImages { // All of the builders will rely on a local Docker: // + Either to build the image, @@ -114,10 +113,10 @@ func (b *Builder) getImageIDForTag(ctx context.Context, tag string) (string, err return insp.ID, nil } -func ToDockerOpts(opts *build.ImageOptions) *docker.BuildOptions { +func ToDockerOpts(opts build.BuilderOptions) *docker.BuildOptions { d := &docker.BuildOptions{Tag: opts.Tag} if opts.Configuration == build.Debug { return d.AddModifier(docker.OptimizeBuildForDebug) } - return d.AddModifier(docker.OptimizeBuildForRelease) + return d.AddModifier(docker.OptimizeBuildForDev) } diff --git a/pkg/skaffold/build/local/local_test.go b/pkg/skaffold/build/local/local_test.go index 65b81829bb1..c57808ec2de 100644 --- a/pkg/skaffold/build/local/local_test.go +++ b/pkg/skaffold/build/local/local_test.go @@ -26,7 +26,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" @@ -48,7 +47,7 @@ func TestLocalRun(t *testing.T) { tests := []struct { description string api *testutil.FakeAPIClient - tags tag.ImageTags + opts []build.BuilderOptions artifacts []*latest.Artifact expected []build.Artifact expectedWarnings []string @@ -64,7 +63,12 @@ func TestLocalRun(t *testing.T) { DockerArtifact: &latest.DockerArtifact{}, }}, }, - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, api: &testutil.FakeAPIClient{}, pushImages: false, expected: []build.Artifact{{ @@ -80,7 +84,12 @@ func TestLocalRun(t *testing.T) { DockerArtifact: &latest.DockerArtifact{}, }}, }, - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, api: &testutil.FakeAPIClient{ ErrImageInspect: true, }, @@ -94,7 +103,12 @@ func TestLocalRun(t *testing.T) { DockerArtifact: &latest.DockerArtifact{}, }}, }, - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, api: &testutil.FakeAPIClient{}, pushImages: true, expected: []build.Artifact{{ @@ -113,7 +127,12 @@ func TestLocalRun(t *testing.T) { DockerArtifact: &latest.DockerArtifact{}, }}, }, - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, api: &testutil.FakeAPIClient{ ErrImageBuild: true, }, @@ -127,19 +146,18 @@ func TestLocalRun(t *testing.T) { DockerArtifact: &latest.DockerArtifact{}, }}, }, - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, pushImages: true, api: &testutil.FakeAPIClient{ ErrImageBuild: true, }, shouldErr: true, }, - { - description: "unknown artifact type", - artifacts: []*latest.Artifact{{}}, - api: &testutil.FakeAPIClient{}, - shouldErr: true, - }, { description: "cache-from images already pulled", artifacts: []*latest.Artifact{{ @@ -150,8 +168,13 @@ func TestLocalRun(t *testing.T) { }, }}, }, - api: (&testutil.FakeAPIClient{}).Add("pull1", "imageID1").Add("pull2", "imageID2"), - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + api: (&testutil.FakeAPIClient{}).Add("pull1", "imageID1").Add("pull2", "imageID2"), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", Tag: "gcr.io/test/image:1", @@ -167,8 +190,13 @@ func TestLocalRun(t *testing.T) { }, }}, }, - api: (&testutil.FakeAPIClient{}).Add("pull1", "imageid").Add("pull2", "anotherimageid"), - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + api: (&testutil.FakeAPIClient{}).Add("pull1", "imageid").Add("pull2", "anotherimageid"), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", Tag: "gcr.io/test/image:1", @@ -187,7 +215,12 @@ func TestLocalRun(t *testing.T) { api: (&testutil.FakeAPIClient{ ErrImagePull: true, }).Add("pull1", ""), - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, expected: []build.Artifact{{ ImageName: "gcr.io/test/image", Tag: "gcr.io/test/image:1", @@ -207,7 +240,12 @@ func TestLocalRun(t *testing.T) { api: &testutil.FakeAPIClient{ ErrImageInspect: true, }, - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, shouldErr: true, }, { @@ -218,7 +256,12 @@ func TestLocalRun(t *testing.T) { DockerArtifact: &latest.DockerArtifact{}, }}, }, - tags: tag.ImageTags(map[string]string{"gcr.io/test/image": "gcr.io/test/image:tag"}), + opts: []build.BuilderOptions{ + { + Tag: "gcr.io/test/image:tag", + Configuration: build.Dev, + }, + }, api: &testutil.FakeAPIClient{ ErrVersion: true, }, @@ -249,7 +292,7 @@ func TestLocalRun(t *testing.T) { })) t.CheckNoError(err) - res, err := builder.Build(context.Background(), ioutil.Discard, test.tags, test.artifacts) + res, err := builder.Build(context.Background(), ioutil.Discard, test.artifacts, test.opts) t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, res) t.CheckDeepEqual(test.expectedWarnings, fakeWarner.Warnings) diff --git a/pkg/skaffold/build/options.go b/pkg/skaffold/build/options.go index c8fd2e852b6..9198f4e7243 100644 --- a/pkg/skaffold/build/options.go +++ b/pkg/skaffold/build/options.go @@ -22,44 +22,33 @@ import ( "encoding/json" "fmt" "sort" - "strconv" ) -// Configuration denotes build configuration for the artifact builder as either Release or Debug -type Configuration int +// Configuration denotes build configuration for the artifact builder as either Dev or Debug +type Configuration string const ( - Release = iota - Debug + Dev = Configuration("dev") + Debug = Configuration("debug") ) -// ImageOptions provides options for the artifact builder -type ImageOptions struct { - // FIXME: Tag should be []string but we don't support multiple tags yet +// BuilderOptions provides options for the artifact builder +type BuilderOptions struct { Tag string // image tag - Configuration Configuration // build image for release or debug - Args map[string]*string // additional builder specific args -} - -var CurrentConfiguration Configuration - -func CreateBuilderOptions(tag string) *ImageOptions { - return &ImageOptions{ - Tag: tag, - Configuration: CurrentConfiguration, - } + Configuration Configuration // build image for dev or debug + BuildArgs map[string]*string // additional builder specific args } // Hash returns the hash of given image option, useful for image caching -func (opts *ImageOptions) Hash() (string, error) { +func (opts *BuilderOptions) Hash() (string, error) { var inputs []string if opts == nil { return "", nil } - inputs = append(inputs, strconv.Itoa(int(opts.Configuration))) - if opts.Args != nil { - inputs = append(inputs, convertBuildArgsToStringArray(opts.Args)...) + inputs = append(inputs, string(opts.Configuration)) + if opts.BuildArgs != nil { + inputs = append(inputs, convertBuildArgsToStringArray(opts.BuildArgs)...) } hasher := sha256.New() diff --git a/pkg/skaffold/build/parallel.go b/pkg/skaffold/build/parallel.go index ab33b583c56..367668f8e33 100644 --- a/pkg/skaffold/build/parallel.go +++ b/pkg/skaffold/build/parallel.go @@ -23,7 +23,6 @@ import ( "io" "sync" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -31,7 +30,7 @@ import ( const bufferedLinesPerArtifact = 10000 -type artifactBuilder func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) +type artifactBuilder func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) // For testing var ( @@ -40,13 +39,13 @@ var ( ) // InParallel builds a list of artifacts in parallel but prints the logs in sequential order. -func InParallel(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildArtifact artifactBuilder, concurrency int) ([]Artifact, error) { +func InParallel(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []BuilderOptions, buildArtifact artifactBuilder, concurrency int) ([]Artifact, error) { if len(artifacts) == 0 { return nil, nil } if len(artifacts) == 1 || concurrency == 1 { - return runInSequence(ctx, out, tags, artifacts, buildArtifact) + return runInSequence(ctx, out, artifacts, options, buildArtifact) } var wg sync.WaitGroup @@ -73,7 +72,7 @@ func InParallel(ctx context.Context, out io.Writer, tags tag.ImageTags, artifact // sync.Map go func(i int) { sem <- true - runBuild(ctx, w, tags, artifacts[i], results, buildArtifact) + runBuild(ctx, w, artifacts[i], options[i], results, buildArtifact) <-sem wg.Done() @@ -87,10 +86,10 @@ func InParallel(ctx context.Context, out io.Writer, tags tag.ImageTags, artifact return collectResults(out, artifacts, results, outputs) } -func runBuild(ctx context.Context, cw io.WriteCloser, tags tag.ImageTags, artifact *latest.Artifact, results *sync.Map, build artifactBuilder) { +func runBuild(ctx context.Context, cw io.WriteCloser, artifact *latest.Artifact, opts BuilderOptions, results *sync.Map, build artifactBuilder) { event.BuildInProgress(artifact.ImageName) - finalTag, err := getBuildResult(ctx, cw, tags, artifact, build) + finalTag, err := getBuildResult(ctx, cw, artifact, opts, build) if err != nil { event.BuildFailed(artifact.ImageName, err) results.Store(artifact.ImageName, err) @@ -110,13 +109,9 @@ func readOutputAndWriteToChannel(r io.Reader, lines chan string) { close(lines) } -func getBuildResult(ctx context.Context, cw io.Writer, tags tag.ImageTags, artifact *latest.Artifact, build artifactBuilder) (string, error) { +func getBuildResult(ctx context.Context, cw io.Writer, artifact *latest.Artifact, opts BuilderOptions, build artifactBuilder) (string, error) { color.Default.Fprintf(cw, "Building [%s]...\n", artifact.ImageName) - tag, present := tags[artifact.ImageName] - if !present { - return "", fmt.Errorf("unable to find tag for image %s", artifact.ImageName) - } - return build(ctx, cw, artifact, CreateBuilderOptions(tag)) + return build(ctx, cw, artifact, opts) } func collectResults(out io.Writer, artifacts []*latest.Artifact, results *sync.Map, outputs []chan string) ([]Artifact, error) { diff --git a/pkg/skaffold/build/parallel_test.go b/pkg/skaffold/build/parallel_test.go index d1a0e135dde..f15729ddc63 100644 --- a/pkg/skaffold/build/parallel_test.go +++ b/pkg/skaffold/build/parallel_test.go @@ -27,47 +27,35 @@ import ( "testing" "time" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/testutil" ) func TestGetBuild(t *testing.T) { tests := []struct { - description string + shouldErr bool buildArtifact artifactBuilder - tags tag.ImageTags + description string expectedTag string expectedOut string - shouldErr bool + opts BuilderOptions }{ { description: "build succeeds", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) { out.Write([]byte("build succeeds")) return fmt.Sprintf("%s@sha256:abac", opts.Tag), nil }, - tags: tag.ImageTags{ - "skaffold/image1": "skaffold/image1:v0.0.1", - "skaffold/image2": "skaffold/image2:v0.0.2", - }, + opts: BuilderOptions{Tag: "skaffold/image1:v0.0.1"}, expectedTag: "skaffold/image1:v0.0.1@sha256:abac", expectedOut: "Building [skaffold/image1]...\nbuild succeeds", }, { description: "build fails", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) { return "", fmt.Errorf("build fails") }, - tags: tag.ImageTags{ - "skaffold/image1": "", - }, - expectedOut: "Building [skaffold/image1]...\n", - shouldErr: true, - }, - { - description: "tag not found", - tags: tag.ImageTags{}, + opts: BuilderOptions{}, expectedOut: "Building [skaffold/image1]...\n", shouldErr: true, }, @@ -77,7 +65,7 @@ func TestGetBuild(t *testing.T) { out := new(bytes.Buffer) artifact := &latest.Artifact{ImageName: "skaffold/image1"} - got, err := getBuildResult(context.Background(), out, test.tags, artifact, test.buildArtifact) + got, err := getBuildResult(context.Background(), out, artifact, test.opts, test.buildArtifact) t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedTag, got) t.CheckDeepEqual(test.expectedOut, out.String()) @@ -203,7 +191,7 @@ func TestInParallel(t *testing.T) { { description: "short and nice build log", expected: "Building [skaffold/image1]...\nshort\nBuilding [skaffold/image2]...\nshort\n", - buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) { out.Write([]byte("short")) return fmt.Sprintf("%s:tag", artifact.ImageName), nil }, @@ -217,7 +205,7 @@ Building [skaffold/image2]... This is a long string more than 10 bytes. And new lines `, - buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + buildFunc: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) { out.Write([]byte("This is a long string more than 10 bytes.\nAnd new lines")) return fmt.Sprintf("%s:tag", artifact.ImageName), nil }, @@ -230,13 +218,15 @@ And new lines {ImageName: "skaffold/image1"}, {ImageName: "skaffold/image2"}, } - tags := tag.ImageTags{ - "skaffold/image1": "skaffold/image1:v0.0.1", - "skaffold/image2": "skaffold/image2:v0.0.2", + + options := []BuilderOptions{ + {Tag: "skaffold/image1"}, + {Tag: "skaffold/image2"}, } + initializeEvents() - InParallel(context.Background(), out, tags, artifacts, test.buildFunc, 0) + InParallel(context.Background(), out, artifacts, options, test.buildFunc, 0) t.CheckDeepEqual(test.expected, out.String()) }) @@ -268,19 +258,19 @@ func TestInParallelConcurrency(t *testing.T) { for _, test := range tests { testutil.Run(t, fmt.Sprintf("%d artifacts, max concurrency=%d", test.artifacts, test.limit), func(t *testutil.T) { var artifacts []*latest.Artifact - tags := tag.ImageTags{} + opts := make([]BuilderOptions, 0) for i := 0; i < test.artifacts; i++ { imageName := fmt.Sprintf("skaffold/image%d", i) tag := fmt.Sprintf("skaffold/image%d:tag", i) artifacts = append(artifacts, &latest.Artifact{ImageName: imageName}) - tags[imageName] = tag + opts = append(opts, BuilderOptions{Tag: tag}) } var actualConcurrency int32 - builder := func(_ context.Context, _ io.Writer, _ *latest.Artifact, opts *ImageOptions) (string, error) { + builder := func(_ context.Context, _ io.Writer, _ *latest.Artifact, opts BuilderOptions) (string, error) { if atomic.AddInt32(&actualConcurrency, 1) > int32(test.maxConcurrency) { return "", fmt.Errorf("only %d build can run at a time", test.maxConcurrency) } @@ -291,7 +281,7 @@ func TestInParallelConcurrency(t *testing.T) { } initializeEvents() - results, err := InParallel(context.Background(), ioutil.Discard, tags, artifacts, builder, test.limit) + results, err := InParallel(context.Background(), ioutil.Discard, artifacts, opts, builder, test.limit) t.CheckNoError(err) t.CheckDeepEqual(test.artifacts, len(results)) @@ -302,14 +292,14 @@ func TestInParallelConcurrency(t *testing.T) { func TestInParallelForArgs(t *testing.T) { tests := []struct { description string - inSeqFunc func(context.Context, io.Writer, tag.ImageTags, []*latest.Artifact, artifactBuilder) ([]Artifact, error) + inSeqFunc func(context.Context, io.Writer, []*latest.Artifact, []BuilderOptions, artifactBuilder) ([]Artifact, error) buildArtifact artifactBuilder artifactLen int expected []Artifact }{ { description: "runs in sequence for 1 artifact", - inSeqFunc: func(context.Context, io.Writer, tag.ImageTags, []*latest.Artifact, artifactBuilder) ([]Artifact, error) { + inSeqFunc: func(context.Context, io.Writer, []*latest.Artifact, []BuilderOptions, artifactBuilder) ([]Artifact, error) { return []Artifact{{ImageName: "singleArtifact", Tag: "one"}}, nil }, artifactLen: 1, @@ -317,7 +307,7 @@ func TestInParallelForArgs(t *testing.T) { }, { description: "runs in parallel for 2 artifacts", - buildArtifact: func(_ context.Context, _ io.Writer, _ *latest.Artifact, opts *ImageOptions) (string, error) { + buildArtifact: func(_ context.Context, _ io.Writer, _ *latest.Artifact, opts BuilderOptions) (string, error) { return opts.Tag, nil }, artifactLen: 2, @@ -335,17 +325,17 @@ func TestInParallelForArgs(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { artifacts := make([]*latest.Artifact, test.artifactLen) - tags := tag.ImageTags{} + opts := make([]BuilderOptions, 0) for i := 0; i < test.artifactLen; i++ { a := fmt.Sprintf("artifact%d", i+1) artifacts[i] = &latest.Artifact{ImageName: a} - tags[a] = fmt.Sprintf("%s@tag%d", a, i+1) + opts = append(opts, BuilderOptions{Tag: fmt.Sprintf("%s@tag%d", a, i+1)}) } if test.inSeqFunc != nil { t.Override(&runInSequence, test.inSeqFunc) } initializeEvents() - actual, _ := InParallel(context.Background(), ioutil.Discard, tags, artifacts, test.buildArtifact, 0) + actual, _ := InParallel(context.Background(), ioutil.Discard, artifacts, opts, test.buildArtifact, 0) t.CheckDeepEqual(test.expected, actual) }) diff --git a/pkg/skaffold/build/sequence.go b/pkg/skaffold/build/sequence.go index 609e2f08931..d416e0f6664 100644 --- a/pkg/skaffold/build/sequence.go +++ b/pkg/skaffold/build/sequence.go @@ -21,27 +21,21 @@ import ( "fmt" "io" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" ) // InSequence builds a list of artifacts in sequence. -func InSequence(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildArtifact artifactBuilder) ([]Artifact, error) { +func InSequence(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []BuilderOptions, buildArtifact artifactBuilder) ([]Artifact, error) { var builds []Artifact - for _, artifact := range artifacts { + for i, artifact := range artifacts { color.Default.Fprintf(out, "Building [%s]...\n", artifact.ImageName) event.BuildInProgress(artifact.ImageName) - tag, present := tags[artifact.ImageName] - if !present { - return nil, fmt.Errorf("unable to find tag for image %s", artifact.ImageName) - } - - finalTag, err := buildArtifact(ctx, out, artifact, CreateBuilderOptions(tag)) + finalTag, err := buildArtifact(ctx, out, artifact, options[i]) if err != nil { event.BuildFailed(artifact.ImageName, err) return nil, fmt.Errorf("couldn't build %q: %w", artifact.ImageName, err) diff --git a/pkg/skaffold/build/sequence_test.go b/pkg/skaffold/build/sequence_test.go index fcb7cdd80e5..a6e9e9a75b7 100644 --- a/pkg/skaffold/build/sequence_test.go +++ b/pkg/skaffold/build/sequence_test.go @@ -24,7 +24,6 @@ import ( "io/ioutil" "testing" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/testutil" @@ -34,19 +33,19 @@ func TestInSequence(t *testing.T) { tests := []struct { description string buildArtifact artifactBuilder - tags tag.ImageTags expectedArtifacts []Artifact + options []BuilderOptions expectedOut string shouldErr bool }{ { description: "build succeeds", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) { return fmt.Sprintf("%s@sha256:abac", opts.Tag), nil }, - tags: tag.ImageTags{ - "skaffold/image1": "skaffold/image1:v0.0.1", - "skaffold/image2": "skaffold/image2:v0.0.2", + options: []BuilderOptions{ + {Tag: "skaffold/image1:v0.0.1"}, + {Tag: "skaffold/image2:v0.0.2"}, }, expectedArtifacts: []Artifact{ {ImageName: "skaffold/image1", Tag: "skaffold/image1:v0.0.1@sha256:abac"}, @@ -56,21 +55,15 @@ func TestInSequence(t *testing.T) { }, { description: "build fails", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { + buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) { return "", fmt.Errorf("build fails") }, - tags: tag.ImageTags{ - "skaffold/image1": "", + options: []BuilderOptions{ + {Tag: ""}, }, expectedOut: "Building [skaffold/image1]...\n", shouldErr: true, }, - { - description: "tag not found", - tags: tag.ImageTags{}, - expectedOut: "Building [skaffold/image1]...\n", - shouldErr: true, - }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { @@ -80,7 +73,7 @@ func TestInSequence(t *testing.T) { {ImageName: "skaffold/image2"}, } - got, err := InSequence(context.Background(), out, test.tags, artifacts, test.buildArtifact) + got, err := InSequence(context.Background(), out, artifacts, test.options, test.buildArtifact) t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedArtifacts, got) t.CheckDeepEqual(test.expectedOut, out.String()) @@ -111,16 +104,16 @@ func TestInSequenceResultsOrder(t *testing.T) { out := ioutil.Discard initializeEvents() artifacts := make([]*latest.Artifact, len(test.images)) - tags := tag.ImageTags{} + opts := make([]BuilderOptions, 0) for i, image := range test.images { artifacts[i] = &latest.Artifact{ ImageName: image, } - tags[image] = image + opts = append(opts, BuilderOptions{Tag: image}) } builder := concatTagger{} - got, err := InSequence(context.Background(), out, tags, artifacts, builder.doBuild) + got, err := InSequence(context.Background(), out, artifacts, opts, builder.doBuild) t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, got) }) @@ -135,7 +128,7 @@ type concatTagger struct { // doBuild calculate the tag based by concatinating the tag values for artifact // builds seen so far. It mimics artifact dependency where the next build result // depends on the previous build result. -func (t *concatTagger) doBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts *ImageOptions) (string, error) { +func (t *concatTagger) doBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuilderOptions) (string, error) { t.tag += opts.Tag return fmt.Sprintf("%s:%s", artifact.ImageName, t.tag), nil } diff --git a/pkg/skaffold/docker/image.go b/pkg/skaffold/docker/image.go index 7a4af777227..eadbd12fe2c 100644 --- a/pkg/skaffold/docker/image.go +++ b/pkg/skaffold/docker/image.go @@ -193,6 +193,12 @@ func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string } imageOpts = opts.ApplyModifiers(imageOpts) + + if logrus.GetLevel() <= logrus.DebugLevel { + d, _ := json.Marshal(imageOpts.BuildArgs) + logrus.Debugf("docker build args: %s\n", string(d)) + } + resp, err := l.apiClient.ImageBuild(ctx, body, *imageOpts) if err != nil { return "", fmt.Errorf("docker build: %w", err) diff --git a/pkg/skaffold/docker/options.go b/pkg/skaffold/docker/options.go index f65e4f0f8d9..042a100299c 100644 --- a/pkg/skaffold/docker/options.go +++ b/pkg/skaffold/docker/options.go @@ -16,15 +16,17 @@ limitations under the License. package docker -import "github.com/docker/docker/api/types" +import ( + "github.com/docker/docker/api/types" +) var buildArgsForDebug = map[string]string{ - "GO_GCFLAGS": "'all=-N -l'", // disable build optimization for Golang + "SKAFFOLD_GO_GCFLAGS": "'all=-N -l'", // disable build optimization for Golang // TODO: Add for other languages } -var buildArgsForRelease = map[string]string{ - "GO_LDFLAGS": "-w", // omit debug information in build output for Golang +var buildArgsForDev = map[string]string{ + "SKAFFOLD_GO_LDFLAGS": "-w", // omit debug information in build output for Golang // TODO: Add for other languages } @@ -61,11 +63,10 @@ func OptimizeBuildForDebug(opts *types.ImageBuildOptions) *types.ImageBuildOptio opts.BuildArgs[k] = &v } } - return opts } -func OptimizeBuildForRelease(opts *types.ImageBuildOptions) *types.ImageBuildOptions { +func OptimizeBuildForDev(opts *types.ImageBuildOptions) *types.ImageBuildOptions { if opts == nil { opts = &types.ImageBuildOptions{} } @@ -74,7 +75,7 @@ func OptimizeBuildForRelease(opts *types.ImageBuildOptions) *types.ImageBuildOpt opts.BuildArgs = make(map[string]*string) } - for k, v := range buildArgsForRelease { + for k, v := range buildArgsForDev { if opts.BuildArgs[k] == nil { opts.BuildArgs[k] = &v } diff --git a/pkg/skaffold/runner/build_deploy.go b/pkg/skaffold/runner/build_deploy.go index 6d7c832ea47..13eb884728e 100644 --- a/pkg/skaffold/runner/build_deploy.go +++ b/pkg/skaffold/runner/build_deploy.go @@ -56,14 +56,19 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa return bRes, nil } - bRes, err := r.cache.Build(ctx, out, tags, artifacts, func(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { + options := make([]build.BuilderOptions, 0) + for _, artifact := range artifacts { + options = append(options, r.CreateBuilderOptions(artifact, tags[artifact.ImageName])) + } + + bRes, err := r.cache.Build(ctx, out, artifacts, options, func(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions) ([]build.Artifact, error) { if len(artifacts) == 0 { return nil, nil } r.hasBuilt = true - bRes, err := r.builder.Build(ctx, out, tags, artifacts) + bRes, err := r.builder.Build(ctx, out, artifacts, options) if err != nil { return nil, err } @@ -201,3 +206,10 @@ func (r *SkaffoldRunner) imageTags(ctx context.Context, out io.Writer, artifacts logrus.Infoln("Tags generated in", time.Since(start)) return imageTags, nil } + +func (r *SkaffoldRunner) CreateBuilderOptions(artifact *latest.Artifact, tag string) build.BuilderOptions { + return build.BuilderOptions{ + Tag: tag, + Configuration: build.Configuration(r.runCtx.Opts.Command), + } +} diff --git a/pkg/skaffold/runner/runner_test.go b/pkg/skaffold/runner/runner_test.go index a77ec54d3fd..5639a466edc 100644 --- a/pkg/skaffold/runner/runner_test.go +++ b/pkg/skaffold/runner/runner_test.go @@ -26,7 +26,6 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/local" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon" @@ -101,7 +100,7 @@ func (t *TestBench) enterNewCycle() { t.currentActions = Actions{} } -func (t *TestBench) Build(_ context.Context, _ io.Writer, _ tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { +func (t *TestBench) Build(_ context.Context, _ io.Writer, artifacts []*latest.Artifact, _ []build.BuilderOptions) ([]build.Artifact, error) { if len(t.buildErrors) > 0 { err := t.buildErrors[0] t.buildErrors = t.buildErrors[1:] diff --git a/pkg/skaffold/runner/timings.go b/pkg/skaffold/runner/timings.go index 40574806f84..46ce627ffea 100644 --- a/pkg/skaffold/runner/timings.go +++ b/pkg/skaffold/runner/timings.go @@ -25,7 +25,6 @@ import ( "k8s.io/apimachinery/pkg/labels" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -55,13 +54,13 @@ func (w withTimings) Labels() map[string]string { return labels.Merge(w.Builder.Labels(), w.Deployer.Labels()) } -func (w withTimings) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { +func (w withTimings) Build(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, options []build.BuilderOptions) ([]build.Artifact, error) { if len(artifacts) == 0 && w.cacheArtifacts { return nil, nil } start := time.Now() - bRes, err := w.Builder.Build(ctx, out, tags, artifacts) + bRes, err := w.Builder.Build(ctx, out, artifacts, options) if err != nil { return nil, err } diff --git a/pkg/skaffold/runner/timings_test.go b/pkg/skaffold/runner/timings_test.go index d5578a424d2..4c70921b84c 100644 --- a/pkg/skaffold/runner/timings_test.go +++ b/pkg/skaffold/runner/timings_test.go @@ -27,7 +27,6 @@ import ( logrustest "github.com/sirupsen/logrus/hooks/test" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/deploy" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/test" @@ -39,7 +38,7 @@ type mockBuilder struct { err bool } -func (m *mockBuilder) Build(context.Context, io.Writer, tag.ImageTags, []*latest.Artifact) ([]build.Artifact, error) { +func (m *mockBuilder) Build(context.Context, io.Writer, []*latest.Artifact, []build.BuilderOptions) ([]build.Artifact, error) { if m.err { return nil, errors.New("Unable to build") } From fa6b188cf3b3cd5e57cb15dcd1c37cbfe53f62cf Mon Sep 17 00:00:00 2001 From: gsquared94 Date: Thu, 16 Jul 2020 15:31:40 -0700 Subject: [PATCH 4/6] Revert temp changes --- integration/debug_test.go | 14 +++++++++++++- integration/testdata/debug/go/Dockerfile | 8 ++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/integration/debug_test.go b/integration/debug_test.go index 846109acf1d..c38923a9a7d 100644 --- a/integration/debug_test.go +++ b/integration/debug_test.go @@ -39,7 +39,19 @@ func TestDebug(t *testing.T) { { description: "kubectl", deployments: []string{"java"}, - pods: []string{"go"}, + pods: []string{"nodejs", "npm", "python3", "go"}, + }, + { + description: "kustomize", + args: []string{"--profile", "kustomize"}, + deployments: []string{"java"}, + pods: []string{"nodejs", "npm", "python3", "go"}, + }, + { + description: "buildpacks", + args: []string{"--profile", "buildpacks"}, + deployments: []string{"java"}, + pods: []string{"nodejs", "npm", "python3", "go"}, }, } for _, test := range tests { diff --git a/integration/testdata/debug/go/Dockerfile b/integration/testdata/debug/go/Dockerfile index 014fdb27ddf..6499b8f4c42 100644 --- a/integration/testdata/debug/go/Dockerfile +++ b/integration/testdata/debug/go/Dockerfile @@ -1,10 +1,10 @@ FROM golang:1.12 as builder -ARG SKAFFOLD_GO_GCFLAGS COPY app.go . -RUN echo "SKAFFOLD_GO_GCFLAGS: ${SKAFFOLD_GO_GCFLAGS}" -# Must use eval to handle GOGCFLAGS with spaces like `all=-N -l` -RUN eval go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -o /app . +ARG SKAFFOLD_GO_GCFLAGS +ARG SKAFFOLD_LD_GCFLAGS +# Must use eval to handle gcflags with spaces like `all=-N -l` +RUN eval go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -ldflags="${SKAFFOLD_GO_LDFLAGS}" -o /app . FROM gcr.io/distroless/base # `skaffold debug` uses GOTRACEBACK as an indicator of the Go runtime From 662652f8921727e0bbe3ef6596610a68584b9920 Mon Sep 17 00:00:00 2001 From: gsquared94 Date: Thu, 16 Jul 2020 19:10:49 -0700 Subject: [PATCH 5/6] Implement for buildpacks --- integration/testdata/debug/skaffold.yaml | 2 - pkg/skaffold/build/buildpacks/build.go | 12 +-- pkg/skaffold/build/buildpacks/build_test.go | 26 +++---- pkg/skaffold/build/buildpacks/lifecycle.go | 14 ++-- pkg/skaffold/build/buildpacks/options.go | 82 +++++++++++++++++++++ pkg/skaffold/build/cache/retrieve_test.go | 2 +- pkg/skaffold/build/gcb/docker.go | 4 +- pkg/skaffold/build/gcb/docker_test.go | 2 +- pkg/skaffold/build/gcb/spec.go | 2 +- pkg/skaffold/build/local/docker.go | 4 +- pkg/skaffold/build/local/docker_test.go | 2 +- pkg/skaffold/build/local/local.go | 12 ++- pkg/skaffold/docker/image.go | 6 +- pkg/skaffold/docker/image_test.go | 4 +- 14 files changed, 132 insertions(+), 42 deletions(-) create mode 100644 pkg/skaffold/build/buildpacks/options.go diff --git a/integration/testdata/debug/skaffold.yaml b/integration/testdata/debug/skaffold.yaml index 657d9df186f..7ec93ad849d 100644 --- a/integration/testdata/debug/skaffold.yaml +++ b/integration/testdata/debug/skaffold.yaml @@ -54,5 +54,3 @@ profiles: context: go buildpacks: builder: "gcr.io/buildpacks/builder:v1" - env: - - GOOGLE_GCFLAGS="-gcflags='all=-N -l'" diff --git a/pkg/skaffold/build/buildpacks/build.go b/pkg/skaffold/build/buildpacks/build.go index 9dce2d2a73e..492703a8886 100644 --- a/pkg/skaffold/build/buildpacks/build.go +++ b/pkg/skaffold/build/buildpacks/build.go @@ -26,18 +26,18 @@ import ( // Build builds an artifact with Cloud Native Buildpacks: // https://buildpacks.io/ -func (b *Builder) Build(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { - built, err := b.build(ctx, out, artifact, tag) +func (b *Builder) Build(ctx context.Context, out io.Writer, artifact *latest.Artifact, opts BuildOptions) (string, error) { + built, err := b.build(ctx, out, artifact, opts) if err != nil { return "", err } - if err := b.localDocker.Tag(ctx, built, tag); err != nil { - return "", fmt.Errorf("tagging %s->%q: %w", built, tag, err) + if err := b.localDocker.Tag(ctx, built, opts.Tag); err != nil { + return "", fmt.Errorf("tagging %s->%q: %w", built, opts.Tag, err) } if b.pushImages { - return b.localDocker.Push(ctx, out, tag) + return b.localDocker.Push(ctx, out, opts.Tag) } - return b.localDocker.ImageID(ctx, tag) + return b.localDocker.ImageID(ctx, opts.Tag) } diff --git a/pkg/skaffold/build/buildpacks/build_test.go b/pkg/skaffold/build/buildpacks/build_test.go index 704c50d0282..5f56b7045af 100644 --- a/pkg/skaffold/build/buildpacks/build_test.go +++ b/pkg/skaffold/build/buildpacks/build_test.go @@ -42,7 +42,7 @@ func TestBuild(t *testing.T) { tests := []struct { description string artifact *latest.Artifact - tag string + opts BuildOptions api *testutil.FakeAPIClient files map[string]string pushImages bool @@ -53,7 +53,7 @@ func TestBuild(t *testing.T) { { description: "success", artifact: buildpacksArtifact("my/builder", "my/run"), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, expectedOptions: &pack.BuildOptions{ AppPath: ".", @@ -66,7 +66,7 @@ func TestBuild(t *testing.T) { { description: "success with buildpacks", artifact: withTrustedBuilder(withBuildpacks([]string{"my/buildpack", "my/otherBuildpack"}, buildpacksArtifact("my/otherBuilder", "my/otherRun"))), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, expectedOptions: &pack.BuildOptions{ AppPath: ".", @@ -81,7 +81,7 @@ func TestBuild(t *testing.T) { { description: "project.toml", artifact: buildpacksArtifact("my/builder2", "my/run2"), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, files: map[string]string{ "project.toml": `[[build.env]] @@ -108,7 +108,7 @@ version = "1.0" { description: "Buildpacks in skaffold.yaml override those in project.toml", artifact: withBuildpacks([]string{"my/buildpack", "my/otherBuildpack"}, buildpacksArtifact("my/builder3", "my/run3")), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, files: map[string]string{ "project.toml": `[[build.buildpacks]] @@ -127,7 +127,7 @@ id = "my/ignored" { description: "Combine env from skaffold.yaml and project.toml", artifact: withEnv([]string{"KEY1=VALUE1"}, buildpacksArtifact("my/builder4", "my/run4")), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, files: map[string]string{ "project.toml": `[[build.env]] @@ -149,7 +149,7 @@ value = "VALUE2" { description: "dev mode", artifact: withSync(&latest.Sync{Auto: &latest.Auto{}}, buildpacksArtifact("another/builder", "another/run")), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, devMode: true, expectedOptions: &pack.BuildOptions{ @@ -165,7 +165,7 @@ value = "VALUE2" { description: "dev mode but no sync", artifact: buildpacksArtifact("my/other-builder", "my/run"), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, devMode: true, expectedOptions: &pack.BuildOptions{ @@ -179,14 +179,14 @@ value = "VALUE2" { description: "invalid ref", artifact: buildpacksArtifact("my/builder", "my/run"), - tag: "in valid ref", + opts: BuildOptions{Tag: "in valid ref"}, api: &testutil.FakeAPIClient{}, shouldErr: true, }, { description: "push error", artifact: buildpacksArtifact("my/builder", "my/run"), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, pushImages: true, api: &testutil.FakeAPIClient{ ErrImagePush: true, @@ -196,14 +196,14 @@ value = "VALUE2" { description: "invalid env", artifact: withEnv([]string{"INVALID"}, buildpacksArtifact("my/builder", "my/run")), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, shouldErr: true, }, { description: "invalid project.toml", artifact: buildpacksArtifact("my/builder2", "my/run2"), - tag: "img:tag", + opts: BuildOptions{Tag: "img:tag"}, api: &testutil.FakeAPIClient{}, files: map[string]string{ "project.toml": `INVALID`, @@ -224,7 +224,7 @@ value = "VALUE2" localDocker := docker.NewLocalDaemon(test.api, nil, false, nil) builder := NewArtifactBuilder(localDocker, test.pushImages, test.devMode) - _, err := builder.Build(context.Background(), ioutil.Discard, test.artifact, test.tag) + _, err := builder.Build(context.Background(), ioutil.Discard, test.artifact, test.opts) t.CheckError(test.shouldErr, err) if test.expectedOptions != nil { diff --git a/pkg/skaffold/build/buildpacks/lifecycle.go b/pkg/skaffold/build/buildpacks/lifecycle.go index 98e8439d4d7..61e06c2a3b4 100644 --- a/pkg/skaffold/build/buildpacks/lifecycle.go +++ b/pkg/skaffold/build/buildpacks/lifecycle.go @@ -45,7 +45,7 @@ var ( // to pull the images that are already pulled. var images pulledImages -func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) { +func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, opts BuildOptions) (string, error) { artifact := a.BuildpackArtifact workspace := a.Workspace @@ -59,9 +59,9 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, // To improve caching, we always build the image with [:latest] tag // This way, the lifecycle is able to "bootstrap" from the previously built image. // The image will then be tagged as usual with the tag provided by the tag policy. - parsed, err := docker.ParseReference(tag) + parsed, err := docker.ParseReference(opts.Tag) if err != nil { - return "", fmt.Errorf("parsing tag %q: %w", tag, err) + return "", fmt.Errorf("parsing tag %q: %w", opts.Tag, err) } latest := parsed.BaseName + ":latest" @@ -99,8 +99,7 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, // Does the builder image need to be pulled? alreadyPulled := images.AreAlreadyPulled(artifact.Builder, artifact.RunImage) - - if err := runPackBuildFunc(ctx, out, b.localDocker, pack.BuildOptions{ + packOpts := &pack.BuildOptions{ AppPath: workspace, Builder: artifact.Builder, RunImage: artifact.RunImage, @@ -111,7 +110,10 @@ func (b *Builder) build(ctx context.Context, out io.Writer, a *latest.Artifact, TrustBuilder: artifact.TrustBuilder, // TODO(dgageot): Support project.toml include/exclude. // FileFilter: func(string) bool { return true }, - }); err != nil { + } + + packOpts = opts.ApplyModifiers(packOpts) + if err := runPackBuildFunc(ctx, out, b.localDocker, *packOpts); err != nil { return "", err } diff --git a/pkg/skaffold/build/buildpacks/options.go b/pkg/skaffold/build/buildpacks/options.go new file mode 100644 index 00000000000..5cea237ee4a --- /dev/null +++ b/pkg/skaffold/build/buildpacks/options.go @@ -0,0 +1,82 @@ +/* +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 buildpacks + +import "github.com/buildpacks/pack" + +var buildArgsForDebug = map[string]string{ + "GOOGLE_GOGCFLAGS": "'all=-N -l'", // disable build optimization for Golang + // TODO: Add for other languages +} + +var buildArgsForDev = map[string]string{ + "GOOGLE_GOLDFLAGS": "-w", // omit debug information in build output for Golang + // TODO: Add for other languages +} + +type BuildOptionsModifier func(*pack.BuildOptions) *pack.BuildOptions + +type BuildOptions struct { + Tag string + modifiers []BuildOptionsModifier +} + +func (b *BuildOptions) AddModifier(m BuildOptionsModifier) *BuildOptions { + b.modifiers = append(b.modifiers, m) + return b +} + +func (b *BuildOptions) ApplyModifiers(opts *pack.BuildOptions) *pack.BuildOptions { + for _, modifier := range b.modifiers { + opts = modifier(opts) + } + return opts +} + +func OptimizeBuildForDebug(opts *pack.BuildOptions) *pack.BuildOptions { + if opts == nil { + opts = &pack.BuildOptions{} + } + + if opts.Env == nil { + opts.Env = make(map[string]string) + } + + for k, v := range buildArgsForDebug { + if _, exists := opts.Env[k]; !exists { + opts.Env[k] = v + } + } + return opts +} + +func OptimizeBuildForDev(opts *pack.BuildOptions) *pack.BuildOptions { + if opts == nil { + opts = &pack.BuildOptions{} + } + + if opts.Env == nil { + opts.Env = make(map[string]string) + } + + for k, v := range buildArgsForDev { + if _, exists := opts.Env[k]; !exists { + opts.Env[k] = v + } + } + return opts +} diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index dfd9e0782bf..3b44b1f4c0a 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -56,7 +56,7 @@ func (b *mockBuilder) BuildAndTest(ctx context.Context, out io.Writer, artifacts b.built = append(b.built, artifact) tag := options[i].Tag - _, err := b.dockerDaemon.Build(ctx, out, artifact.Workspace, artifact.DockerArtifact, &docker.BuildOptions{Tag: tag}) + _, err := b.dockerDaemon.Build(ctx, out, artifact.Workspace, artifact.DockerArtifact, docker.BuildOptions{Tag: tag}) if err != nil { return nil, err } diff --git a/pkg/skaffold/build/gcb/docker.go b/pkg/skaffold/build/gcb/docker.go index 1dbe953d146..ad00873fdbe 100644 --- a/pkg/skaffold/build/gcb/docker.go +++ b/pkg/skaffold/build/gcb/docker.go @@ -26,7 +26,7 @@ import ( ) // dockerBuildSpec lists the build steps required to build a docker image. -func (b *Builder) dockerBuildSpec(artifact *latest.DockerArtifact, opts *docker.BuildOptions) (cloudbuild.Build, error) { +func (b *Builder) dockerBuildSpec(artifact *latest.DockerArtifact, opts docker.BuildOptions) (cloudbuild.Build, error) { args, err := b.dockerBuildArgs(artifact, opts) if err != nil { return cloudbuild.Build{}, err @@ -60,7 +60,7 @@ func (b *Builder) cacheFromSteps(artifact *latest.DockerArtifact) []*cloudbuild. } // dockerBuildArgs lists the arguments passed to `docker` to build a given image. -func (b *Builder) dockerBuildArgs(artifact *latest.DockerArtifact, opts *docker.BuildOptions) ([]string, error) { +func (b *Builder) dockerBuildArgs(artifact *latest.DockerArtifact, opts docker.BuildOptions) ([]string, error) { ba, err := docker.GetBuildArgs(artifact, opts) if err != nil { return nil, fmt.Errorf("getting docker build args: %w", err) diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go index ce25aa0aa97..d25fa9efec4 100644 --- a/pkg/skaffold/build/gcb/docker_test.go +++ b/pkg/skaffold/build/gcb/docker_test.go @@ -80,7 +80,7 @@ func TestPullCacheFrom(t *testing.T) { builder := newBuilder(latest.GoogleCloudBuild{ DockerImage: "docker/docker", }) - desc, err := builder.dockerBuildSpec(artifact, &docker.BuildOptions{Tag: "nginx2"}) + desc, err := builder.dockerBuildSpec(artifact, docker.BuildOptions{Tag: "nginx2"}) expected := []*cloudbuild.BuildStep{{ Name: "docker/docker", diff --git a/pkg/skaffold/build/gcb/spec.go b/pkg/skaffold/build/gcb/spec.go index ae3bb351a10..026eceeeba4 100644 --- a/pkg/skaffold/build/gcb/spec.go +++ b/pkg/skaffold/build/gcb/spec.go @@ -59,7 +59,7 @@ func (b *Builder) buildSpecForArtifact(a *latest.Artifact, tag string) (cloudbui return b.kanikoBuildSpec(a.KanikoArtifact, tag) case a.DockerArtifact != nil: - return b.dockerBuildSpec(a.DockerArtifact, &docker.BuildOptions{Tag: tag}) + return b.dockerBuildSpec(a.DockerArtifact, docker.BuildOptions{Tag: tag}) case a.JibArtifact != nil: return b.jibBuildSpec(a, tag) diff --git a/pkg/skaffold/build/local/docker.go b/pkg/skaffold/build/local/docker.go index 04de3250001..3d1814b9d7b 100644 --- a/pkg/skaffold/build/local/docker.go +++ b/pkg/skaffold/build/local/docker.go @@ -29,7 +29,7 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/warnings" ) -func (b *Builder) buildDocker(ctx context.Context, out io.Writer, a *latest.Artifact, opts *docker.BuildOptions) (string, error) { +func (b *Builder) buildDocker(ctx context.Context, out io.Writer, a *latest.Artifact, opts docker.BuildOptions) (string, error) { // Fail fast if the Dockerfile can't be found. dockerfile, err := docker.NormalizeDockerfilePath(a.Workspace, a.DockerArtifact.DockerfilePath) if err != nil { @@ -66,7 +66,7 @@ func (b *Builder) retrieveExtraEnv() []string { return b.localDocker.ExtraEnv() } -func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts *docker.BuildOptions) (string, error) { +func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts docker.BuildOptions) (string, error) { dockerfilePath, err := docker.NormalizeDockerfilePath(workspace, a.DockerfilePath) if err != nil { return "", fmt.Errorf("normalizing dockerfile path: %w", err) diff --git a/pkg/skaffold/build/local/docker_test.go b/pkg/skaffold/build/local/docker_test.go index b5d0fc028e5..ba7f620af56 100644 --- a/pkg/skaffold/build/local/docker_test.go +++ b/pkg/skaffold/build/local/docker_test.go @@ -93,7 +93,7 @@ func TestDockerCLIBuild(t *testing.T) { }, } - _, err = builder.buildDocker(context.Background(), ioutil.Discard, artifact, &docker.BuildOptions{Tag: "tag"}) + _, err = builder.buildDocker(context.Background(), ioutil.Discard, artifact, docker.BuildOptions{Tag: "tag"}) t.CheckNoError(err) }) } diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 3f700ee6968..edcfba0f764 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -86,7 +86,7 @@ func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *lat switch { case a.DockerArtifact != nil: - return b.buildDocker(ctx, out, a, ToDockerOpts(opts)) + return b.buildDocker(ctx, out, a, *ToDockerOpts(opts)) case a.BazelArtifact != nil: return bazel.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages).Build(ctx, out, a, opts.Tag) @@ -98,7 +98,7 @@ func (b *Builder) runBuildForArtifact(ctx context.Context, out io.Writer, a *lat return custom.NewArtifactBuilder(b.localDocker, b.insecureRegistries, b.pushImages, b.retrieveExtraEnv()).Build(ctx, out, a, opts.Tag) case a.BuildpackArtifact != nil: - return buildpacks.NewArtifactBuilder(b.localDocker, b.pushImages, b.devMode).Build(ctx, out, a, opts.Tag) + return buildpacks.NewArtifactBuilder(b.localDocker, b.pushImages, b.devMode).Build(ctx, out, a, *ToBuildPacksOpts(opts)) default: return "", fmt.Errorf("unexpected type %q for local artifact:\n%s", misc.ArtifactType(a), misc.FormatArtifact(a)) @@ -120,3 +120,11 @@ func ToDockerOpts(opts build.BuilderOptions) *docker.BuildOptions { } return d.AddModifier(docker.OptimizeBuildForDev) } + +func ToBuildPacksOpts(opts build.BuilderOptions) *buildpacks.BuildOptions { + d := &buildpacks.BuildOptions{Tag: opts.Tag} + if opts.Configuration == build.Debug { + return d.AddModifier(buildpacks.OptimizeBuildForDebug) + } + return d.AddModifier(buildpacks.OptimizeBuildForDev) +} diff --git a/pkg/skaffold/docker/image.go b/pkg/skaffold/docker/image.go index eadbd12fe2c..f7d02b31822 100644 --- a/pkg/skaffold/docker/image.go +++ b/pkg/skaffold/docker/image.go @@ -61,7 +61,7 @@ type LocalDaemon interface { ExtraEnv() []string ServerVersion(ctx context.Context) (types.Version, error) ConfigFile(ctx context.Context, image string) (*v1.ConfigFile, error) - Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts *BuildOptions) (string, error) + Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts BuildOptions) (string, error) Push(ctx context.Context, out io.Writer, ref string) (string, error) Pull(ctx context.Context, out io.Writer, ref string) error Load(ctx context.Context, out io.Writer, input io.Reader, ref string) (string, error) @@ -155,7 +155,7 @@ func (l *localDaemon) ConfigFile(ctx context.Context, image string) (*v1.ConfigF } // Build performs a docker build and returns the imageID. -func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts *BuildOptions) (string, error) { +func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string, a *latest.DockerArtifact, opts BuildOptions) (string, error) { logrus.Debugf("Running docker build: context: %s, dockerfile: %s", workspace, a.DockerfilePath) // Like `docker build`, we ignore the errors @@ -413,7 +413,7 @@ func (l *localDaemon) ImageRemove(ctx context.Context, image string, opts types. } // GetBuildArgs gives the build args flags for docker build. -func GetBuildArgs(a *latest.DockerArtifact, opts *BuildOptions) ([]string, error) { +func GetBuildArgs(a *latest.DockerArtifact, opts BuildOptions) ([]string, error) { var args []string buildArgs, err := EvaluateBuildArgs(a.BuildArgs) diff --git a/pkg/skaffold/docker/image_test.go b/pkg/skaffold/docker/image_test.go index 2968b9423a4..fb428cf1d96 100644 --- a/pkg/skaffold/docker/image_test.go +++ b/pkg/skaffold/docker/image_test.go @@ -190,7 +190,7 @@ func TestBuild(t *testing.T) { t.SetEnvs(test.env) localDocker := NewLocalDaemon(test.api, nil, false, nil) - _, err := localDocker.Build(context.Background(), ioutil.Discard, test.workspace, test.artifact, &BuildOptions{Tag: "finalimage"}) + _, err := localDocker.Build(context.Background(), ioutil.Discard, test.workspace, test.artifact, BuildOptions{Tag: "finalimage"}) if test.shouldErr { t.CheckErrorContains(test.expectedError, err) @@ -322,7 +322,7 @@ func TestGetBuildArgs(t *testing.T) { testutil.Run(t, test.description, func(t *testutil.T) { t.Override(&util.OSEnviron, func() []string { return test.env }) - result, err := GetBuildArgs(test.artifact, &BuildOptions{Tag: ""}) + result, err := GetBuildArgs(test.artifact, BuildOptions{Tag: ""}) t.CheckError(test.shouldErr, err) if !test.shouldErr { From 538d50b87cfd1ae9470ce409897c8af0b060d5a1 Mon Sep 17 00:00:00 2001 From: gsquared94 Date: Fri, 17 Jul 2020 13:46:09 -0700 Subject: [PATCH 6/6] Add cache tests --- pkg/skaffold/build/cache/hash_test.go | 69 ++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index a1aa60cbb4c..d8db6711368 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -55,6 +55,7 @@ func TestGetHashForArtifact(t *testing.T) { description string dependencies []string artifact *latest.Artifact + opts build.BuilderOptions devMode bool expected string }{ @@ -136,6 +137,72 @@ func TestGetHashForArtifact(t *testing.T) { devMode: true, expected: "d18a2202185518cd139dbdff9be2e4fb972a5de7f2f2448c41b05cb310d1e875", }, + { + description: "build options for dev", + dependencies: []string{"a", "b"}, + artifact: &latest.Artifact{}, + opts: build.BuilderOptions{ + Tag: "tag", + Configuration: build.Dev, + BuildArgs: nil, + }, + expected: "e1f69fa9734709714f69a532a7edcda1cafe8aac165bbccfb28e7dfa7435e525", + }, + { + description: "build options for debug", + dependencies: []string{"a", "b"}, + artifact: &latest.Artifact{}, + opts: build.BuilderOptions{ + Tag: "tag", + Configuration: build.Debug, + BuildArgs: nil, + }, + expected: "121f79bd6d6e80446324aff291bfc4d54da3d5e3eefd6c1d11cd989f1a95cb7a", + }, + { + description: "ignore tag in options", + dependencies: []string{"a", "b"}, + artifact: &latest.Artifact{}, + opts: build.BuilderOptions{ + Tag: "different-tag", + Configuration: build.Debug, + BuildArgs: nil, + }, + expected: "121f79bd6d6e80446324aff291bfc4d54da3d5e3eefd6c1d11cd989f1a95cb7a", + }, + { + description: "options args (set 1)", + dependencies: []string{"a", "b"}, + artifact: &latest.Artifact{}, + opts: build.BuilderOptions{ + Tag: "tag", + Configuration: build.Debug, + BuildArgs: map[string]*string{"one": stringPointer("1"), "two": stringPointer("2")}, + }, + expected: "da5cf2a8a386aafc89ea8ad070f0d5f8eee9046fb4ea641cd3da54c2a6139e80", + }, + { + description: "options args (set 1, different order)", + dependencies: []string{"a", "b"}, + artifact: &latest.Artifact{}, + opts: build.BuilderOptions{ + Tag: "tag", + Configuration: build.Debug, + BuildArgs: map[string]*string{"two": stringPointer("2"), "one": stringPointer("1")}, + }, + expected: "da5cf2a8a386aafc89ea8ad070f0d5f8eee9046fb4ea641cd3da54c2a6139e80", + }, + { + description: "options args (set 2)", + dependencies: []string{"a", "b"}, + artifact: &latest.Artifact{}, + opts: build.BuilderOptions{ + Tag: "tag", + Configuration: build.Debug, + BuildArgs: map[string]*string{"two": stringPointer("2"), "three": stringPointer("3")}, + }, + expected: "1543293a81f8724a7552b4df49143a87b2be33a40fc3940bef790b7ddfaf7839", + }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { @@ -143,7 +210,7 @@ func TestGetHashForArtifact(t *testing.T) { t.Override(&artifactConfigFunction, fakeArtifactConfig) depLister := stubDependencyLister(test.dependencies) - actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, build.BuilderOptions{}, test.devMode) + actual, err := getHashForArtifact(context.Background(), depLister, test.artifact, test.opts, test.devMode) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual)