From f96742fd08728e468f17bb913d73caeecf50f03a Mon Sep 17 00:00:00 2001 From: tejal29 Date: Mon, 7 Dec 2020 16:07:48 -0800 Subject: [PATCH] Move key from dockerfilePath to artifact name --- pkg/skaffold/build/cache/retrieve_test.go | 2 +- pkg/skaffold/build/cluster/kaniko.go | 6 +-- pkg/skaffold/build/custom/dependencies.go | 10 +++-- .../build/custom/dependencies_test.go | 6 +-- pkg/skaffold/build/dependencies.go | 6 +-- pkg/skaffold/build/docker/docker.go | 2 +- pkg/skaffold/diagnose/diagnose.go | 3 +- pkg/skaffold/docker/context.go | 9 ++--- pkg/skaffold/docker/context_test.go | 2 +- pkg/skaffold/docker/dependencies.go | 40 ++++++++++++++----- pkg/skaffold/docker/dependencies_test.go | 15 ++++--- pkg/skaffold/docker/image.go | 10 ++--- pkg/skaffold/docker/image_test.go | 2 +- pkg/skaffold/util/store.go | 2 +- 14 files changed, 66 insertions(+), 49 deletions(-) diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index f7a1ed4f583..08d3563a6d4 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -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 } diff --git a/pkg/skaffold/build/cluster/kaniko.go b/pkg/skaffold/build/cluster/kaniko.go index bd5644ee924..43aeb46dfc1 100644 --- a/pkg/skaffold/build/cluster/kaniko.go +++ b/pkg/skaffold/build/cluster/kaniko.go @@ -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 diff --git a/pkg/skaffold/build/custom/dependencies.go b/pkg/skaffold/build/custom/dependencies.go index 521f32eb3a8..6320d7017e5 100644 --- a/pkg/skaffold/build/custom/dependencies.go +++ b/pkg/skaffold/build/custom/dependencies.go @@ -30,11 +30,10 @@ 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, getDockerBuildConfig(workspace, artifactName, a), cfg) case a.Dependencies.Command != "": split := strings.Split(a.Dependencies.Command, " ") @@ -53,3 +52,8 @@ func GetDependencies(ctx context.Context, workspace string, a *latest.CustomArti return list.Files(workspace, a.Dependencies.Paths, a.Dependencies.Ignore) } } + +func getDockerBuildConfig(ws string, artifact string, a *latest.CustomArtifact) docker.BuildConfig { + dockerfile := a.Dependencies.Dockerfile + return docker.NewBuildConfig(ws, artifact, dockerfile.Path, dockerfile.BuildArgs) +} diff --git a/pkg/skaffold/build/custom/dependencies_test.go b/pkg/skaffold/build/custom/dependencies_test.go index c7a88ab789b..f86368694ff 100644 --- a/pkg/skaffold/build/custom/dependencies_test.go +++ b/pkg/skaffold/build/custom/dependencies_test.go @@ -50,7 +50,7 @@ func TestGetDependenciesDockerfile(t *testing.T) { } expected := []string{"Dockerfile", filepath.FromSlash("baz/file"), "foo"} - deps, err := GetDependencies(context.Background(), tmpDir.Root(), customArtifact, nil) + deps, err := GetDependencies(context.Background(), tmpDir.Root(), "test", customArtifact, nil) testutil.CheckErrorAndDeepEqual(t, false, err, expected, deps) } @@ -69,7 +69,7 @@ func TestGetDependenciesCommand(t *testing.T) { } expected := []string{"file1", "file2", "file3"} - deps, err := GetDependencies(context.Background(), "", customArtifact, nil) + deps, err := GetDependencies(context.Background(), "", "test", customArtifact, nil) t.CheckNoError(err) t.CheckDeepEqual(expected, deps) @@ -119,7 +119,7 @@ func TestGetDependenciesPaths(t *testing.T) { tmpDir := t.NewTempDir(). Touch("foo", "bar", "baz/file") - deps, err := GetDependencies(context.Background(), tmpDir.Root(), &latest.CustomArtifact{ + deps, err := GetDependencies(context.Background(), tmpDir.Root(), "test", &latest.CustomArtifact{ Dependencies: &latest.CustomDependencies{ Paths: test.paths, Ignore: test.ignore, diff --git a/pkg/skaffold/build/dependencies.go b/pkg/skaffold/build/dependencies.go index da60f6e5fd6..4a073bcefed 100644 --- a/pkg/skaffold/build/dependencies.go +++ b/pkg/skaffold/build/dependencies.go @@ -48,7 +48,7 @@ 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) @@ -56,7 +56,7 @@ 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.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) @@ -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) diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 53658431206..a5fd90aaec2 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -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 { diff --git a/pkg/skaffold/diagnose/diagnose.go b/pkg/skaffold/diagnose/diagnose.go index 9a909ebe255..01a758692a3 100644 --- a/pkg/skaffold/diagnose/diagnose.go +++ b/pkg/skaffold/diagnose/diagnose.go @@ -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 diff --git a/pkg/skaffold/docker/context.go b/pkg/skaffold/docker/context.go index 244c6ee2917..23187cb7910 100644 --- a/pkg/skaffold/docker/context.go +++ b/pkg/skaffold/docker/context.go @@ -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) } diff --git a/pkg/skaffold/docker/context_test.go b/pkg/skaffold/docker/context_test.go index 6c30c84c212..bfc59edca2e 100644 --- a/pkg/skaffold/docker/context_test.go +++ b/pkg/skaffold/docker/context_test.go @@ -46,7 +46,7 @@ func TestDockerContext(t *testing.T) { reader, writer := io.Pipe() go func() { - err := CreateDockerTarContext(context.Background(), writer, dir, artifact, nil) + err := CreateDockerTarContext(context.Background(), writer, NewBuildConfig(dir, "test", artifact.DockerfilePath, artifact.BuildArgs), nil) if err != nil { writer.CloseWithError(err) } else { diff --git a/pkg/skaffold/docker/dependencies.go b/pkg/skaffold/docker/dependencies.go index a5ddce02d0c..be2ad3e113d 100644 --- a/pkg/skaffold/docker/dependencies.go +++ b/pkg/skaffold/docker/dependencies.go @@ -33,7 +33,25 @@ 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. @@ -50,26 +68,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) } diff --git a/pkg/skaffold/docker/dependencies_test.go b/pkg/skaffold/docker/dependencies_test.go index 4f9c8fc5a8a..cff2856f37d 100644 --- a/pkg/skaffold/docker/dependencies_test.go +++ b/pkg/skaffold/docker/dependencies_test.go @@ -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) @@ -674,7 +674,7 @@ func TestGetDependenciesCached(t *testing.T) { retrieveImgMock: func(_ string, _ Config) (*v1.ConfigFile, error) { return nil, fmt.Errorf("unexpected call") }, - dependencyCache: map[string]interface{}{"Dockerfile": []string{"random.go"}}, + dependencyCache: map[string]interface{}{"dummy": []string{"random.go"}}, expected: []string{"random.go"}, }, { @@ -682,15 +682,14 @@ func TestGetDependenciesCached(t *testing.T) { retrieveImgMock: func(_ string, _ Config) (*v1.ConfigFile, error) { return &v1.ConfigFile{}, nil }, - dependencyCache: map[string]interface{}{"Dockerfile": fmt.Errorf("remote manifest fetch")}, + dependencyCache: map[string]interface{}{"dummy": fmt.Errorf("remote manifest fetch")}, shouldErr: true, }, { description: "with cached results for dockerfile in another app", retrieveImgMock: imageFetcher.fetch, - dependencyCache: map[string]interface{}{ - filepath.Join("app", "Dockerfile"): []string{"random.go"}}, - expected: []string{"Dockerfile", "server.go"}, + dependencyCache: map[string]interface{}{"another": []string{"random.go"}}, + expected: []string{"Dockerfile", "server.go"}, }, } @@ -704,11 +703,11 @@ func TestGetDependenciesCached(t *testing.T) { tmpDir.Write("Dockerfile", copyServerGo) for k, v := range test.dependencyCache { - dependencyCache.Exec(tmpDir.Path(k), func() interface{} { + dependencyCache.Exec(k, func() interface{} { return v }) } - deps, err := GetDependenciesCached(context.Background(), tmpDir.Root(), "Dockerfile", map[string]*string{}, nil) + deps, err := GetDependenciesCached(context.Background(), NewBuildConfig(tmpDir.Root(), "dummy", "Dockerfile", map[string]*string{}), nil) t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, deps) }) } diff --git a/pkg/skaffold/docker/image.go b/pkg/skaffold/docker/image.go index c4d6babdf46..2e0bd356239 100644 --- a/pkg/skaffold/docker/image.go +++ b/pkg/skaffold/docker/image.go @@ -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) @@ -174,7 +174,7 @@ 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) { +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, dockerfile: %s", workspace, a.DockerfilePath) if err := l.CheckCompatible(a); err != nil { @@ -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 diff --git a/pkg/skaffold/docker/image_test.go b/pkg/skaffold/docker/image_test.go index 83356786afe..4c0b279f2cc 100644 --- a/pkg/skaffold/docker/image_test.go +++ b/pkg/skaffold/docker/image_test.go @@ -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) diff --git a/pkg/skaffold/util/store.go b/pkg/skaffold/util/store.go index 0be39689054..36a8b963608 100644 --- a/pkg/skaffold/util/store.go +++ b/pkg/skaffold/util/store.go @@ -56,7 +56,7 @@ func (o *SyncStore) Exec(key string, f func() interface{}) interface{} { // Store will store the results for a key in a cache // This function is not safe to use if multiple subroutines store the -// result for the same artifact. +// result for the same key. func (o *SyncStore) Store(key string, r interface{}) { o.results.Store(key, r) }