Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ImageOptions to artifact builder. #4467

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/skaffold/app/cmd/debug.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand Down
7 changes: 6 additions & 1 deletion integration/examples/getting-started/Dockerfile
Original file line number Diff line number Diff line change
@@ -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
gsquared94 marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
4 changes: 2 additions & 2 deletions integration/testdata/debug/go/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -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 .
gsquared94 marked this conversation as resolved.
Show resolved Hide resolved

FROM gcr.io/distroless/base
# `skaffold debug` uses GOTRACEBACK as an indicator of the Go runtime
Expand Down
3 changes: 0 additions & 3 deletions integration/testdata/debug/skaffold.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ build:
context: python3
- image: skaffold-debug-go
context: go
docker:
buildArgs:
GOGCFLAGS: "-gcflags='all=-N -l'"

deploy:
kubectl:
Expand Down
7 changes: 4 additions & 3 deletions pkg/skaffold/build/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
12 changes: 11 additions & 1 deletion pkg/skaffold/build/cache/hash.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -89,6 +90,15 @@ func getHashForArtifact(ctx context.Context, depLister DependencyLister, a *late
inputs = append(inputs, evaluatedEnv...)
}

// add image options hash
gsquared94 marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
17 changes: 9 additions & 8 deletions pkg/skaffold/build/cache/hash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)

Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
11 changes: 6 additions & 5 deletions pkg/skaffold/build/cache/lookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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()
}()
}
Expand All @@ -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)}
}
Expand All @@ -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 {
Expand Down
13 changes: 7 additions & 6 deletions pkg/skaffold/build/cache/lookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
2 changes: 1 addition & 1 deletion pkg/skaffold/build/cache/retrieve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/skaffold/build/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
gsquared94 marked this conversation as resolved.
Show resolved Hide resolved
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) {
Expand Down
10 changes: 5 additions & 5 deletions pkg/skaffold/build/gcb/cloud_build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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) {
Expand Down
12 changes: 6 additions & 6 deletions pkg/skaffold/build/gcb/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}

Expand All @@ -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, ".")

Expand Down
3 changes: 2 additions & 1 deletion pkg/skaffold/build/gcb/docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down
Loading