Skip to content

Commit

Permalink
Move key from dockerfilePath to artifact name
Browse files Browse the repository at this point in the history
  • Loading branch information
tejal29 committed Dec 8, 2020
1 parent a380575 commit 62b2d0b
Show file tree
Hide file tree
Showing 18 changed files with 79 additions and 66 deletions.
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 @@ -66,7 +66,7 @@ func (b *mockBuilder) BuildAndTest(ctx context.Context, out io.Writer, tags tag.
b.built = append(b.built, artifact)
tag := tags[artifact.ImageName]
opts := docker.BuildOptions{Tag: tag, Mode: config.RunModes.Dev}
_, err := b.dockerDaemon.Build(ctx, out, artifact.Workspace, artifact.DockerArtifact, opts)
_, err := b.dockerDaemon.Build(ctx, out, artifact.Workspace, artifact.ImageName, artifact.DockerArtifact, opts)
if err != nil {
return nil, err
}
Expand Down
6 changes: 2 additions & 4 deletions pkg/skaffold/build/cluster/kaniko.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,8 @@ func (b *Builder) copyKanikoBuildContext(ctx context.Context, workspace string,

buildCtx, buildCtxWriter := io.Pipe()
go func() {
err := docker.CreateDockerTarContext(ctx, buildCtxWriter, workspace, &latest.DockerArtifact{
BuildArgs: artifact.BuildArgs,
DockerfilePath: artifact.DockerfilePath,
}, b.cfg)
err := docker.CreateDockerTarContext(ctx, buildCtxWriter, docker.NewBuildConfig(
workspace, artifact.Image, artifact.DockerfilePath, artifact.BuildArgs), b.cfg)
if err != nil {
buildCtxWriter.CloseWithError(fmt.Errorf("creating docker context: %w", err))
return
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/build/custom/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ import (
)

// GetDependencies returns dependencies listed for a custom artifact
func GetDependencies(ctx context.Context, workspace string, a *latest.CustomArtifact, cfg docker.Config) ([]string, error) {
func GetDependencies(ctx context.Context, workspace string, artifactName string, a *latest.CustomArtifact, cfg docker.Config) ([]string, error) {
switch {
case a.Dependencies.Dockerfile != nil:
dockerfile := a.Dependencies.Dockerfile
return docker.GetDependencies(ctx, workspace, dockerfile.Path, dockerfile.BuildArgs, cfg)
return docker.GetDependencies(ctx, docker.NewBuildConfig(workspace, artifactName, dockerfile.Path, dockerfile.BuildArgs), cfg)

case a.Dependencies.Command != "":
split := strings.Split(a.Dependencies.Command, " ")
Expand Down
6 changes: 3 additions & 3 deletions pkg/skaffold/build/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ func DependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker
if evalErr != nil {
return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr)
}
paths, err = docker.GetDependencies(ctx, a.Workspace, a.DockerArtifact.DockerfilePath, args, cfg)
paths, err = docker.GetDependencies(ctx, docker.NewBuildConfig(a.Workspace, a.ImageName, a.DockerArtifact.DockerfilePath, args), cfg)

case a.KanikoArtifact != nil:
deps := docker.ResolveDependencyImages(a.Dependencies, r, false)
args, evalErr := docker.EvalBuildArgs(cfg.Mode(), a.Workspace, a.KanikoArtifact.DockerfilePath, a.KanikoArtifact.BuildArgs, deps)
if evalErr != nil {
return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr)
}
paths, err = docker.GetDependencies(ctx, a.Workspace, a.KanikoArtifact.DockerfilePath, args, cfg)
paths, err = docker.GetDependencies(ctx, docker.NewBuildConfig(a.Workspace, a.ImageName, a.KanikoArtifact.DockerfilePath, args), cfg)

case a.BazelArtifact != nil:
paths, err = bazel.GetDependencies(ctx, a.Workspace, a.BazelArtifact)
Expand All @@ -65,7 +65,7 @@ func DependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker
paths, err = jib.GetDependencies(ctx, a.Workspace, a.JibArtifact)

case a.CustomArtifact != nil:
paths, err = custom.GetDependencies(ctx, a.Workspace, a.CustomArtifact, cfg)
paths, err = custom.GetDependencies(ctx, a.Workspace, a.ImageName, a.CustomArtifact, cfg)

case a.BuildpackArtifact != nil:
paths, err = buildpacks.GetDependencies(ctx, a.Workspace, a.BuildpackArtifact)
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/build/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact,
if b.useCLI {
imageID, err = b.dockerCLIBuild(ctx, color.GetWriter(out), a.Workspace, dockerfile, a.ArtifactType.DockerArtifact, opts)
} else {
imageID, err = b.localDocker.Build(ctx, out, a.Workspace, a.ArtifactType.DockerArtifact, opts)
imageID, err = b.localDocker.Build(ctx, out, a.Workspace, a.ImageName, a.ArtifactType.DockerArtifact, opts)
}

if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion pkg/skaffold/diagnose/diagnose.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ func timeToComputeMTimes(deps []string) (time.Duration, error) {
func sizeOfDockerContext(ctx context.Context, a *latest.Artifact, cfg docker.Config) (int64, error) {
buildCtx, buildCtxWriter := io.Pipe()
go func() {
err := docker.CreateDockerTarContext(ctx, buildCtxWriter, a.Workspace, a.DockerArtifact, cfg)
err := docker.CreateDockerTarContext(ctx, buildCtxWriter, docker.NewBuildConfig(
a.Workspace, a.ImageName, a.DockerArtifact.DockerfilePath, nil), cfg)
if err != nil {
buildCtxWriter.CloseWithError(fmt.Errorf("creating docker context: %w", err))
return
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/docker/build_args.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,11 @@ func evalBuildArgs(mode config.RunMode, workspace string, dockerfilePath string,

absDockerfilePath, err := NormalizeDockerfilePath(workspace, dockerfilePath)
if err != nil {
return nil, fmt.Errorf("normalizing dockerfile path: %w", err)
return nil, fmt.Errorf("normalizing dockerfilePath path: %w", err)
}
f, err := os.Open(absDockerfilePath)
if err != nil {
return nil, fmt.Errorf("reading dockerfile: %w", err)
return nil, fmt.Errorf("reading dockerfilePath: %w", err)
}
defer f.Close()
result, err = filterUnusedBuildArgs(f, result)
Expand Down
9 changes: 4 additions & 5 deletions pkg/skaffold/docker/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,21 @@ import (
"io"
"path/filepath"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
)

func CreateDockerTarContext(ctx context.Context, w io.Writer, workspace string, a *latest.DockerArtifact, cfg Config) error {
paths, err := GetDependenciesCached(ctx, workspace, a.DockerfilePath, a.BuildArgs, cfg)
func CreateDockerTarContext(ctx context.Context, w io.Writer, buildCfg BuildConfig, cfg Config) error {
paths, err := GetDependenciesCached(ctx, buildCfg, cfg)
if err != nil {
return fmt.Errorf("getting relative tar paths: %w", err)
}

var p []string
for _, path := range paths {
p = append(p, filepath.Join(workspace, path))
p = append(p, filepath.Join(buildCfg.workspace, path))
}

if err := util.CreateTar(w, workspace, p); err != nil {
if err := util.CreateTar(w, buildCfg.workspace, p); err != nil {
return fmt.Errorf("creating tar gz: %w", err)
}

Expand Down
41 changes: 29 additions & 12 deletions pkg/skaffold/docker/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,24 @@ var (
dependencyCache = util.NewSyncStore()
)

// NormalizeDockerfilePath returns the absolute path to the dockerfile.
// BuildConfig encapsulates all the build configuration required for performing a dockerbuild.
type BuildConfig struct {
workspace string
artifact string
dockerfilePath string
args map[string]*string
}

// NewBuildConfig returns a `BuildConfig` for a dockerfilePath build.
func NewBuildConfig(ws string, a string, path string, args map[string]*string) BuildConfig {
return BuildConfig{
workspace: ws,
artifact: a,
dockerfilePath: path,
args: args,
}
}
// NormalizeDockerfilePath returns the absolute path to the dockerfilePath.
func NormalizeDockerfilePath(context, dockerfile string) (string, error) {
// Expected case: should be found relative to the context directory.
// If it does not exist, check if it's found relative to the current directory in case it's shared.
Expand All @@ -50,26 +67,26 @@ func NormalizeDockerfilePath(context, dockerfile string) (string, error) {
// GetDependencies finds the sources dependency for the given docker artifact.
// it caches the results for the computed dependency which can be used by `GetDependenciesCached`
// All paths are relative to the workspace.
func GetDependencies(ctx context.Context, workspace string, dockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) {
absDockerfilePath, err := NormalizeDockerfilePath(workspace, dockerfilePath)
func GetDependencies(ctx context.Context, buildCfg BuildConfig, cfg Config) ([]string, error) {
absDockerfilePath, err := NormalizeDockerfilePath(buildCfg.workspace, buildCfg.dockerfilePath)
if err != nil {
return nil, fmt.Errorf("normalizing dockerfile path: %w", err)
return nil, fmt.Errorf("normalizing dockerfilePath path: %w", err)
}
result := getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg)
dependencyCache.Store(absDockerfilePath, result)
result := getDependencies(buildCfg.workspace, buildCfg.dockerfilePath, absDockerfilePath, buildCfg.args, cfg)
dependencyCache.Store(buildCfg.artifact, result)
return resultPair(result)
}

// GetDependenciesCached reads from cache finds the sources dependency for the given docker artifact.
// All paths are relative to the workspace.
func GetDependenciesCached(ctx context.Context, workspace string, dockerfilePath string, buildArgs map[string]*string, cfg Config) ([]string, error) {
absDockerfilePath, err := NormalizeDockerfilePath(workspace, dockerfilePath)
func GetDependenciesCached(ctx context.Context, buildCfg BuildConfig, cfg Config) ([]string, error) {
absDockerfilePath, err := NormalizeDockerfilePath(buildCfg.workspace, buildCfg.dockerfilePath)
if err != nil {
return nil, fmt.Errorf("normalizing dockerfile path: %w", err)
return nil, fmt.Errorf("normalizing dockerfilePath path: %w", err)
}

deps := dependencyCache.Exec(absDockerfilePath, func() interface{} {
return getDependencies(workspace, dockerfilePath, absDockerfilePath, buildArgs, cfg)
deps := dependencyCache.Exec(buildCfg.artifact, func() interface{} {
return getDependencies(buildCfg.workspace, buildCfg.dockerfilePath, absDockerfilePath, buildCfg.args, cfg)
})
return resultPair(deps)
}
Expand Down Expand Up @@ -114,7 +131,7 @@ func getDependencies(workspace string, dockerfilePath string, absDockerfilePath
return fmt.Errorf("walking workspace: %w", err)
}

// Always add dockerfile even if it's .dockerignored. The daemon will need it anyways.
// Always add dockerfilePath even if it's .dockerignored. The daemon will need it anyways.
if !filepath.IsAbs(dockerfilePath) {
files[dockerfilePath] = true
} else {
Expand Down
22 changes: 11 additions & 11 deletions pkg/skaffold/docker/dependencies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -301,13 +301,13 @@ func TestGetDependencies(t *testing.T) {
expected: []string{"Dockerfile"},
},
{
description: "multistage dockerfile",
description: "multistage dockerfilePath",
dockerfile: multiStageDockerfile1,
workspace: "",
expected: []string{"Dockerfile", "worker.go"},
},
{
description: "multistage dockerfile with source dependencies in both stages",
description: "multistage dockerfilePath with source dependencies in both stages",
dockerfile: multiStageDockerfile2,
workspace: "",
expected: []string{"Dockerfile", "server.go", "worker.go"},
Expand Down Expand Up @@ -344,7 +344,7 @@ func TestGetDependencies(t *testing.T) {
expected: []string{".dot", "Dockerfile", "file", "server.go", "test.conf", "worker.go"},
},
{
description: "dockerignore dockerfile",
description: "dockerignore dockerfilePath",
dockerfile: copyServerGo,
ignore: "Dockerfile",
workspace: ".",
Expand Down Expand Up @@ -543,7 +543,7 @@ func TestGetDependencies(t *testing.T) {
expected: []string{".dot", "Dockerfile", "Dockerfile.dockerignore", "file", "server.go", "test.conf", "worker.go"},
},
{
description: "invalid dockerfile",
description: "invalid dockerfilePath",
dockerfile: invalidFrom,
workspace: ".",
shouldErr: true,
Expand Down Expand Up @@ -577,7 +577,7 @@ func TestGetDependencies(t *testing.T) {
}

workspace := tmpDir.Path(test.workspace)
deps, err := GetDependencies(context.Background(), workspace, "Dockerfile", test.buildArgs, nil)
deps, err := GetDependencies(context.Background(), NewBuildConfig(workspace, "test", "Dockerfile", test.buildArgs), nil)

t.CheckError(test.shouldErr, err)
t.CheckDeepEqual(test.expected, deps)
Expand All @@ -594,31 +594,31 @@ func TestNormalizeDockerfilePath(t *testing.T) {
expected string // relative path
}{
{
description: "dockerfile found in context",
description: "dockerfilePath found in context",
files: []string{"Dockerfile", "context/Dockerfile"},
dockerfile: "Dockerfile",
expected: "context/Dockerfile",
},
{
description: "path to dockerfile resolved in context first",
description: "path to dockerfilePath resolved in context first",
files: []string{"context/context/Dockerfile", "context/Dockerfile"},
dockerfile: "context/Dockerfile",
expected: "context/context/Dockerfile",
},
{
description: "path to dockerfile in working directory",
description: "path to dockerfilePath in working directory",
files: []string{"context/Dockerfile"},
dockerfile: "context/Dockerfile",
expected: "context/Dockerfile",
},
{
description: "workspace dockerfile when missing in context",
description: "workspace dockerfilePath when missing in context",
files: []string{"Dockerfile", "context/randomfile.txt"},
dockerfile: "Dockerfile",
expected: "Dockerfile",
},
{
description: "explicit dockerfile path",
description: "explicit dockerfilePath path",
files: []string{"context/Dockerfile", "elsewhere/Dockerfile"},
dockerfile: "elsewhere/Dockerfile",
expected: "elsewhere/Dockerfile",
Expand Down Expand Up @@ -686,7 +686,7 @@ func TestGetDependenciesCached(t *testing.T) {
shouldErr: true,
},
{
description: "with cached results for dockerfile in another app",
description: "with cached results for dockerfilePath in another app",
retrieveImgMock: imageFetcher.fetch,
dependencyCache: map[string]interface{}{
filepath.Join("app", "Dockerfile"): []string{"random.go"}},
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/docker/docker_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (c ArtifactConfig) ConfiguredImage() string {
return ""
}

// Path returns the path to the dockerfile
// Path returns the path to the dockerfilePath
func (c ArtifactConfig) Path() string {
return c.File
}
Expand All @@ -87,7 +87,7 @@ func validate(path string) bool {
return false
}

// validate each node contains valid dockerfile directive
// validate each node contains valid dockerfilePath directive
for _, child := range res.AST.Children {
_, ok := command.Commands[child.Value]
if !ok {
Expand Down
12 changes: 5 additions & 7 deletions pkg/skaffold/docker/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,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, artifact 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)
Expand Down Expand Up @@ -174,8 +174,8 @@ func (l *localDaemon) CheckCompatible(a *latest.DockerArtifact) error {
}

// 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) {
logrus.Debugf("Running docker build: context: %s, dockerfile: %s", workspace, a.DockerfilePath)
func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string, artifact string, a *latest.DockerArtifact, opts BuildOptions) (string, error) {
logrus.Debugf("Running docker build: context: %s, dockerfilePath: %s", workspace, a.DockerfilePath)

if err := l.CheckCompatible(a); err != nil {
return "", err
Expand All @@ -191,10 +191,8 @@ func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string

buildCtx, buildCtxWriter := io.Pipe()
go func() {
err := CreateDockerTarContext(ctx, buildCtxWriter, workspace, &latest.DockerArtifact{
DockerfilePath: a.DockerfilePath,
BuildArgs: buildArgs,
}, l.cfg)
err := CreateDockerTarContext(ctx, buildCtxWriter,
NewBuildConfig(workspace, artifact, a.DockerfilePath, buildArgs), l.cfg)
if err != nil {
buildCtxWriter.CloseWithError(fmt.Errorf("creating docker context: %w", err))
return
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/docker/image_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func TestBuild(t *testing.T) {

localDocker := NewLocalDaemon(test.api, nil, false, nil)
opts := BuildOptions{Tag: "finalimage", Mode: test.mode}
_, err := localDocker.Build(context.Background(), ioutil.Discard, test.workspace, test.artifact, opts)
_, err := localDocker.Build(context.Background(), ioutil.Discard, test.workspace, "final-image", test.artifact, opts)

if test.shouldErr {
t.CheckErrorContains(test.expectedError, err)
Expand Down
14 changes: 7 additions & 7 deletions pkg/skaffold/docker/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,12 @@ func readCopyCmdsFromDockerfile(onlyLastImage bool, absDockerfilePath, workspace

res, err := parser.Parse(f)
if err != nil {
return nil, fmt.Errorf("parsing dockerfile %q: %w", absDockerfilePath, err)
return nil, fmt.Errorf("parsing dockerfilePath %q: %w", absDockerfilePath, err)
}

// instructions.Parse will check for malformed Dockerfile
if _, _, err := instructions.Parse(res.AST); err != nil {
return nil, fmt.Errorf("parsing dockerfile %q: %w", absDockerfilePath, err)
return nil, fmt.Errorf("parsing dockerfilePath %q: %w", absDockerfilePath, err)
}

dockerfileLines := res.AST.Children
Expand All @@ -101,11 +101,11 @@ func readCopyCmdsFromDockerfile(onlyLastImage bool, absDockerfilePath, workspace
return expandSrcGlobPatterns(workspace, cpCmds)
}

// filterUnusedBuildArgs removes entries from the build arguments map that are not found in the dockerfile
// filterUnusedBuildArgs removes entries from the build arguments map that are not found in the dockerfilePath
func filterUnusedBuildArgs(dockerFile io.Reader, buildArgs map[string]*string) (map[string]*string, error) {
res, err := parser.Parse(dockerFile)
if err != nil {
return nil, fmt.Errorf("parsing dockerfile: %w", err)
return nil, fmt.Errorf("parsing dockerfilePath: %w", err)
}
m := make(map[string]*string)
for _, n := range res.AST.Children {
Expand Down Expand Up @@ -195,7 +195,7 @@ func expandSrcGlobPatterns(workspace string, cpCmds []*copyCommand) ([]fromTo, e
}
}

logrus.Debugf("Found dependencies for dockerfile: %v", fts)
logrus.Debugf("Found dependencies for dockerfilePath: %v", fts)
return fts, nil
}

Expand Down Expand Up @@ -266,7 +266,7 @@ func extractCopyCommands(nodes []*parser.Node, onlyLastImage bool, cfg Config) (
}

func readCopyCommand(value *parser.Node, envs []string, workdir string) (*copyCommand, error) {
// If the --from flag is provided, we are dealing with a multi-stage dockerfile
// If the --from flag is provided, we are dealing with a multi-stage dockerfilePath
// Adding a dependency from a different stage does not imply a source dependency
if hasMultiStageFlag(value.Flags) {
return nil, nil
Expand All @@ -283,7 +283,7 @@ func readCopyCommand(value *parser.Node, envs []string, workdir string) (*copyCo
paths = append(paths, path)
}
if len(paths) == 0 {
return nil, fmt.Errorf("invalid dockerfile instruction: %q", value.Original)
return nil, fmt.Errorf("invalid dockerfilePath instruction: %q", value.Original)
}

// All paths are sources except the last one
Expand Down
Loading

0 comments on commit 62b2d0b

Please sign in to comment.