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 5 commits
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
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 SKAFFOLD_GO_GCFLAGS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't die on this hill, but this is really supposed to be the most minimal possible example, so this is kind of straying from that. maybe these can go in another example for debug? I don't care too much though

ARG SKAFFOLD_GO_LDFLAGS

COPY main.go .
RUN go build -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
Expand Down
7 changes: 4 additions & 3 deletions integration/testdata/debug/go/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
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 SKAFFOLD_GO_GCFLAGS
ARG SKAFFOLD_LD_GCFLAGS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be SKAFFOLD_GO_LDFLAGS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 🤦🏽‍♂️

# 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
Expand Down
5 changes: 0 additions & 5 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 Expand Up @@ -57,5 +54,3 @@ profiles:
context: go
buildpacks:
builder: "gcr.io/buildpacks/builder:v1"
env:
- GOOGLE_GCFLAGS="-gcflags='all=-N -l'"
3 changes: 1 addition & 2 deletions pkg/skaffold/build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"io"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
)

Expand All @@ -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
Expand Down
12 changes: 6 additions & 6 deletions pkg/skaffold/build/buildpacks/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
26 changes: 13 additions & 13 deletions pkg/skaffold/build/buildpacks/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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: ".",
Expand All @@ -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: ".",
Expand All @@ -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]]
Expand All @@ -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]]
Expand All @@ -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]]
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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,
Expand All @@ -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`,
Expand All @@ -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 {
Expand Down
14 changes: 8 additions & 6 deletions pkg/skaffold/build/buildpacks/lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

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

Expand Down Expand Up @@ -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,
Expand All @@ -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
}

Expand Down
82 changes: 82 additions & 0 deletions pkg/skaffold/build/buildpacks/options.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
Copyright 2019 The Skaffold Authors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Copyright 2019 The Skaffold Authors
Copyright 2020 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
}
7 changes: 4 additions & 3 deletions pkg/skaffold/build/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
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"
Expand All @@ -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.BuilderOptions) (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.BuilderOptions) (string, error) {
return getHashForArtifact(ctx, dependencies, a, opts, runCtx.Opts.IsDevMode())
},
}, nil
}
Expand Down
10 changes: 9 additions & 1 deletion pkg/skaffold/build/cache/hash.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (

"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"
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.BuilderOptions, devMode bool) (string, error) {
var inputs []string

// Append the artifact's configuration
Expand Down Expand Up @@ -89,6 +90,13 @@ 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
h, err := opts.Hash()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it would be nice if builders that have no change between build, dev, and debug would not need a rebuild. So buildpacks would need a rebuild between build and dev, but docker wouldn't.

if err != nil {
return "", fmt.Errorf("evaluating build args: %w", err)
}
inputs = append(inputs, h)

// get a key for the hashes
hasher := sha256.New()
enc := json.NewEncoder(hasher)
Expand Down
Loading