Skip to content

Simplify docker related code. #854

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

Merged
merged 1 commit into from
Jul 27, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
9 changes: 5 additions & 4 deletions pkg/skaffold/bazel/bazel.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@ func query(target string) string {
}

// GetDependencies finds the sources dependencies for the given bazel artifact.
func GetDependencies(a *v1alpha2.Artifact) ([]string, error) {
cmd := exec.Command("bazel", "query", query(a.BazelArtifact.BuildTarget), "--noimplicit_deps", "--order_output=no")
cmd.Dir = a.Workspace
// All paths are relative to the workspace.
func GetDependencies(workspace string, a *v1alpha2.BazelArtifact) ([]string, error) {
cmd := exec.Command("bazel", "query", query(a.BuildTarget), "--noimplicit_deps", "--order_output=no")
cmd.Dir = workspace
stdout, err := util.RunCmdOut(cmd)
if err != nil {
return nil, errors.Wrap(err, "getting bazel dependencies")
Expand All @@ -59,7 +60,7 @@ func GetDependencies(a *v1alpha2.Artifact) ([]string, error) {
deps = append(deps, depToPath(l))
}

if _, err := os.Stat(filepath.Join(a.Workspace, "WORKSPACE")); err == nil {
if _, err := os.Stat(filepath.Join(workspace, "WORKSPACE")); err == nil {
deps = append(deps, "WORKSPACE")
}

Expand Down
41 changes: 0 additions & 41 deletions pkg/skaffold/build/deps.go

This file was deleted.

2 changes: 1 addition & 1 deletion pkg/skaffold/build/gcb/container_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, tagger tag.T
}

fmt.Fprintf(out, "Pushing code to gs://%s/%s\n", cbBucket, buildObject)
if err := docker.UploadContextToGCS(ctx, artifact.Workspace, artifact.DockerArtifact.DockerfilePath, cbBucket, buildObject); err != nil {
if err := docker.UploadContextToGCS(ctx, artifact.Workspace, artifact.DockerArtifact, cbBucket, buildObject); err != nil {
return "", errors.Wrap(err, "uploading source tarball")
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/build/kaniko/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func runKaniko(ctx context.Context, out io.Writer, artifact *v1alpha2.Artifact,

initialTag := util.RandomID()
tarName := fmt.Sprintf("context-%s.tar.gz", initialTag)
if err := docker.UploadContextToGCS(ctx, artifact.Workspace, dockerfilePath, cfg.GCSBucket, tarName); err != nil {
if err := docker.UploadContextToGCS(ctx, artifact.Workspace, artifact.DockerArtifact, cfg.GCSBucket, tarName); err != nil {
return "", errors.Wrap(err, "uploading tar to gcs")
}
defer gcsDelete(ctx, cfg.GCSBucket, tarName)
Expand Down
9 changes: 1 addition & 8 deletions pkg/skaffold/build/local/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/docker/docker/api/types"
"github.com/pkg/errors"
)

Expand Down Expand Up @@ -56,13 +55,7 @@ func (b *Builder) buildDocker(ctx context.Context, out io.Writer, workspace stri
return "", errors.Wrap(err, "running build")
}
} else {
err := docker.RunBuild(ctx, out, b.api, workspace, types.ImageBuildOptions{
Tags: []string{initialTag},
Dockerfile: a.DockerfilePath,
BuildArgs: a.BuildArgs,
CacheFrom: a.CacheFrom,
})
if err != nil {
if err := docker.BuildArtifact(ctx, out, b.api, workspace, a, initialTag); err != nil {
return "", errors.Wrap(err, "running build")
}
}
Expand Down
21 changes: 13 additions & 8 deletions pkg/skaffold/docker/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strings"

cstorage "cloud.google.com/go/storage"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/pkg/errors"
)
Expand All @@ -39,37 +40,41 @@ func NormalizeDockerfilePath(context, dockerfile string) (string, error) {
return filepath.Abs(dockerfile)
}

func CreateDockerTarContext(buildArgs map[string]*string, w io.Writer, context, dockerfilePath string) error {
paths, err := GetDependencies(buildArgs, context, dockerfilePath)
func CreateDockerTarContext(w io.Writer, workspace string, a *v1alpha2.DockerArtifact) error {
paths, err := GetDependencies(workspace, a)
if err != nil {
return errors.Wrap(err, "getting relative tar paths")
}
if err := util.CreateTar(w, context, paths); err != nil {

if err := util.CreateTar(w, workspace, paths); err != nil {
return errors.Wrap(err, "creating tar gz")
}

return nil
}

func CreateDockerTarGzContext(buildArgs map[string]*string, w io.Writer, context, dockerfilePath string) error {
paths, err := GetDependencies(buildArgs, context, dockerfilePath)
func CreateDockerTarGzContext(w io.Writer, workspace string, a *v1alpha2.DockerArtifact) error {
paths, err := GetDependencies(workspace, a)
if err != nil {
return errors.Wrap(err, "getting relative tar paths")
}
if err := util.CreateTarGz(w, context, paths); err != nil {

if err := util.CreateTarGz(w, workspace, paths); err != nil {
return errors.Wrap(err, "creating tar gz")
}

return nil
}

func UploadContextToGCS(ctx context.Context, context, dockerfilePath, bucket, objectName string) error {
func UploadContextToGCS(ctx context.Context, workspace string, a *v1alpha2.DockerArtifact, bucket, objectName string) error {
c, err := cstorage.NewClient(ctx)
if err != nil {
return err
}
defer c.Close()

w := c.Bucket(bucket).Object(objectName).NewWriter(ctx)
if err := CreateDockerTarGzContext(map[string]*string{}, w, context, dockerfilePath); err != nil {
if err := CreateDockerTarGzContext(w, workspace, a); err != nil {
return errors.Wrap(err, "uploading targz to google storage")
}
return w.Close()
Expand Down
8 changes: 7 additions & 1 deletion pkg/skaffold/docker/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"path/filepath"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
"github.com/GoogleContainerTools/skaffold/testutil"
)

Expand All @@ -36,6 +37,11 @@ func TestDockerContext(t *testing.T) {
RetrieveImage = retrieveImage
}()

artifact := &v1alpha2.DockerArtifact{
DockerfilePath: "Dockerfile",
BuildArgs: map[string]*string{},
}

os.Mkdir(filepath.Join(tmpDir, "files"), 0750)
ioutil.WriteFile(filepath.Join(tmpDir, "files", "ignored.txt"), []byte(""), 0644)
ioutil.WriteFile(filepath.Join(tmpDir, "files", "included.txt"), []byte(""), 0644)
Expand All @@ -45,7 +51,7 @@ func TestDockerContext(t *testing.T) {
ioutil.WriteFile(filepath.Join(tmpDir, "alsoignored.txt"), []byte(""), 0644)
reader, writer := io.Pipe()
go func() {
err := CreateDockerTarContext(map[string]*string{}, writer, tmpDir, "Dockerfile")
err := CreateDockerTarContext(writer, tmpDir, artifact)
if err != nil {
writer.CloseWithError(err)
} else {
Expand Down
18 changes: 12 additions & 6 deletions pkg/skaffold/docker/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,17 @@ import (
"github.com/sirupsen/logrus"
)

// RunBuild performs a docker build and returns nothing
func RunBuild(ctx context.Context, out io.Writer, cli APIClient, workspace string, opts types.ImageBuildOptions) error {
logrus.Debugf("Running docker build: context: %s, dockerfile: %s", workspace, opts.Dockerfile)
// BuildArtifact performs a docker build and returns nothing
func BuildArtifact(ctx context.Context, out io.Writer, cli APIClient, workspace string, a *v1alpha2.DockerArtifact, initialTag string) error {
logrus.Debugf("Running docker build: context: %s, dockerfile: %s", workspace, a.DockerfilePath)

// Like `docker build`, we ignore the errors
// See https://github.com/docker/cli/blob/75c1bb1f33d7cedbaf48404597d5bf9818199480/cli/command/image/build.go#L364
authConfigs, _ := DefaultAuthHelper.GetAllAuthConfigs()
opts.AuthConfigs = authConfigs

buildCtx, buildCtxWriter := io.Pipe()
go func() {
err := CreateDockerTarContext(opts.BuildArgs, buildCtxWriter, workspace, opts.Dockerfile)
err := CreateDockerTarContext(buildCtxWriter, workspace, a)
if err != nil {
buildCtxWriter.CloseWithError(errors.Wrap(err, "creating docker context"))
return
Expand All @@ -61,11 +60,18 @@ func RunBuild(ctx context.Context, out io.Writer, cli APIClient, workspace strin
progressOutput := streamformatter.NewProgressOutput(out)
body := progress.NewProgressReader(buildCtx, progressOutput, 0, "", "Sending build context to Docker daemon")

resp, err := cli.ImageBuild(ctx, body, opts)
resp, err := cli.ImageBuild(ctx, body, types.ImageBuildOptions{
Tags: []string{initialTag},
Dockerfile: a.DockerfilePath,
BuildArgs: a.BuildArgs,
CacheFrom: a.CacheFrom,
AuthConfigs: authConfigs,
})
if err != nil {
return errors.Wrap(err, "docker build")
}
defer resp.Body.Close()

return StreamDockerMessages(out, resp.Body)
}

Expand Down
15 changes: 3 additions & 12 deletions pkg/skaffold/docker/image_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@ import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
"github.com/GoogleContainerTools/skaffold/testutil"
"github.com/docker/docker/api/types"
"github.com/google/go-cmp/cmp"
)

Expand Down Expand Up @@ -96,7 +94,7 @@ func TestRunPush(t *testing.T) {
}
}

func TestRunBuild(t *testing.T) {
func TestRunBuildArtifact(t *testing.T) {
var tests = []testImageAPI{
{
description: "build",
Expand All @@ -122,16 +120,9 @@ func TestRunBuild(t *testing.T) {
}
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
tmp, cleanup := testutil.TempDir(t)
defer cleanup()

ioutil.WriteFile(filepath.Join(tmp, "Dockerfile"), []byte{}, os.ModePerm)

api := testutil.NewFakeImageAPIClient(test.tagToImageID, test.testOpts)
err := RunBuild(context.Background(), ioutil.Discard, api, tmp, types.ImageBuildOptions{
Dockerfile: "Dockerfile",
Tags: []string{"finalimage"},
})

err := BuildArtifact(context.Background(), ioutil.Discard, api, ".", &v1alpha2.DockerArtifact{}, "finalimage")

testutil.CheckError(t, test.shouldErr, err)
})
Expand Down
13 changes: 8 additions & 5 deletions pkg/skaffold/docker/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"strings"
"sync"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
"github.com/docker/docker/builder/dockerignore"
"github.com/docker/docker/pkg/fileutils"
"github.com/google/go-containerregistry/pkg/v1"
Expand Down Expand Up @@ -168,13 +169,15 @@ func readDockerfile(workspace, absDockerfilePath string, buildArgs map[string]*s
return deps, nil
}

func GetDependencies(buildArgs map[string]*string, workspace, dockerfilePath string) ([]string, error) {
absDockerfilePath, err := NormalizeDockerfilePath(workspace, dockerfilePath)
// GetDependencies finds the sources dependencies for the given docker artifact.
// All paths are relative to the workspace.
func GetDependencies(workspace string, a *v1alpha2.DockerArtifact) ([]string, error) {
absDockerfilePath, err := NormalizeDockerfilePath(workspace, a.DockerfilePath)
if err != nil {
return nil, errors.Wrap(err, "normalizing dockerfile path")
}

deps, err := readDockerfile(workspace, absDockerfilePath, buildArgs)
deps, err := readDockerfile(workspace, absDockerfilePath, a.BuildArgs)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -252,8 +255,8 @@ func GetDependencies(buildArgs map[string]*string, workspace, dockerfilePath str
}

// Always add dockerfile even if it's .dockerignored. The daemon will need it anyways.
if !filepath.IsAbs(dockerfilePath) {
files[dockerfilePath] = true
if !filepath.IsAbs(a.DockerfilePath) {
files[a.DockerfilePath] = true
} else {
files[absDockerfilePath] = true
}
Expand Down
7 changes: 6 additions & 1 deletion pkg/skaffold/docker/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"path/filepath"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
"github.com/GoogleContainerTools/skaffold/testutil"
"github.com/google/go-containerregistry/pkg/v1"
)
Expand Down Expand Up @@ -307,7 +308,11 @@ func TestGetDependencies(t *testing.T) {
ioutil.WriteFile(filepath.Join(workspace, ".dockerignore"), []byte(test.ignore), 0644)
}

deps, err := GetDependencies(test.buildArgs, workspace, "Dockerfile")
deps, err := GetDependencies(workspace, &v1alpha2.DockerArtifact{
BuildArgs: test.buildArgs,
DockerfilePath: "Dockerfile",
})

testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, deps)
})
}
Expand Down
20 changes: 18 additions & 2 deletions pkg/skaffold/watch/artifacts.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ package watch

import (
"context"
"fmt"
"path/filepath"
"strings"
"time"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/bazel"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha2"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -112,7 +114,7 @@ func matchesAny(artifact *v1alpha2.Artifact, files []string) (bool, error) {
return false, nil
}

deps, err := build.DependenciesForArtifact(artifact)
deps, err := dependenciesForArtifact(artifact)
if err != nil {
return false, errors.Wrap(err, "getting dependencies for artifact")
}
Expand All @@ -128,6 +130,20 @@ func matchesAny(artifact *v1alpha2.Artifact, files []string) (bool, error) {
return false, nil
}

// All paths are relative to the workspace.
func dependenciesForArtifact(a *v1alpha2.Artifact) ([]string, error) {
switch {
case a.DockerArtifact != nil:
return docker.GetDependencies(a.Workspace, a.DockerArtifact)

case a.BazelArtifact != nil:
return bazel.GetDependencies(a.Workspace, a.BazelArtifact)

default:
return nil, fmt.Errorf("undefined artifact type: %+v", a.ArtifactType)
}
}

// inWorkspace filters out the paths that are not in the artifact's workspace.
func inWorkspace(artifact *v1alpha2.Artifact, files []string) ([]string, error) {
var inWorkspace []string
Expand Down