From 1657f18312b569ceb9b1712f79b3740dfaf1343d Mon Sep 17 00:00:00 2001 From: Abe Winter Date: Wed, 19 Jun 2024 14:05:48 -0400 Subject: [PATCH 01/44] provide a docker.ImageReference in dependency resolution step --- pkg/skaffold/graph/dependencies.go | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/pkg/skaffold/graph/dependencies.go b/pkg/skaffold/graph/dependencies.go index d3a2a9adcd9..b4cc68d5c2b 100644 --- a/pkg/skaffold/graph/dependencies.go +++ b/pkg/skaffold/graph/dependencies.go @@ -43,7 +43,7 @@ type SourceDependenciesCache interface { TransitiveArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) // SingleArtifactDependencies returns the source dependencies for only the target artifact. // The result (even if an error) is cached so that the function is evaluated only once for every artifact. The cache is reset before the start of the next devloop. - SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) + SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tags map[string]string) ([]string, error) // Reset removes the cached source dependencies for all artifacts Reset() } @@ -65,7 +65,7 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont }) defer endTrace() - deps, err := r.SingleArtifactDependencies(ctx, a) + deps, err := r.SingleArtifactDependencies(ctx, a, nil) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err @@ -81,14 +81,14 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return deps, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tags map[string]string) ([]string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "SingleArtifactDependencies", map[string]string{ "ArtifactName": instrumentation.PII(a.ImageName), }) defer endTrace() res, err := r.cache.Exec(a.ImageName, func() ([]string, error) { - return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver) + return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver, tags) }) if err != nil { endTrace(instrumentation.TraceEndError(err)) @@ -104,8 +104,20 @@ func (r *dependencyResolverImpl) Reset() { r.cache = util.NewSyncStore[[]string]() } +func quickMakeEnvTags(tag string) (map[string]string, error) { + imgRef, err := docker.ParseReference(tag) + if err != nil { + return nil, fmt.Errorf("couldn't parse image tag %s %w", tag, err) + } + return map[string]string{ + "IMAGE_REPO": imgRef.Repo, + "IMAGE_NAME": imgRef.Name, + "IMAGE_TAG": imgRef.Tag, + }, nil +} + // sourceDependenciesForArtifact returns the build dependencies for the current artifact. -func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver) ([]string, error) { +func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver, tags map[string]string) ([]string, error) { var ( paths []string err error @@ -118,7 +130,12 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg // For single build scenarios like `build` and `run`, it is called for the cache hash calculations which are already handled in `artifactHasher`. // For `dev` it will succeed on the first dev loop and list any additional dependencies found from the base artifact's ONBUILD instructions as a file added instead of modified (see `filemon.Events`) deps := docker.ResolveDependencyImages(a.Dependencies, r, false) - args, evalErr := docker.EvalBuildArgs(cfg.Mode(), a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, deps) + var envTags map[string]string + envTags, err = quickMakeEnvTags(tags[a.ImageName]) + if err != nil { + return nil, err + } + args, evalErr := docker.EvalBuildArgsWithEnv(cfg.Mode(), a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, deps, envTags) if evalErr != nil { return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr) } From 147d21b369c9d9cde3f01f429909c4771a9c0ff0 Mon Sep 17 00:00:00 2001 From: Abe Winter Date: Wed, 19 Jun 2024 14:08:58 -0400 Subject: [PATCH 02/44] pass nils --- pkg/skaffold/build/gcb/cloud_build.go | 2 +- pkg/skaffold/diagnose/diagnose.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 920ba600dec..266edbd0748 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -127,7 +127,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer }) } - dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact) + dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact, nil) if err != nil { return "", sErrors.NewErrorWithStatusCode(&proto.ActionableErr{ ErrCode: proto.StatusCode_BUILD_GCB_GET_DEPENDENCY_ERR, diff --git a/pkg/skaffold/diagnose/diagnose.go b/pkg/skaffold/diagnose/diagnose.go index 5db4f2df9fe..f8b1d58f5fa 100644 --- a/pkg/skaffold/diagnose/diagnose.go +++ b/pkg/skaffold/diagnose/diagnose.go @@ -123,7 +123,7 @@ func timeToListDependencies(ctx context.Context, a *latest.Artifact, cfg Config) start := time.Now() g := graph.ToArtifactGraph(cfg.Artifacts()) sourceDependencies := graph.NewSourceDependenciesCache(cfg, nil, g) - paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a) + paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a, nil) return timeutil.Humanize(time.Since(start)), paths, err } From 35a37c16e4d1e89d42243e7376a20f9590ebfd31 Mon Sep 17 00:00:00 2001 From: Abe Winter Date: Wed, 19 Jun 2024 14:10:53 -0400 Subject: [PATCH 03/44] other plumbing --- pkg/skaffold/build/cache/cache.go | 2 +- pkg/skaffold/build/cache/hash.go | 16 ++++++++-------- pkg/skaffold/build/cache/lookup.go | 7 ++++--- pkg/skaffold/runner/new.go | 4 ++-- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 371ffb104f2..d721d1f93bb 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -61,7 +61,7 @@ type cache struct { } // DependencyLister fetches a list of dependencies for an artifact -type DependencyLister func(ctx context.Context, artifact *latest.Artifact) ([]string, error) +type DependencyLister func(ctx context.Context, artifact *latest.Artifact, tags map[string]string) ([]string, error) type Config interface { docker.Config diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index 02d80202ab9..0d05bb82c31 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -47,7 +47,7 @@ var ( ) type artifactHasher interface { - hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver) (string, error) + hash(context.Context, *latest.Artifact, platform.Resolver, map[string]string) (string, error) } type artifactHasherImpl struct { @@ -67,20 +67,20 @@ func newArtifactHasher(artifacts graph.ArtifactGraph, lister DependencyLister, m } } -func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Resolver) (string, error) { +func (h *artifactHasherImpl) hash(ctx context.Context, a *latest.Artifact, platforms platform.Resolver, tags map[string]string) (string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "hash_GenerateHashOneArtifact", map[string]string{ "ImageName": instrumentation.PII(a.ImageName), }) defer endTrace() - hash, err := h.safeHash(ctx, out, a, platforms.GetPlatforms(a.ImageName)) + hash, err := h.safeHash(ctx, a, platforms.GetPlatforms(a.ImageName), tags) if err != nil { endTrace(instrumentation.TraceEndError(err)) return "", err } hashes := []string{hash} for _, dep := range sortedDependencies(a, h.artifacts) { - depHash, err := h.hash(ctx, out, dep, platforms) + depHash, err := h.hash(ctx, dep, platforms, tags) if err != nil { endTrace(instrumentation.TraceEndError(err)) return "", err @@ -94,15 +94,15 @@ func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest. return encode(hashes) } -func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher) (string, error) { +func (h *artifactHasherImpl) safeHash(ctx context.Context, a *latest.Artifact, platforms platform.Matcher, tags map[string]string) (string, error) { return h.syncStore.Exec(a.ImageName, func() (string, error) { - return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms) + return singleArtifactHash(ctx, h.lister, a, h.mode, platforms, tags) }) } // singleArtifactHash calculates the hash for a single artifact, and ignores its required artifacts. -func singleArtifactHash(ctx context.Context, out io.Writer, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher) (string, error) { +func singleArtifactHash(ctx context.Context, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher, tags map[string]string) (string, error) { var inputs []string // Append the artifact's configuration @@ -113,7 +113,7 @@ func singleArtifactHash(ctx context.Context, out io.Writer, depLister Dependency inputs = append(inputs, config) // Append the digest of each input file - deps, err := depLister(ctx, a) + deps, err := depLister(ctx, a, tags) if err != nil { return "", fmt.Errorf("getting dependencies for %q: %w", a.ImageName, err) } diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index f9634dcfda7..3e4581378d5 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -48,7 +48,7 @@ func (c *cache) lookupArtifacts(ctx context.Context, out io.Writer, tags tag.Ima i := i go func() { - details[i] = c.lookup(ctx, out, artifacts[i], tags[artifacts[i].ImageName], platforms, h) + details[i] = c.lookup(ctx, artifacts[i], tags, platforms, h) wg.Done() }() } @@ -57,13 +57,14 @@ func (c *cache) lookupArtifacts(ctx context.Context, out io.Writer, tags tag.Ima return details } -func (c *cache) lookup(ctx context.Context, out io.Writer, a *latest.Artifact, tag string, platforms platform.Resolver, h artifactHasher) cacheDetails { +func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tags map[string]string, platforms platform.Resolver, h artifactHasher) cacheDetails { + tag := tags[a.ImageName] ctx, endTrace := instrumentation.StartTrace(ctx, "lookup_CacheLookupOneArtifact", map[string]string{ "ImageName": instrumentation.PII(a.ImageName), }) defer endTrace() - hash, err := h.hash(ctx, out, a, platforms) + hash, err := h.hash(ctx, a, platforms, tags) if err != nil { return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)} } diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index 4adc8518f97..e2a8081b16d 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -117,11 +117,11 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold return nil, fmt.Errorf("creating actiosn runner: %w", err) } - depLister := func(ctx context.Context, artifact *latest.Artifact) ([]string, error) { + depLister := func(ctx context.Context, artifact *latest.Artifact, tags map[string]string) ([]string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "NewForConfig_depLister") defer endTrace() - buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact) + buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, tags) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err From f6411bfb9bca51f5d50b2655117cf64a62a64943 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Tue, 14 Jan 2025 22:12:42 +0000 Subject: [PATCH 04/44] fix: add back out io.Writer field --- pkg/skaffold/build/cache/hash.go | 10 +++++----- pkg/skaffold/build/cache/lookup.go | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index 0d05bb82c31..97267dd30fb 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -47,7 +47,7 @@ var ( ) type artifactHasher interface { - hash(context.Context, *latest.Artifact, platform.Resolver, map[string]string) (string, error) + hash(context.Context, out io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) } type artifactHasherImpl struct { @@ -67,7 +67,7 @@ func newArtifactHasher(artifacts graph.ArtifactGraph, lister DependencyLister, m } } -func (h *artifactHasherImpl) hash(ctx context.Context, a *latest.Artifact, platforms platform.Resolver, tags map[string]string) (string, error) { +func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Resolver, tags map[string]string) (string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "hash_GenerateHashOneArtifact", map[string]string{ "ImageName": instrumentation.PII(a.ImageName), }) @@ -94,15 +94,15 @@ func (h *artifactHasherImpl) hash(ctx context.Context, a *latest.Artifact, platf return encode(hashes) } -func (h *artifactHasherImpl) safeHash(ctx context.Context, a *latest.Artifact, platforms platform.Matcher, tags map[string]string) (string, error) { +func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher, tags map[string]string) (string, error) { return h.syncStore.Exec(a.ImageName, func() (string, error) { - return singleArtifactHash(ctx, h.lister, a, h.mode, platforms, tags) + return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms, tags) }) } // singleArtifactHash calculates the hash for a single artifact, and ignores its required artifacts. -func singleArtifactHash(ctx context.Context, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher, tags map[string]string) (string, error) { +func singleArtifactHash(ctx context.Context, out io.Writer, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher, tags map[string]string) (string, error) { var inputs []string // Append the artifact's configuration diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index 3e4581378d5..f86a2d92d45 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -48,7 +48,7 @@ func (c *cache) lookupArtifacts(ctx context.Context, out io.Writer, tags tag.Ima i := i go func() { - details[i] = c.lookup(ctx, artifacts[i], tags, platforms, h) + details[i] = c.lookup(ctx, out, artifacts[i], tags, platforms, h) wg.Done() }() } @@ -57,14 +57,14 @@ func (c *cache) lookupArtifacts(ctx context.Context, out io.Writer, tags tag.Ima return details } -func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tags map[string]string, platforms platform.Resolver, h artifactHasher) cacheDetails { +func (c *cache) lookup(ctx context.Context, out io.Writer, a *latest.Artifact, tags map[string]string, platforms platform.Resolver, h artifactHasher) cacheDetails { tag := tags[a.ImageName] ctx, endTrace := instrumentation.StartTrace(ctx, "lookup_CacheLookupOneArtifact", map[string]string{ "ImageName": instrumentation.PII(a.ImageName), }) defer endTrace() - hash, err := h.hash(ctx, a, platforms, tags) + hash, err := h.hash(ctx, out, a, platforms, tags) if err != nil { return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)} } From a61996cdf1f705e7fcfce143fca9cccbb795eca4 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Tue, 14 Jan 2025 22:36:27 +0000 Subject: [PATCH 05/44] fix bugs --- pkg/skaffold/build/cache/hash.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index 97267dd30fb..f4eb0020099 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -47,7 +47,7 @@ var ( ) type artifactHasher interface { - hash(context.Context, out io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) + hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) } type artifactHasherImpl struct { @@ -73,14 +73,14 @@ func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest. }) defer endTrace() - hash, err := h.safeHash(ctx, a, platforms.GetPlatforms(a.ImageName), tags) + hash, err := h.safeHash(ctx, out, a, platforms.GetPlatforms(a.ImageName), tags) if err != nil { endTrace(instrumentation.TraceEndError(err)) return "", err } hashes := []string{hash} for _, dep := range sortedDependencies(a, h.artifacts) { - depHash, err := h.hash(ctx, dep, platforms, tags) + depHash, err := h.hash(ctx, out, dep, platforms, tags) if err != nil { endTrace(instrumentation.TraceEndError(err)) return "", err From d506f62449e2e33ce669f532c4995c46da6c6437 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 15 Jan 2025 19:16:22 +0000 Subject: [PATCH 06/44] fix lookup and hash tests --- pkg/skaffold/build/cache/hash_test.go | 22 +++++++++++----------- pkg/skaffold/build/cache/lookup_test.go | 4 ++-- pkg/skaffold/build/cache/retrieve_test.go | 2 +- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index f4c7099f522..d5cc2d220bb 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -31,7 +31,7 @@ import ( ) func stubDependencyLister(dependencies []string) DependencyLister { - return func(context.Context, *latest.Artifact) ([]string, error) { + return func(context.Context, *latest.Artifact, map[string]string) ([]string, error) { return dependencies, nil } } @@ -162,7 +162,7 @@ func TestGetHashForArtifact(t *testing.T) { } depLister := stubDependencyLister(test.dependencies) - actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms) + actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms, nil) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -242,11 +242,11 @@ func TestGetHashForArtifactWithDependencies(t *testing.T) { } } - depLister := func(_ context.Context, a *latest.Artifact) ([]string, error) { + depLister := func(_ context.Context, a *latest.Artifact, _ map[string]string) ([]string, error) { return test.fileDeps[a.ImageName], nil } - actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}) + actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}, nil) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -309,21 +309,21 @@ func TestBuildArgs(t *testing.T) { } t.Override(&fileHasherFunc, mockCacheHasher) t.Override(&artifactConfigFunc, fakeArtifactConfig) - actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}) + actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change order of buildargs artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"two": util.Ptr("2"), "one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}) + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change build args, get different hash artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}) + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) t.CheckNoError(err) if actual == test.expected { @@ -356,7 +356,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { t.Override(&artifactConfigFunc, fakeArtifactConfig) depLister := stubDependencyLister([]string{"graph"}) - hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}) + hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) t.CheckNoError(err) @@ -366,7 +366,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { return []string{"FOO=baz"} } - hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}) + hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) t.CheckNoError(err) if hash1 == hash2 { @@ -423,7 +423,7 @@ func TestCacheHasher(t *testing.T) { path := originalFile depLister := stubDependencyLister([]string{tmpDir.Path(originalFile)}) - oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}) + oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, nil) t.CheckNoError(err) test.update(originalFile, tmpDir) @@ -432,7 +432,7 @@ func TestCacheHasher(t *testing.T) { } depLister = stubDependencyLister([]string{tmpDir.Path(path)}) - newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}) + newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, nil) t.CheckNoError(err) t.CheckFalse(test.differentHash && oldHash == newHash) diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go index 455f08bb218..2c6b33cdb89 100644 --- a/pkg/skaffold/build/cache/lookup_test.go +++ b/pkg/skaffold/build/cache/lookup_test.go @@ -233,7 +233,7 @@ type mockHasher struct { val string } -func (m mockHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver) (string, error) { +func (m mockHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) { return m.val, nil } @@ -241,7 +241,7 @@ type failingHasher struct { err error } -func (f failingHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver) (string, error) { +func (f failingHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) { return "", f.err } diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index 9a21409ba5a..13e1f1d074d 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -37,7 +37,7 @@ import ( ) func depLister(files map[string][]string) DependencyLister { - return func(_ context.Context, artifact *latest.Artifact) ([]string, error) { + return func(_ context.Context, artifact *latest.Artifact, _ map[string]string) ([]string, error) { list, found := files[artifact.ImageName] if !found { return nil, errors.New("unknown artifact") From c3bcf4dd915974420f28bfbaded5a80c34bcfdca Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 15 Jan 2025 19:34:04 +0000 Subject: [PATCH 07/44] fix tag test --- pkg/skaffold/tag/custom_template_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/skaffold/tag/custom_template_test.go b/pkg/skaffold/tag/custom_template_test.go index 7530809d9c5..46528d48acc 100644 --- a/pkg/skaffold/tag/custom_template_test.go +++ b/pkg/skaffold/tag/custom_template_test.go @@ -35,7 +35,7 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return []string{}, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, _ map[string]string) ([]string, error) { return []string{}, nil } From 67014b3afbbf5ef439cdda9f760340d202849224 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 15 Jan 2025 19:36:28 +0000 Subject: [PATCH 08/44] fix skaffold/runner --- pkg/skaffold/runner/listen_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/skaffold/runner/listen_test.go b/pkg/skaffold/runner/listen_test.go index 59dcf853c88..f8d6af8a302 100644 --- a/pkg/skaffold/runner/listen_test.go +++ b/pkg/skaffold/runner/listen_test.go @@ -59,7 +59,7 @@ func (f *fakeDepsResolver) TransitiveArtifactDependencies(context.Context, *late return nil, nil } -func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact) ([]string, error) { +func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact, map[string]string) ([]string, error) { return nil, nil } From 5076965329f9b775f7e06b4bad9ccb33b3c49ab4 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 15 Jan 2025 20:00:27 +0000 Subject: [PATCH 09/44] fix skaffold/graph --- pkg/skaffold/graph/dependencies_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 37993f3f7aa..94955b4e8ff 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -43,7 +43,7 @@ func TestSourceDependenciesCache(t *testing.T) { "img4": {"file41", "file42"}, } counts := map[string]int{"img1": 0, "img2": 0, "img3": 0, "img4": 0} - t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver) ([]string, error) { + t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver, _ map[string]string) ([]string, error) { counts[a.ImageName]++ return deps[a.ImageName], nil }) @@ -89,7 +89,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, test.dockerArtifactResolver) + paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, test.dockerArtifactResolver, map[string]string{}) t.CheckNoError(err) t.CheckDeepEqual(test.expectedPaths, paths, cmpopts.SortSlices(func(x, y string) bool { return x < y })) From 5324000387f60d70731afaa7190218020a04f680 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 15 Jan 2025 22:36:33 +0000 Subject: [PATCH 10/44] remove the tags field where its not needed --- pkg/skaffold/build/cache/cache.go | 2 +- pkg/skaffold/build/cache/hash.go | 10 +++++----- pkg/skaffold/build/cache/hash_test.go | 4 ++-- pkg/skaffold/build/cache/retrieve_test.go | 2 +- pkg/skaffold/build/gcb/cloud_build.go | 2 +- pkg/skaffold/diagnose/diagnose.go | 3 ++- pkg/skaffold/diagnose/diagnose_test.go | 3 +++ pkg/skaffold/graph/dependencies.go | 16 +++++++++------- pkg/skaffold/graph/dependencies_test.go | 4 ++-- pkg/skaffold/runner/listen_test.go | 2 +- pkg/skaffold/runner/new.go | 4 ++-- pkg/skaffold/tag/custom_template_test.go | 2 +- 12 files changed, 30 insertions(+), 24 deletions(-) diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index d721d1f93bb..371ffb104f2 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -61,7 +61,7 @@ type cache struct { } // DependencyLister fetches a list of dependencies for an artifact -type DependencyLister func(ctx context.Context, artifact *latest.Artifact, tags map[string]string) ([]string, error) +type DependencyLister func(ctx context.Context, artifact *latest.Artifact) ([]string, error) type Config interface { docker.Config diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index f4eb0020099..7f758c4d668 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -73,7 +73,7 @@ func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest. }) defer endTrace() - hash, err := h.safeHash(ctx, out, a, platforms.GetPlatforms(a.ImageName), tags) + hash, err := h.safeHash(ctx, out, a, platforms.GetPlatforms(a.ImageName)) if err != nil { endTrace(instrumentation.TraceEndError(err)) return "", err @@ -94,15 +94,15 @@ func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest. return encode(hashes) } -func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher, tags map[string]string) (string, error) { +func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher) (string, error) { return h.syncStore.Exec(a.ImageName, func() (string, error) { - return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms, tags) + return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms) }) } // singleArtifactHash calculates the hash for a single artifact, and ignores its required artifacts. -func singleArtifactHash(ctx context.Context, out io.Writer, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher, tags map[string]string) (string, error) { +func singleArtifactHash(ctx context.Context, out io.Writer, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher) (string, error) { var inputs []string // Append the artifact's configuration @@ -113,7 +113,7 @@ func singleArtifactHash(ctx context.Context, out io.Writer, depLister Dependency inputs = append(inputs, config) // Append the digest of each input file - deps, err := depLister(ctx, a, tags) + deps, err := depLister(ctx, a) if err != nil { return "", fmt.Errorf("getting dependencies for %q: %w", a.ImageName, err) } diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index d5cc2d220bb..7bcdcc1f8df 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -31,7 +31,7 @@ import ( ) func stubDependencyLister(dependencies []string) DependencyLister { - return func(context.Context, *latest.Artifact, map[string]string) ([]string, error) { + return func(context.Context, *latest.Artifact) ([]string, error) { return dependencies, nil } } @@ -242,7 +242,7 @@ func TestGetHashForArtifactWithDependencies(t *testing.T) { } } - depLister := func(_ context.Context, a *latest.Artifact, _ map[string]string) ([]string, error) { + depLister := func(_ context.Context, a *latest.Artifact) ([]string, error) { return test.fileDeps[a.ImageName], nil } diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index 13e1f1d074d..9a21409ba5a 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -37,7 +37,7 @@ import ( ) func depLister(files map[string][]string) DependencyLister { - return func(_ context.Context, artifact *latest.Artifact, _ map[string]string) ([]string, error) { + return func(_ context.Context, artifact *latest.Artifact) ([]string, error) { list, found := files[artifact.ImageName] if !found { return nil, errors.New("unknown artifact") diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 266edbd0748..920ba600dec 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -127,7 +127,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer }) } - dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact, nil) + dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact) if err != nil { return "", sErrors.NewErrorWithStatusCode(&proto.ActionableErr{ ErrCode: proto.StatusCode_BUILD_GCB_GET_DEPENDENCY_ERR, diff --git a/pkg/skaffold/diagnose/diagnose.go b/pkg/skaffold/diagnose/diagnose.go index f8b1d58f5fa..3075a5314d4 100644 --- a/pkg/skaffold/diagnose/diagnose.go +++ b/pkg/skaffold/diagnose/diagnose.go @@ -123,7 +123,8 @@ func timeToListDependencies(ctx context.Context, a *latest.Artifact, cfg Config) start := time.Now() g := graph.ToArtifactGraph(cfg.Artifacts()) sourceDependencies := graph.NewSourceDependenciesCache(cfg, nil, g) - paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a, nil) + + paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a) return timeutil.Humanize(time.Since(start)), paths, err } diff --git a/pkg/skaffold/diagnose/diagnose_test.go b/pkg/skaffold/diagnose/diagnose_test.go index e79b0f2e350..260a48f500b 100644 --- a/pkg/skaffold/diagnose/diagnose_test.go +++ b/pkg/skaffold/diagnose/diagnose_test.go @@ -81,8 +81,11 @@ func TestCheckArtifacts(t *testing.T) { testutil.Run(t, "", func(t *testutil.T) { tmpDir := t.NewTempDir().Write("Dockerfile", "FROM busybox") + // imgTag := "{{.IMAGE_TAG}}" + err := CheckArtifacts(context.Background(), &mockConfig{ artifacts: []*latest.Artifact{{ + ImageName: "base", Workspace: tmpDir.Root(), ArtifactType: latest.ArtifactType{ DockerArtifact: &latest.DockerArtifact{ diff --git a/pkg/skaffold/graph/dependencies.go b/pkg/skaffold/graph/dependencies.go index b4cc68d5c2b..19ce8a176ff 100644 --- a/pkg/skaffold/graph/dependencies.go +++ b/pkg/skaffold/graph/dependencies.go @@ -43,7 +43,7 @@ type SourceDependenciesCache interface { TransitiveArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) // SingleArtifactDependencies returns the source dependencies for only the target artifact. // The result (even if an error) is cached so that the function is evaluated only once for every artifact. The cache is reset before the start of the next devloop. - SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tags map[string]string) ([]string, error) + SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) // Reset removes the cached source dependencies for all artifacts Reset() } @@ -65,7 +65,7 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont }) defer endTrace() - deps, err := r.SingleArtifactDependencies(ctx, a, nil) + deps, err := r.SingleArtifactDependencies(ctx, a) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err @@ -81,14 +81,14 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return deps, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tags map[string]string) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "SingleArtifactDependencies", map[string]string{ "ArtifactName": instrumentation.PII(a.ImageName), }) defer endTrace() res, err := r.cache.Exec(a.ImageName, func() ([]string, error) { - return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver, tags) + return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver) }) if err != nil { endTrace(instrumentation.TraceEndError(err)) @@ -104,11 +104,13 @@ func (r *dependencyResolverImpl) Reset() { r.cache = util.NewSyncStore[[]string]() } -func quickMakeEnvTags(tag string) (map[string]string, error) { +// QuickMakeEnvTags generate a set of build tags from the docker image name. +func QuickMakeEnvTags(tag string) (map[string]string, error) { imgRef, err := docker.ParseReference(tag) if err != nil { return nil, fmt.Errorf("couldn't parse image tag %s %w", tag, err) } + return map[string]string{ "IMAGE_REPO": imgRef.Repo, "IMAGE_NAME": imgRef.Name, @@ -117,7 +119,7 @@ func quickMakeEnvTags(tag string) (map[string]string, error) { } // sourceDependenciesForArtifact returns the build dependencies for the current artifact. -func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver, tags map[string]string) ([]string, error) { +func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver) ([]string, error) { var ( paths []string err error @@ -131,7 +133,7 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg // For `dev` it will succeed on the first dev loop and list any additional dependencies found from the base artifact's ONBUILD instructions as a file added instead of modified (see `filemon.Events`) deps := docker.ResolveDependencyImages(a.Dependencies, r, false) var envTags map[string]string - envTags, err = quickMakeEnvTags(tags[a.ImageName]) + envTags, err = QuickMakeEnvTags(a.ImageName) if err != nil { return nil, err } diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 94955b4e8ff..37993f3f7aa 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -43,7 +43,7 @@ func TestSourceDependenciesCache(t *testing.T) { "img4": {"file41", "file42"}, } counts := map[string]int{"img1": 0, "img2": 0, "img3": 0, "img4": 0} - t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver, _ map[string]string) ([]string, error) { + t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver) ([]string, error) { counts[a.ImageName]++ return deps[a.ImageName], nil }) @@ -89,7 +89,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, test.dockerArtifactResolver, map[string]string{}) + paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, test.dockerArtifactResolver) t.CheckNoError(err) t.CheckDeepEqual(test.expectedPaths, paths, cmpopts.SortSlices(func(x, y string) bool { return x < y })) diff --git a/pkg/skaffold/runner/listen_test.go b/pkg/skaffold/runner/listen_test.go index f8d6af8a302..59dcf853c88 100644 --- a/pkg/skaffold/runner/listen_test.go +++ b/pkg/skaffold/runner/listen_test.go @@ -59,7 +59,7 @@ func (f *fakeDepsResolver) TransitiveArtifactDependencies(context.Context, *late return nil, nil } -func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact, map[string]string) ([]string, error) { +func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact) ([]string, error) { return nil, nil } diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index e2a8081b16d..4adc8518f97 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -117,11 +117,11 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold return nil, fmt.Errorf("creating actiosn runner: %w", err) } - depLister := func(ctx context.Context, artifact *latest.Artifact, tags map[string]string) ([]string, error) { + depLister := func(ctx context.Context, artifact *latest.Artifact) ([]string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "NewForConfig_depLister") defer endTrace() - buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, tags) + buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err diff --git a/pkg/skaffold/tag/custom_template_test.go b/pkg/skaffold/tag/custom_template_test.go index 46528d48acc..7530809d9c5 100644 --- a/pkg/skaffold/tag/custom_template_test.go +++ b/pkg/skaffold/tag/custom_template_test.go @@ -35,7 +35,7 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return []string{}, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, _ map[string]string) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) { return []string{}, nil } From 27845564691d9aa75bb4358d78f12089306265ab Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 02:17:48 +0000 Subject: [PATCH 11/44] make fixes to test --- pkg/skaffold/build/gcb/cloud_build.go | 2 +- pkg/skaffold/diagnose/diagnose.go | 3 +-- pkg/skaffold/diagnose/diagnose_test.go | 2 -- pkg/skaffold/graph/dependencies.go | 29 ++++++++++++++----------- pkg/skaffold/graph/dependencies_test.go | 11 +++++++--- pkg/skaffold/runner/listen_test.go | 2 +- pkg/skaffold/runner/new.go | 11 +++++++++- 7 files changed, 37 insertions(+), 23 deletions(-) diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 920ba600dec..266edbd0748 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -127,7 +127,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer }) } - dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact) + dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact, nil) if err != nil { return "", sErrors.NewErrorWithStatusCode(&proto.ActionableErr{ ErrCode: proto.StatusCode_BUILD_GCB_GET_DEPENDENCY_ERR, diff --git a/pkg/skaffold/diagnose/diagnose.go b/pkg/skaffold/diagnose/diagnose.go index 3075a5314d4..f8b1d58f5fa 100644 --- a/pkg/skaffold/diagnose/diagnose.go +++ b/pkg/skaffold/diagnose/diagnose.go @@ -123,8 +123,7 @@ func timeToListDependencies(ctx context.Context, a *latest.Artifact, cfg Config) start := time.Now() g := graph.ToArtifactGraph(cfg.Artifacts()) sourceDependencies := graph.NewSourceDependenciesCache(cfg, nil, g) - - paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a) + paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a, nil) return timeutil.Humanize(time.Since(start)), paths, err } diff --git a/pkg/skaffold/diagnose/diagnose_test.go b/pkg/skaffold/diagnose/diagnose_test.go index 260a48f500b..e79ab7399d8 100644 --- a/pkg/skaffold/diagnose/diagnose_test.go +++ b/pkg/skaffold/diagnose/diagnose_test.go @@ -81,8 +81,6 @@ func TestCheckArtifacts(t *testing.T) { testutil.Run(t, "", func(t *testutil.T) { tmpDir := t.NewTempDir().Write("Dockerfile", "FROM busybox") - // imgTag := "{{.IMAGE_TAG}}" - err := CheckArtifacts(context.Background(), &mockConfig{ artifacts: []*latest.Artifact{{ ImageName: "base", diff --git a/pkg/skaffold/graph/dependencies.go b/pkg/skaffold/graph/dependencies.go index 19ce8a176ff..0c05a4096ea 100644 --- a/pkg/skaffold/graph/dependencies.go +++ b/pkg/skaffold/graph/dependencies.go @@ -43,7 +43,7 @@ type SourceDependenciesCache interface { TransitiveArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) // SingleArtifactDependencies returns the source dependencies for only the target artifact. // The result (even if an error) is cached so that the function is evaluated only once for every artifact. The cache is reset before the start of the next devloop. - SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) + SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, envTags map[string]string) ([]string, error) // Reset removes the cached source dependencies for all artifacts Reset() } @@ -65,7 +65,15 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont }) defer endTrace() - deps, err := r.SingleArtifactDependencies(ctx, a) + tag, ok := r.artifactResolver.GetImageTag(a.ImageName) + if !ok { + return nil, fmt.Errorf("unable to resolve tag for image: %s", a.ImageName) + } + envTags, err := EnvTags(tag) + if err != nil { + return nil, fmt.Errorf("unable to parse build args from tag %s: err %w", tag, err) + } + deps, err := r.SingleArtifactDependencies(ctx, a, envTags) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err @@ -81,14 +89,14 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return deps, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, envTags map[string]string) ([]string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "SingleArtifactDependencies", map[string]string{ "ArtifactName": instrumentation.PII(a.ImageName), }) defer endTrace() res, err := r.cache.Exec(a.ImageName, func() ([]string, error) { - return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver) + return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver, envTags) }) if err != nil { endTrace(instrumentation.TraceEndError(err)) @@ -104,13 +112,12 @@ func (r *dependencyResolverImpl) Reset() { r.cache = util.NewSyncStore[[]string]() } -// QuickMakeEnvTags generate a set of build tags from the docker image name. -func QuickMakeEnvTags(tag string) (map[string]string, error) { +// EnvTags generate a set of build tags from the docker image name. +func EnvTags(tag string) (map[string]string, error) { imgRef, err := docker.ParseReference(tag) if err != nil { return nil, fmt.Errorf("couldn't parse image tag %s %w", tag, err) } - return map[string]string{ "IMAGE_REPO": imgRef.Repo, "IMAGE_NAME": imgRef.Name, @@ -119,7 +126,7 @@ func QuickMakeEnvTags(tag string) (map[string]string, error) { } // sourceDependenciesForArtifact returns the build dependencies for the current artifact. -func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver) ([]string, error) { +func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver, envTags map[string]string) ([]string, error) { var ( paths []string err error @@ -132,11 +139,7 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg // For single build scenarios like `build` and `run`, it is called for the cache hash calculations which are already handled in `artifactHasher`. // For `dev` it will succeed on the first dev loop and list any additional dependencies found from the base artifact's ONBUILD instructions as a file added instead of modified (see `filemon.Events`) deps := docker.ResolveDependencyImages(a.Dependencies, r, false) - var envTags map[string]string - envTags, err = QuickMakeEnvTags(a.ImageName) - if err != nil { - return nil, err - } + args, evalErr := docker.EvalBuildArgsWithEnv(cfg.Mode(), a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, deps, envTags) if evalErr != nil { return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr) diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 37993f3f7aa..f4eb9412272 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -28,6 +28,11 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/testutil" ) +type fakeResolver struct{} + +func (f *fakeResolver) GetImageTag(string) (string, bool) { + return "image:latest", true +} func TestSourceDependenciesCache(t *testing.T) { testutil.Run(t, "TestTransitiveSourceDependenciesCache", func(t *testutil.T) { g := map[string]*latest.Artifact{ @@ -43,12 +48,12 @@ func TestSourceDependenciesCache(t *testing.T) { "img4": {"file41", "file42"}, } counts := map[string]int{"img1": 0, "img2": 0, "img3": 0, "img4": 0} - t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver) ([]string, error) { + t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver, _ map[string]string) ([]string, error) { counts[a.ImageName]++ return deps[a.ImageName], nil }) - r := NewSourceDependenciesCache(nil, nil, g) + r := NewSourceDependenciesCache(nil, &fakeResolver{}, g) d, err := r.TransitiveArtifactDependencies(context.Background(), g["img1"]) t.CheckNoError(err) expectedDeps := []string{"file11", "file12", "file21", "file22", "file31", "file32", "file41", "file42", "file41", "file42"} @@ -89,7 +94,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, test.dockerArtifactResolver) + paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, test.dockerArtifactResolver, nil) t.CheckNoError(err) t.CheckDeepEqual(test.expectedPaths, paths, cmpopts.SortSlices(func(x, y string) bool { return x < y })) diff --git a/pkg/skaffold/runner/listen_test.go b/pkg/skaffold/runner/listen_test.go index 59dcf853c88..f8d6af8a302 100644 --- a/pkg/skaffold/runner/listen_test.go +++ b/pkg/skaffold/runner/listen_test.go @@ -59,7 +59,7 @@ func (f *fakeDepsResolver) TransitiveArtifactDependencies(context.Context, *late return nil, nil } -func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact) ([]string, error) { +func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact, map[string]string) ([]string, error) { return nil, nil } diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index 4adc8518f97..394990179b0 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -121,7 +121,16 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold ctx, endTrace := instrumentation.StartTrace(ctx, "NewForConfig_depLister") defer endTrace() - buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact) + tag, ok := store.GetImageTag(artifact.ImageName) + if !ok { + return nil, fmt.Errorf("unable to resolve tag for image: %s", artifact.ImageName) + } + envTags, err := graph.EnvTags(tag) + if err != nil { + return nil, fmt.Errorf("unable to get build args for tag %s, err: %w", tag, err) + } + + buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, envTags) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err From 630979ac0775d1608138f43cd939e5a42c169606 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 06:03:13 +0000 Subject: [PATCH 12/44] fix tests --- pkg/skaffold/docker/build_args_test.go | 13 ++---------- pkg/skaffold/docker/dockertest.go | 25 ++++++++++++++++++++++++ pkg/skaffold/graph/dependencies_test.go | 20 +++++++------------ pkg/skaffold/runner/new.go | 11 +---------- pkg/skaffold/tag/custom_template_test.go | 2 +- pkg/skaffold/tag/input_digest.go | 2 +- 6 files changed, 37 insertions(+), 36 deletions(-) create mode 100644 pkg/skaffold/docker/dockertest.go diff --git a/pkg/skaffold/docker/build_args_test.go b/pkg/skaffold/docker/build_args_test.go index 9ba27ab767f..bad76137011 100644 --- a/pkg/skaffold/docker/build_args_test.go +++ b/pkg/skaffold/docker/build_args_test.go @@ -226,13 +226,13 @@ func TestCreateBuildArgsFromArtifacts(t *testing.T) { }{ { description: "can resolve artifacts", - r: mockArtifactResolver{m: map[string]string{"img1": "tag1", "img2": "tag2", "img3": "tag3", "img4": "tag4"}}, + r: NewMockArtifactResolver(map[string]string{"img1": "tag1", "img2": "tag2", "img3": "tag3", "img4": "tag4"}), deps: []*latest.ArtifactDependency{{ImageName: "img3", Alias: "alias3"}, {ImageName: "img4", Alias: "alias4"}}, args: map[string]*string{"alias3": util.Ptr("tag3"), "alias4": util.Ptr("tag4")}, }, { description: "cannot resolve artifacts", - r: mockArtifactResolver{m: make(map[string]string)}, + r: NewMockArtifactResolver(map[string]string{}), args: map[string]*string{"alias3": nil, "alias4": nil}, deps: []*latest.ArtifactDependency{{ImageName: "img3", Alias: "alias3"}, {ImageName: "img4", Alias: "alias4"}}, }, @@ -281,12 +281,3 @@ func TestBuildArgTemplating(t *testing.T) { t.CheckMatches(*filledMap["SO_SECRET"], "do not say it") }) } - -type mockArtifactResolver struct { - m map[string]string -} - -func (r mockArtifactResolver) GetImageTag(imageName string) (string, bool) { - val, found := r.m[imageName] - return val, found -} diff --git a/pkg/skaffold/docker/dockertest.go b/pkg/skaffold/docker/dockertest.go new file mode 100644 index 00000000000..8ebf8a15298 --- /dev/null +++ b/pkg/skaffold/docker/dockertest.go @@ -0,0 +1,25 @@ +package docker + +// MockArtifactResolver mocks docker.ArtifactResolver interface. +type mockArtifactResolver struct { + m map[string]string +} + +// NewMockArtifactResolver returns a mock ArtifactResolver for testing. +func NewMockArtifactResolver(m map[string]string) *mockArtifactResolver { + return &mockArtifactResolver{m} +} + +// simpleMockArtifactResolver is an implementation of docker.ArtifactResolver +// that returns the same value for any key +type simpleMockArtifactResolver struct{} + +// GetImageTag is an implementation of docker.ArtifactResolver that +// always returns the same tag. +func (s *simpleMockArtifactResolver) GetImageTag(_ string) (string, bool) { + return "image:latest", true +} + +func NewSimpleMockArtifactResolver() ArtifactResolver { + return &simpleMockArtifactResolver{} +} diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index f4eb9412272..56fdfc0527e 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -28,11 +28,6 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/testutil" ) -type fakeResolver struct{} - -func (f *fakeResolver) GetImageTag(string) (string, bool) { - return "image:latest", true -} func TestSourceDependenciesCache(t *testing.T) { testutil.Run(t, "TestTransitiveSourceDependenciesCache", func(t *testutil.T) { g := map[string]*latest.Artifact{ @@ -47,13 +42,13 @@ func TestSourceDependenciesCache(t *testing.T) { "img3": {"file31", "file32"}, "img4": {"file41", "file42"}, } - counts := map[string]int{"img1": 0, "img2": 0, "img3": 0, "img4": 0} + counts := map[string]int{"izmg1": 0, "img2": 0, "img3": 0, "img4": 0} t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver, _ map[string]string) ([]string, error) { counts[a.ImageName]++ return deps[a.ImageName], nil }) - r := NewSourceDependenciesCache(nil, &fakeResolver{}, g) + r := NewSourceDependenciesCache(nil, docker.NewSimpleMockArtifactResolver(), g) d, err := r.TransitiveArtifactDependencies(context.Background(), g["img1"]) t.CheckNoError(err) expectedDeps := []string{"file11", "file12", "file21", "file22", "file31", "file32", "file41", "file42", "file41", "file42"} @@ -72,11 +67,10 @@ func TestSourceDependenciesForArtifact(t *testing.T) { "dir2/frob.go", ) tests := []struct { - description string - artifact *latest.Artifact - dockerConfig docker.Config - dockerArtifactResolver docker.ArtifactResolver - expectedPaths []string + description string + artifact *latest.Artifact + dockerConfig docker.Config + expectedPaths []string }{ { description: "ko default dependencies", @@ -94,7 +88,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, test.dockerArtifactResolver, nil) + paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, nil, nil) t.CheckNoError(err) t.CheckDeepEqual(test.expectedPaths, paths, cmpopts.SortSlices(func(x, y string) bool { return x < y })) diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index 394990179b0..15b1b5bff40 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -121,16 +121,7 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold ctx, endTrace := instrumentation.StartTrace(ctx, "NewForConfig_depLister") defer endTrace() - tag, ok := store.GetImageTag(artifact.ImageName) - if !ok { - return nil, fmt.Errorf("unable to resolve tag for image: %s", artifact.ImageName) - } - envTags, err := graph.EnvTags(tag) - if err != nil { - return nil, fmt.Errorf("unable to get build args for tag %s, err: %w", tag, err) - } - - buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, envTags) + buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, nil) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err diff --git a/pkg/skaffold/tag/custom_template_test.go b/pkg/skaffold/tag/custom_template_test.go index 7530809d9c5..80553912a8e 100644 --- a/pkg/skaffold/tag/custom_template_test.go +++ b/pkg/skaffold/tag/custom_template_test.go @@ -35,7 +35,7 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return []string{}, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, envTags map[string]string) ([]string, error) { return []string{}, nil } diff --git a/pkg/skaffold/tag/input_digest.go b/pkg/skaffold/tag/input_digest.go index 36b4674cc80..4273640dd4d 100644 --- a/pkg/skaffold/tag/input_digest.go +++ b/pkg/skaffold/tag/input_digest.go @@ -40,7 +40,7 @@ type inputDigestTagger struct { } func NewInputDigestTagger(cfg docker.Config, ag graph.ArtifactGraph) (Tagger, error) { - return NewInputDigestTaggerWithSourceCache(cfg, graph.NewSourceDependenciesCache(cfg, nil, ag)) + return NewInputDigestTaggerWithSourceCache(cfg, graph.NewSourceDependenciesCache(cfg, docker.NewSimpleMockArtifactResolver(), ag)) } func NewInputDigestTaggerWithSourceCache(cfg docker.Config, cache graph.SourceDependenciesCache) (Tagger, error) { From 65af3c983ada30d8aa90cae8ad95c805d5a33211 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 06:09:45 +0000 Subject: [PATCH 13/44] fix another typo --- pkg/skaffold/docker/dockertest.go | 5 +++++ pkg/skaffold/graph/dependencies_test.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/docker/dockertest.go b/pkg/skaffold/docker/dockertest.go index 8ebf8a15298..ef590501e4b 100644 --- a/pkg/skaffold/docker/dockertest.go +++ b/pkg/skaffold/docker/dockertest.go @@ -10,6 +10,11 @@ func NewMockArtifactResolver(m map[string]string) *mockArtifactResolver { return &mockArtifactResolver{m} } +func (r mockArtifactResolver) GetImageTag(imageName string) (string, bool) { + val, found := r.m[imageName] + return val, found +} + // simpleMockArtifactResolver is an implementation of docker.ArtifactResolver // that returns the same value for any key type simpleMockArtifactResolver struct{} diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 56fdfc0527e..61583e7d3de 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -42,7 +42,7 @@ func TestSourceDependenciesCache(t *testing.T) { "img3": {"file31", "file32"}, "img4": {"file41", "file42"}, } - counts := map[string]int{"izmg1": 0, "img2": 0, "img3": 0, "img4": 0} + counts := map[string]int{"img1": 0, "img2": 0, "img3": 0, "img4": 0} t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver, _ map[string]string) ([]string, error) { counts[a.ImageName]++ return deps[a.ImageName], nil From 7e6707847ab714b3e3737b016333baf49d556c8d Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 06:32:51 +0000 Subject: [PATCH 14/44] add license boilerplate --- pkg/skaffold/docker/dockertest.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/pkg/skaffold/docker/dockertest.go b/pkg/skaffold/docker/dockertest.go index ef590501e4b..54b8c14daa1 100644 --- a/pkg/skaffold/docker/dockertest.go +++ b/pkg/skaffold/docker/dockertest.go @@ -1,3 +1,19 @@ +/* +Copyright 2025 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 docker // MockArtifactResolver mocks docker.ArtifactResolver interface. From ae699c0619287d8502b71a58fce5e36e332f781d Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 14:22:22 +0000 Subject: [PATCH 15/44] add 2025 as a valid year --- hack/boilerplate/boilerplate.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/boilerplate/boilerplate.py b/hack/boilerplate/boilerplate.py index 22af9f0ce06..a949ea6c975 100644 --- a/hack/boilerplate/boilerplate.py +++ b/hack/boilerplate/boilerplate.py @@ -145,7 +145,7 @@ def get_regexs(): # Search for "YEAR" which exists in the boilerplate, but shouldn't in the real thing regexs["year"] = re.compile( 'YEAR' ) # dates can be 2018, company holder names can be anything - regexs["date"] = re.compile( '(2019|2020|2021|2022|2023|2024)' ) + regexs["date"] = re.compile( '(2019|2020|2021|2022|2023|2024|2025)' ) # strip // +build \n\n build constraints regexs["go_build_constraints"] = re.compile(r"^(// \+build.*\n)+\n", re.MULTILINE) # strip //go:build \n build constraints (for go1.17 and higher) From 0b1901226527a0dd58e7a8a847f46b928dba5140 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 20:08:20 +0000 Subject: [PATCH 16/44] chore:add integration test --- integration/build_test.go | 29 +++++++++++++++++++ .../base/Dockerfile | 11 +++++++ .../docker-run-with-build-args/base/go.mod | 3 ++ .../docker-run-with-build-args/base/main.go | 13 +++++++++ .../child/Dockerfile | 5 ++++ .../docker-run-with-build-args/skaffold.yaml | 29 +++++++++++++++++++ 6 files changed, 90 insertions(+) create mode 100644 integration/testdata/docker-run-with-build-args/base/Dockerfile create mode 100644 integration/testdata/docker-run-with-build-args/base/go.mod create mode 100644 integration/testdata/docker-run-with-build-args/base/main.go create mode 100644 integration/testdata/docker-run-with-build-args/child/Dockerfile create mode 100644 integration/testdata/docker-run-with-build-args/skaffold.yaml diff --git a/integration/build_test.go b/integration/build_test.go index eb46951049f..444bd89868b 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -37,6 +37,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/v2/testutil" + "k8s.io/apimachinery/pkg/util/wait" ) const imageName = "us-central1-docker.pkg.dev/k8s-skaffold/testing/simple-build:" @@ -331,3 +332,31 @@ func failNowIfError(t Fataler, err error) { t.Fatal(err) } } + +func TestRunWithDockerAndBuildArgs(t *testing.T) { + tests := []struct { + description string + projectDir string + }{ + { + description: "IMAGE_REPO, IMAGE_TAG, and IMAGE_NAME are passed to the docker build args", + projectDir: "testdata/docker-run-with-build-args", + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + + skaffold.Build().InDir(test.projectDir).Run(t.T) + + defer skaffold.Delete().InDir(test.projectDir).Run(t.T) + expected := "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest" + + err := wait.PollImmediate(time.Millisecond*500, 1*time.Minute, func() (bool, error) { + out, _ := exec.Command("docker", "run", "child:latest").Output() + return string(out) == expected, nil + }) + failNowIfError(t, err) + }) + } +} \ No newline at end of file diff --git a/integration/testdata/docker-run-with-build-args/base/Dockerfile b/integration/testdata/docker-run-with-build-args/base/Dockerfile new file mode 100644 index 00000000000..24eb993a214 --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/base/Dockerfile @@ -0,0 +1,11 @@ +FROM golang:1.23 as builder +WORKDIR /code +COPY main.go . +COPY go.mod . +# `skaffold debug` sets SKAFFOLD_GO_GCFLAGS to disable compiler optimizations +ARG SKAFFOLD_GO_GCFLAGS +ARG IMAGE_REPO +ARG IMAGE_NAME +ARG IMAGE_TAG +RUN go build -gcflags="${SKAFFOLD_GO_GCFLAGS}" -ldflags="-X main.ImageRepo=${IMAGE_REPO} -X main.ImageName=${IMAGE_NAME} -X main.ImageTag=${IMAGE_TAG}" -trimpath -o /app main.go + diff --git a/integration/testdata/docker-run-with-build-args/base/go.mod b/integration/testdata/docker-run-with-build-args/base/go.mod new file mode 100644 index 00000000000..447daad7f1f --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/base/go.mod @@ -0,0 +1,3 @@ +module github.com/GoogleContainerTools/skaffold/examples/docker-deploy-with-build-args/base + +go 1.23 diff --git a/integration/testdata/docker-run-with-build-args/base/main.go b/integration/testdata/docker-run-with-build-args/base/main.go new file mode 100644 index 00000000000..87d0c5047f8 --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/base/main.go @@ -0,0 +1,13 @@ +package main + +import ( + "fmt" +) + +var ImageRepo = "unknown" +var ImageTag = "unknown" +var ImageName = "unknown" + +func main() { + fmt.Printf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s", ImageRepo, ImageName, ImageTag) +} diff --git a/integration/testdata/docker-run-with-build-args/child/Dockerfile b/integration/testdata/docker-run-with-build-args/child/Dockerfile new file mode 100644 index 00000000000..0c931200956 --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/child/Dockerfile @@ -0,0 +1,5 @@ +ARG BASE +FROM $BASE as parent +FROM alpine +COPY --from=parent /app . +CMD ["./app"] diff --git a/integration/testdata/docker-run-with-build-args/skaffold.yaml b/integration/testdata/docker-run-with-build-args/skaffold.yaml new file mode 100644 index 00000000000..a9a79bb4ec5 --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/skaffold.yaml @@ -0,0 +1,29 @@ +apiVersion: skaffold/v4beta12 +kind: Config +build: + tagPolicy: + sha256: {} + local: + push: false + useDockerCLI: true + artifacts: + - image: gcr.io/k8s-skaffold/skaffold + context: base + docker: + dockerfile: Dockerfile + noCache: true + buildArgs: + IMAGE_REPO: '{{.IMAGE_REPO}}' + IMAGE_NAME: '{{.IMAGE_NAME}}' + IMAGE_TAG: '{{.IMAGE_TAG}}' + - image: child + requires: + - image: gcr.io/k8s-skaffold/skaffold + alias: BASE + context: child + docker: + dockerfile: Dockerfile + noCache: true +deploy: + docker: + images: [child] From da11b021479f227dd4337090b2ce3c3ff8680527 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 20:22:06 +0000 Subject: [PATCH 17/44] format go file --- integration/testdata/docker-run-with-build-args/base/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/testdata/docker-run-with-build-args/base/main.go b/integration/testdata/docker-run-with-build-args/base/main.go index 87d0c5047f8..70b7cb8a9da 100644 --- a/integration/testdata/docker-run-with-build-args/base/main.go +++ b/integration/testdata/docker-run-with-build-args/base/main.go @@ -9,5 +9,5 @@ var ImageTag = "unknown" var ImageName = "unknown" func main() { - fmt.Printf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s", ImageRepo, ImageName, ImageTag) + fmt.Printf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s", ImageRepo, ImageName, ImageTag) } From 0787e5a9f730dfeedca0e5c0d16e07941aa61b12 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 21:02:05 +0000 Subject: [PATCH 18/44] fix: lint and test --- integration/build_test.go | 10 +++++----- .../testdata/docker-run-with-build-args/base/main.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/integration/build_test.go b/integration/build_test.go index 444bd89868b..9d29a3d906e 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -335,12 +335,12 @@ func failNowIfError(t Fataler, err error) { func TestRunWithDockerAndBuildArgs(t *testing.T) { tests := []struct { - description string - projectDir string + description string + projectDir string }{ { - description: "IMAGE_REPO, IMAGE_TAG, and IMAGE_NAME are passed to the docker build args", - projectDir: "testdata/docker-run-with-build-args", + description: "IMAGE_REPO, IMAGE_TAG, and IMAGE_NAME are passed to the docker build args", + projectDir: "testdata/docker-run-with-build-args", }, } @@ -359,4 +359,4 @@ func TestRunWithDockerAndBuildArgs(t *testing.T) { failNowIfError(t, err) }) } -} \ No newline at end of file +} diff --git a/integration/testdata/docker-run-with-build-args/base/main.go b/integration/testdata/docker-run-with-build-args/base/main.go index 70b7cb8a9da..a54c49f2fff 100644 --- a/integration/testdata/docker-run-with-build-args/base/main.go +++ b/integration/testdata/docker-run-with-build-args/base/main.go @@ -9,5 +9,5 @@ var ImageTag = "unknown" var ImageName = "unknown" func main() { - fmt.Printf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s", ImageRepo, ImageName, ImageTag) + fmt.Printf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s\n", ImageRepo, ImageName, ImageTag) } From 10ed9ecb4d1d7f6f25437465496c36ace68eece5 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 21:17:14 +0000 Subject: [PATCH 19/44] fix test --- integration/build_test.go | 8 +++++++- .../testdata/docker-run-with-build-args/base/main.go | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/integration/build_test.go b/integration/build_test.go index 9d29a3d906e..98ba74282e3 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -351,11 +351,17 @@ func TestRunWithDockerAndBuildArgs(t *testing.T) { defer skaffold.Delete().InDir(test.projectDir).Run(t.T) expected := "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest" + got := "" err := wait.PollImmediate(time.Millisecond*500, 1*time.Minute, func() (bool, error) { out, _ := exec.Command("docker", "run", "child:latest").Output() - return string(out) == expected, nil + got = strings.Trim(string(out), " \n") + return got == expected, nil }) + + if err != nil { + t.Errorf("docker run produced incorrect output, got:[%s], want:[%s], err: %v", got, expected, err ) + } failNowIfError(t, err) }) } diff --git a/integration/testdata/docker-run-with-build-args/base/main.go b/integration/testdata/docker-run-with-build-args/base/main.go index a54c49f2fff..6cd9c0ba221 100644 --- a/integration/testdata/docker-run-with-build-args/base/main.go +++ b/integration/testdata/docker-run-with-build-args/base/main.go @@ -9,5 +9,6 @@ var ImageTag = "unknown" var ImageName = "unknown" func main() { - fmt.Printf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s\n", ImageRepo, ImageName, ImageTag) + output := fmt.Sprintf("IMAGE_REPO: %s, IMAGE_NAME: %s, IMAGE_TAG:%s\n", ImageRepo, ImageName, ImageTag) + fmt.Println(output) } From 104f90856212d6546ae1e923858b356df1b474c4 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Thu, 16 Jan 2025 22:09:30 +0000 Subject: [PATCH 20/44] fix:lint --- integration/build_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/build_test.go b/integration/build_test.go index 98ba74282e3..eb802f9256e 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -360,7 +360,7 @@ func TestRunWithDockerAndBuildArgs(t *testing.T) { }) if err != nil { - t.Errorf("docker run produced incorrect output, got:[%s], want:[%s], err: %v", got, expected, err ) + t.Errorf("docker run produced incorrect output, got:[%s], want:[%s], err: %v", got, expected, err) } failNowIfError(t, err) }) From 0469ddd0a698fe1b6df66b2fa18213fa66c4e801 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 15:30:36 +0000 Subject: [PATCH 21/44] add debug output --- integration/build_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integration/build_test.go b/integration/build_test.go index eb802f9256e..36d9081bb12 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -355,6 +355,7 @@ func TestRunWithDockerAndBuildArgs(t *testing.T) { err := wait.PollImmediate(time.Millisecond*500, 1*time.Minute, func() (bool, error) { out, _ := exec.Command("docker", "run", "child:latest").Output() + t.Logf("Output:[%s]\n", out) got = strings.Trim(string(out), " \n") return got == expected, nil }) From 20c9e2a21c66574f309c8aca1be6635304fe6176 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 16:53:37 +0000 Subject: [PATCH 22/44] add test action for debugging --- .github/workflows/test-linux.yml | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 .github/workflows/test-linux.yml diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml new file mode 100644 index 00000000000..e69de29bb2d From 6090a405a25414c8c7577e1b10de695785a17307 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 16:57:31 +0000 Subject: [PATCH 23/44] add test --- .github/workflows/test-linux.yml | 141 +++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml index e69de29bb2d..f271641eedb 100644 --- a/.github/workflows/test-linux.yml +++ b/.github/workflows/test-linux.yml @@ -0,0 +1,141 @@ +name: Test Docker run + +# Triggers the workflow on push or pull request events +on: [push, pull_request] + +permissions: read-all + +concurrency: + group: build-${{ github.event.pull_request.number || github.ref }}-${{github.workflow}} + cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} + +jobs: + + build: + name: PR integration tests (linux) + runs-on: ubuntu-latest + strategy: + matrix: + kustomize_version: [5.5.0] + ko_version: [0.17.1] + kompose_version: [1.34.0] + kpt_version: [1.0.0-beta.55] + minikube_version: [1.34.0] + gcloud_sdk_version: [502.0.0] + container_structure_tests_version: [1.19.3] + java: [21] + integration_test_partitions: [0, 1, 2, 3] + steps: + + - name: Check out code into the Go module directory + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: 1.23.* + id: go + + - name: Set up Java + uses: actions/setup-java@v4 + with: + distribution: 'temurin' + java-version: ${{ matrix.java }} + + # Retrieve build locations with `go env` + # + - id: go-cache-paths + run: | + echo "go-build=$(go env GOCACHE)" >> $GITHUB_OUTPUT + echo "go-mod=$(go env GOMODCACHE)" >> $GITHUB_OUTPUT + + - uses: actions/cache@v4 + with: + path: | + ${{ steps.go-cache-paths.outputs.go-build }} + ${{ steps.go-cache-paths.outputs.go-mod }} + key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ runner.os }}-go- + + # Skip integration tests for `docs`-only changes (only works for PR-based dev workflows like Skaffold's). + # NOTE: grep output is stored in env var with `|| true` as the run command cannot fail or action will fail + - name: Check if only docs changes were made in this PR + run: | + echo ${{ github.event.before }} + echo ${{ github.event.after }} + NON_DOCS_FILES_CHANGED=$(git diff --name-only ${{ github.event.pull_request.base.sha }}..${{ github.sha }}| grep -v '^docs/' || true) + echo "NON_DOCS_FILES_CHANGED=${#NON_DOCS_FILES_CHANGED}" >> $GITHUB_ENV # get the char len of diff output (used later) + + - name: Install Kustomize + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + wget -O kustomize.tar.gz https://github.com/kubernetes-sigs/kustomize/releases/download/kustomize/v${{ matrix.kustomize_version }}/kustomize_v${{ matrix.kustomize_version }}_linux_amd64.tar.gz + sudo tar -xvf kustomize.tar.gz -C /usr/local/bin/ + + - name: Install Ko + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + wget -O ko.tar.gz https://github.com/google/ko/releases/download/v${{ matrix.ko_version }}/ko_${{ matrix.ko_version }}_Linux_x86_64.tar.gz + sudo tar -xvf ko.tar.gz -C /usr/local/bin/ + + - name: Install Kompose + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + wget -O kompose https://github.com/kubernetes/kompose/releases/download/v${{ matrix.kompose_version }}/kompose-linux-amd64 && chmod +x kompose + sudo mv kompose /usr/local/bin/ + + - name: Install Kpt + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + wget -O kpt https://github.com/GoogleContainerTools/kpt/releases/download/v${{ matrix.kpt_version }}/kpt_linux_amd64 && chmod +x kpt + sudo mv kpt /usr/local/bin/ + + - name: Install GCloud + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + wget -O gcloud.tar.gz https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud-sdk-${{ matrix.gcloud_sdk_version }}-linux-x86_64.tar.gz + tar -xvf gcloud.tar.gz -C ${HOME}/ + ${HOME}/google-cloud-sdk/install.sh --usage-reporting=false --bash-completion=false --disable-installation-options + echo "${HOME}/google-cloud-sdk/bin" >> $GITHUB_PATH + + - name: Configure GCloud with Docker + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: gcloud auth configure-docker + + - name: Install Container Structure Test + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + wget -O container-structure-test https://github.com/GoogleContainerTools/container-structure-test/releases/download/v${{ matrix.container_structure_tests_version }}/container-structure-test-linux-amd64 && chmod +x container-structure-test + sudo mv container-structure-test /usr/local/bin/ + + - name: Setup other files and permissions + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + sudo chown $(whoami):docker ${HOME}/.docker -R + sudo chmod g+rw ${HOME}/.docker -R + echo '{}' > ${HOME}/.docker/config.json + mkdir -p ${HOME}/.m2/ && cp ./hack/maven/settings.xml ${HOME}/.m2/settings.xml + + - name: Install Minikube and start cluster + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + curl -Lo minikube https://github.com/kubernetes/minikube/releases/download/v${{ matrix.minikube_version }}/minikube-linux-amd64 + sudo install minikube /usr/local/bin/minikube + minikube start --profile=minikube --driver=docker + + - name: Make and install Skaffold binary from current PR + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} # non docs files were changed, skaffold build needed + run: | + make + sudo install "${HOME}/work/skaffold/skaffold/out/skaffold" /usr/local/bin/skaffold + + - name: Run integration tests + if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} + run: | + cd integration/testdata/docker-run-with-build-args + skaffold config set --global collect-metrics false + skaffold build + docker run child:latest \ No newline at end of file From a212113c8f905489d23bde3d1058edd20c8b69d8 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 17:07:02 +0000 Subject: [PATCH 24/44] add test --- .github/workflows/test-linux.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml index f271641eedb..0ea86c7e09b 100644 --- a/.github/workflows/test-linux.yml +++ b/.github/workflows/test-linux.yml @@ -138,4 +138,5 @@ jobs: cd integration/testdata/docker-run-with-build-args skaffold config set --global collect-metrics false skaffold build - docker run child:latest \ No newline at end of file + docker image list + docker run child \ No newline at end of file From d52f74492b0b9de90ad2f404ae0d3418a8048583 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 17:15:07 +0000 Subject: [PATCH 25/44] add test --- .github/workflows/test-linux.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml index 0ea86c7e09b..a5090b0da5e 100644 --- a/.github/workflows/test-linux.yml +++ b/.github/workflows/test-linux.yml @@ -137,6 +137,4 @@ jobs: run: | cd integration/testdata/docker-run-with-build-args skaffold config set --global collect-metrics false - skaffold build - docker image list - docker run child \ No newline at end of file + skaffold run --tail \ No newline at end of file From 8f7960a5e42826d747c899d1119b3097ef037578 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 18:05:39 +0000 Subject: [PATCH 26/44] add test --- .github/workflows/test-linux.yml | 5 ++--- integration/build_test.go | 3 ++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml index a5090b0da5e..560a9399f6d 100644 --- a/.github/workflows/test-linux.yml +++ b/.github/workflows/test-linux.yml @@ -23,8 +23,6 @@ jobs: minikube_version: [1.34.0] gcloud_sdk_version: [502.0.0] container_structure_tests_version: [1.19.3] - java: [21] - integration_test_partitions: [0, 1, 2, 3] steps: - name: Check out code into the Go module directory @@ -137,4 +135,5 @@ jobs: run: | cd integration/testdata/docker-run-with-build-args skaffold config set --global collect-metrics false - skaffold run --tail \ No newline at end of file + skaffold build --cache-artifacts=false --verbosity=debug + docker run child:latest \ No newline at end of file diff --git a/integration/build_test.go b/integration/build_test.go index 36d9081bb12..32544900eaa 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -347,7 +347,8 @@ func TestRunWithDockerAndBuildArgs(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - skaffold.Build().InDir(test.projectDir).Run(t.T) + ns, _ := DefaultNamespace(t.T) + skaffold.Build().InNs(ns.Name).InDir(test.projectDir).Run(t.T) defer skaffold.Delete().InDir(test.projectDir).Run(t.T) expected := "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest" From a0569f3f808c6e94003b7364f5dd4bb82cf66690 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 18:33:13 +0000 Subject: [PATCH 27/44] add test --- .github/workflows/test-linux.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml index 560a9399f6d..d3fda619183 100644 --- a/.github/workflows/test-linux.yml +++ b/.github/workflows/test-linux.yml @@ -23,6 +23,7 @@ jobs: minikube_version: [1.34.0] gcloud_sdk_version: [502.0.0] container_structure_tests_version: [1.19.3] + java: [21] steps: - name: Check out code into the Go module directory From 09b820af8e902d49c62d3962c9d7642332d5cc8e Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 19:22:56 +0000 Subject: [PATCH 28/44] use the default context when building the image --- .github/workflows/test-linux.yml | 4 +--- integration/build_test.go | 5 +++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml index d3fda619183..a3495c63e73 100644 --- a/.github/workflows/test-linux.yml +++ b/.github/workflows/test-linux.yml @@ -134,7 +134,5 @@ jobs: - name: Run integration tests if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} run: | - cd integration/testdata/docker-run-with-build-args skaffold config set --global collect-metrics false - skaffold build --cache-artifacts=false --verbosity=debug - docker run child:latest \ No newline at end of file + INTEGRATION_TEST_ARGS="-run=TestRunWithDockerAndBuildArgs/" make integration diff --git a/integration/build_test.go b/integration/build_test.go index 32544900eaa..d2fb73cf7e5 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -337,18 +337,19 @@ func TestRunWithDockerAndBuildArgs(t *testing.T) { tests := []struct { description string projectDir string + args []string }{ { description: "IMAGE_REPO, IMAGE_TAG, and IMAGE_NAME are passed to the docker build args", projectDir: "testdata/docker-run-with-build-args", + args: []string{"--kube-context", "default"}, }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - ns, _ := DefaultNamespace(t.T) - skaffold.Build().InNs(ns.Name).InDir(test.projectDir).Run(t.T) + skaffold.Build(test.args...).InDir(test.projectDir).Run(t.T) defer skaffold.Delete().InDir(test.projectDir).Run(t.T) expected := "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest" From 19cc68ed5d0cdacf41b411a48b11de8fd090fb53 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 19:40:19 +0000 Subject: [PATCH 29/44] fix imports --- integration/build_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/build_test.go b/integration/build_test.go index d2fb73cf7e5..06ecb8dc7ec 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -28,6 +28,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" v1 "github.com/opencontainers/image-spec/specs-go/v1" + "k8s.io/apimachinery/pkg/util/wait" "github.com/GoogleContainerTools/skaffold/v2/cmd/skaffold/app/flags" "github.com/GoogleContainerTools/skaffold/v2/integration/skaffold" @@ -37,7 +38,6 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/v2/testutil" - "k8s.io/apimachinery/pkg/util/wait" ) const imageName = "us-central1-docker.pkg.dev/k8s-skaffold/testing/simple-build:" From 6ea296a78ac07f024adde7ea305d421f05631a9a Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Fri, 17 Jan 2025 21:04:13 +0000 Subject: [PATCH 30/44] fix: flaky helm deploy unit test by making the order of helm command calls consistent --- pkg/skaffold/deploy/helm/helm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/skaffold/deploy/helm/helm.go b/pkg/skaffold/deploy/helm/helm.go index 4f2f44136ce..3102fd8d952 100644 --- a/pkg/skaffold/deploy/helm/helm.go +++ b/pkg/skaffold/deploy/helm/helm.go @@ -312,6 +312,10 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art olog.Entry(ctx).Infof("Installing releases (%d releases)", len(releases)) } + // sort releases in the same level, this is merely to ensure that the series of helm commands are in order for + // consistency in unit testing. + sort.Strings(releases) + // Deploy releases in current level for _, releaseName := range releases { release := releaseNameToRelease[releaseName] From fd7d94d6c1eb0da66e591c4d6497d198294f1b8c Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Sat, 18 Jan 2025 00:16:36 +0000 Subject: [PATCH 31/44] fix lint --- integration/build_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/integration/build_test.go b/integration/build_test.go index 06ecb8dc7ec..23276c038c5 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -348,10 +348,9 @@ func TestRunWithDockerAndBuildArgs(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - skaffold.Build(test.args...).InDir(test.projectDir).Run(t.T) - defer skaffold.Delete().InDir(test.projectDir).Run(t.T) + expected := "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest" got := "" From 954da5cb3d620d343670763566f8620be41ea854 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 04:50:18 +0000 Subject: [PATCH 32/44] add unit tests and fix existing tests --- = | 30 +++++++ cmd/skaffold/app/cmd/diagnose.go | 13 ++- pkg/skaffold/build/cache/cache.go | 2 +- pkg/skaffold/build/cache/hash.go | 16 ++-- pkg/skaffold/build/cache/hash_test.go | 24 +++--- pkg/skaffold/build/cache/lookup.go | 2 +- pkg/skaffold/build/cache/lookup_test.go | 4 +- pkg/skaffold/build/cache/retrieve_test.go | 2 +- pkg/skaffold/build/gcb/cloud_build.go | 2 +- pkg/skaffold/deploy/util/util.go | 88 ++++++++++++++++++++ pkg/skaffold/diagnose/diagnose.go | 16 ++-- pkg/skaffold/diagnose/diagnose_test.go | 5 +- pkg/skaffold/docker/dependencies_test.go | 21 ----- pkg/skaffold/docker/dockertest.go | 74 +++++++++++++++++ pkg/skaffold/graph/dependencies.go | 20 ++--- pkg/skaffold/graph/dependencies_test.go | 60 ++++++++++++-- pkg/skaffold/runner/build.go | 97 ++--------------------- pkg/skaffold/runner/listen_test.go | 2 +- pkg/skaffold/runner/new.go | 4 +- pkg/skaffold/tag/custom_template_test.go | 2 +- 20 files changed, 318 insertions(+), 166 deletions(-) create mode 100644 = diff --git a/= b/= new file mode 100644 index 00000000000..6077aff7d3a --- /dev/null +++ b/= @@ -0,0 +1,30 @@ +ARGC: 1 +ARGIND: 0 +ARGV: array, 1 elements +BINMODE: 0 +CONVFMT: "%.6g" +ENVIRON: array, 73 elements +ERRNO: "" +FIELDWIDTHS: "" +FILENAME: "-" +FNR: 22 +FPAT: "[^[:space:]]+" +FS: " " +FUNCTAB: array, 42 elements +IGNORECASE: 0 +LINT: 0 +NF: 2 +NR: 22 +OFMT: "%.6g" +OFS: " " +ORS: "\n" +PREC: 53 +PROCINFO: array, 140 elements +RLENGTH: 0 +ROUNDMODE: "N" +RS: "\n" +RSTART: 0 +RT: "\n" +SUBSEP: "\034" +SYMTAB: array, 28 elements +TEXTDOMAIN: "messages" diff --git a/cmd/skaffold/app/cmd/diagnose.go b/cmd/skaffold/app/cmd/diagnose.go index 05437b1622c..abc9baf890a 100644 --- a/cmd/skaffold/app/cmd/diagnose.go +++ b/cmd/skaffold/app/cmd/diagnose.go @@ -22,6 +22,8 @@ import ( "io" "os" + deployutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/util" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" "github.com/spf13/cobra" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/diagnose" @@ -106,13 +108,20 @@ func printArtifactDiagnostics(ctx context.Context, out io.Writer, configs []sche if err != nil { return fmt.Errorf("getting run context: %w", err) } + tagger, err := tag.NewTaggerMux(runCtx) + if err != nil { + return fmt.Errorf("getting tagger: %w", err) + } + imageTags, err := deployutil.ImageTags(ctx, runCtx, tagger, out, runCtx.Artifacts()) + if err != nil { + return fmt.Errorf("getting tags: %w", err) + } for _, c := range configs { config := c.(*latest.SkaffoldConfig) fmt.Fprintln(out, "Skaffold version:", version.Get().GitCommit) fmt.Fprintln(out, "Configuration version:", config.APIVersion) fmt.Fprintln(out, "Number of artifacts:", len(config.Build.Artifacts)) - - if err := diagnose.CheckArtifacts(ctx, runCtx, out); err != nil { + if err := diagnose.CheckArtifacts(ctx, runCtx, imageTags, out); err != nil { return fmt.Errorf("running diagnostic on artifacts: %w", err) } diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 371ffb104f2..ca32e87ccbc 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -61,7 +61,7 @@ type cache struct { } // DependencyLister fetches a list of dependencies for an artifact -type DependencyLister func(ctx context.Context, artifact *latest.Artifact) ([]string, error) +type DependencyLister func(ctx context.Context, artifact *latest.Artifact, tag string) ([]string, error) type Config interface { docker.Config diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index 7f758c4d668..5bfb2715a0c 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -47,7 +47,7 @@ var ( ) type artifactHasher interface { - hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) + hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, string) (string, error) } type artifactHasherImpl struct { @@ -67,20 +67,20 @@ func newArtifactHasher(artifacts graph.ArtifactGraph, lister DependencyLister, m } } -func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Resolver, tags map[string]string) (string, error) { +func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Resolver, tag string) (string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "hash_GenerateHashOneArtifact", map[string]string{ "ImageName": instrumentation.PII(a.ImageName), }) defer endTrace() - hash, err := h.safeHash(ctx, out, a, platforms.GetPlatforms(a.ImageName)) + hash, err := h.safeHash(ctx, out, a, platforms.GetPlatforms(a.ImageName), tag) if err != nil { endTrace(instrumentation.TraceEndError(err)) return "", err } hashes := []string{hash} for _, dep := range sortedDependencies(a, h.artifacts) { - depHash, err := h.hash(ctx, out, dep, platforms, tags) + depHash, err := h.hash(ctx, out, dep, platforms, tag) if err != nil { endTrace(instrumentation.TraceEndError(err)) return "", err @@ -94,15 +94,15 @@ func (h *artifactHasherImpl) hash(ctx context.Context, out io.Writer, a *latest. return encode(hashes) } -func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher) (string, error) { +func (h *artifactHasherImpl) safeHash(ctx context.Context, out io.Writer, a *latest.Artifact, platforms platform.Matcher, tag string) (string, error) { return h.syncStore.Exec(a.ImageName, func() (string, error) { - return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms) + return singleArtifactHash(ctx, out, h.lister, a, h.mode, platforms, tag) }) } // singleArtifactHash calculates the hash for a single artifact, and ignores its required artifacts. -func singleArtifactHash(ctx context.Context, out io.Writer, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher) (string, error) { +func singleArtifactHash(ctx context.Context, out io.Writer, depLister DependencyLister, a *latest.Artifact, mode config.RunMode, m platform.Matcher, tag string) (string, error) { var inputs []string // Append the artifact's configuration @@ -113,7 +113,7 @@ func singleArtifactHash(ctx context.Context, out io.Writer, depLister Dependency inputs = append(inputs, config) // Append the digest of each input file - deps, err := depLister(ctx, a) + deps, err := depLister(ctx, a, tag) if err != nil { return "", fmt.Errorf("getting dependencies for %q: %w", a.ImageName, err) } diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index 7bcdcc1f8df..a90dceba5d8 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -31,7 +31,7 @@ import ( ) func stubDependencyLister(dependencies []string) DependencyLister { - return func(context.Context, *latest.Artifact) ([]string, error) { + return func(context.Context, *latest.Artifact, string) ([]string, error) { return dependencies, nil } } @@ -50,6 +50,8 @@ var fakeArtifactConfig = func(a *latest.Artifact) (string, error) { return "", nil } +var testTag = "gcr.io/my-repo/my-image:latest" + const Dockerfile = "Dockerfile" func TestGetHashForArtifact(t *testing.T) { @@ -162,7 +164,7 @@ func TestGetHashForArtifact(t *testing.T) { } depLister := stubDependencyLister(test.dependencies) - actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms, nil) + actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -242,11 +244,11 @@ func TestGetHashForArtifactWithDependencies(t *testing.T) { } } - depLister := func(_ context.Context, a *latest.Artifact) ([]string, error) { + depLister := func(_ context.Context, a *latest.Artifact, tag string) ([]string, error) { return test.fileDeps[a.ImageName], nil } - actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}, nil) + actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -309,21 +311,21 @@ func TestBuildArgs(t *testing.T) { } t.Override(&fileHasherFunc, mockCacheHasher) t.Override(&artifactConfigFunc, fakeArtifactConfig) - actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) + actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change order of buildargs artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"two": util.Ptr("2"), "one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change build args, get different hash artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) if actual == test.expected { @@ -356,7 +358,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { t.Override(&artifactConfigFunc, fakeArtifactConfig) depLister := stubDependencyLister([]string{"graph"}) - hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) + hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) @@ -366,7 +368,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { return []string{"FOO=baz"} } - hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, nil) + hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) if hash1 == hash2 { @@ -423,7 +425,7 @@ func TestCacheHasher(t *testing.T) { path := originalFile depLister := stubDependencyLister([]string{tmpDir.Path(originalFile)}) - oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, nil) + oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, testTag) t.CheckNoError(err) test.update(originalFile, tmpDir) @@ -432,7 +434,7 @@ func TestCacheHasher(t *testing.T) { } depLister = stubDependencyLister([]string{tmpDir.Path(path)}) - newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, nil) + newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckFalse(test.differentHash && oldHash == newHash) diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index f86a2d92d45..aa84c43a70d 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -64,7 +64,7 @@ func (c *cache) lookup(ctx context.Context, out io.Writer, a *latest.Artifact, t }) defer endTrace() - hash, err := h.hash(ctx, out, a, platforms, tags) + hash, err := h.hash(ctx, out, a, platforms, tag) if err != nil { return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)} } diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go index 2c6b33cdb89..3e01c90d69b 100644 --- a/pkg/skaffold/build/cache/lookup_test.go +++ b/pkg/skaffold/build/cache/lookup_test.go @@ -233,7 +233,7 @@ type mockHasher struct { val string } -func (m mockHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) { +func (m mockHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, string) (string, error) { return m.val, nil } @@ -241,7 +241,7 @@ type failingHasher struct { err error } -func (f failingHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, map[string]string) (string, error) { +func (f failingHasher) hash(context.Context, io.Writer, *latest.Artifact, platform.Resolver, string) (string, error) { return "", f.err } diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index 9a21409ba5a..171a0873474 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -37,7 +37,7 @@ import ( ) func depLister(files map[string][]string) DependencyLister { - return func(_ context.Context, artifact *latest.Artifact) ([]string, error) { + return func(_ context.Context, artifact *latest.Artifact, tag string) ([]string, error) { list, found := files[artifact.ImageName] if !found { return nil, errors.New("unknown artifact") diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 266edbd0748..2ec89efc4ab 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -127,7 +127,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer }) } - dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact, nil) + dependencies, err := b.sourceDependencies.SingleArtifactDependencies(ctx, artifact, tag) if err != nil { return "", sErrors.NewErrorWithStatusCode(&proto.ActionableErr{ ErrCode: proto.StatusCode_BUILD_GCB_GET_DEPENDENCY_ERR, diff --git a/pkg/skaffold/deploy/util/util.go b/pkg/skaffold/deploy/util/util.go index 72e5f26dfe5..071fa107d0d 100644 --- a/pkg/skaffold/deploy/util/util.go +++ b/pkg/skaffold/deploy/util/util.go @@ -22,8 +22,15 @@ import ( "io" "os" "path/filepath" + "runtime" + "time" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" + timeutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util/time" "github.com/buildpacks/lifecycle/cmd" + "golang.org/x/sync/semaphore" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" k8s "k8s.io/client-go/kubernetes" @@ -37,6 +44,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/manifest" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util/stringset" ) @@ -184,3 +192,83 @@ func GetManifestsFromHydratedManifests(ctx context.Context, hydratedManifests [] return manifests, nil } + +type tagErr struct { + tag string + err error +} + +// ImageTags generates tags for a list of artifacts +func ImageTags(ctx context.Context, runCtx *runcontext.RunContext, tagger tag.Tagger, out io.Writer, artifacts []*latest.Artifact) (tag.ImageTags, error) { + start := time.Now() + maxWorkers := runtime.GOMAXPROCS(0) + + if len(artifacts) > 0 { + output.Default.Fprintln(out, "Generating tags...") + } else { + output.Default.Fprintln(out, "No tags generated") + } + + tagErrs := make([]chan tagErr, len(artifacts)) + + // Use a weighted semaphore as a counting semaphore to limit the number of simultaneous taggers. + sem := semaphore.NewWeighted(int64(maxWorkers)) + for i := range artifacts { + tagErrs[i] = make(chan tagErr, 1) + + if err := sem.Acquire(ctx, 1); err != nil { + return nil, err + } + + i := i + go func() { + defer sem.Release(1) + _tag, err := tag.GenerateFullyQualifiedImageName(ctx, tagger, *artifacts[i]) + tagErrs[i] <- tagErr{tag: _tag, err: err} + }() + } + + imageTags := make(tag.ImageTags, len(artifacts)) + showWarning := false + + for i, artifact := range artifacts { + imageName := artifact.ImageName + out, ctx := output.WithEventContext(ctx, out, constants.Build, imageName) + output.Default.Fprintf(out, " - %s -> ", imageName) + + select { + case <-ctx.Done(): + return nil, context.Canceled + + case t := <-tagErrs[i]: + if t.err != nil { + log.Entry(ctx).Debug(t.err) + log.Entry(ctx).Debug("Using a fall-back tagger") + + fallbackTag, err := tag.GenerateFullyQualifiedImageName(ctx, &tag.ChecksumTagger{}, *artifact) + if err != nil { + return nil, fmt.Errorf("generating checksum as fall-back tag for %q: %w", imageName, err) + } + + t.tag = fallbackTag + showWarning = true + } + + _tag, err := ApplyDefaultRepo(runCtx.GlobalConfig(), runCtx.DefaultRepo(), t.tag) + + if err != nil { + return nil, err + } + + fmt.Fprintln(out, _tag) + imageTags[imageName] = _tag + } + } + + if showWarning { + output.Yellow.Fprintln(out, "Some taggers failed. Rerun with -vdebug for errors.") + } + + log.Entry(ctx).Infoln("Tags generated in", timeutil.Humanize(time.Since(start))) + return imageTags, nil +} diff --git a/pkg/skaffold/diagnose/diagnose.go b/pkg/skaffold/diagnose/diagnose.go index f8b1d58f5fa..dcab9463f1e 100644 --- a/pkg/skaffold/diagnose/diagnose.go +++ b/pkg/skaffold/diagnose/diagnose.go @@ -29,6 +29,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/sync" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" timeutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util/time" ) @@ -39,7 +40,7 @@ type Config interface { Artifacts() []*latest.Artifact } -func CheckArtifacts(ctx context.Context, cfg Config, out io.Writer) error { +func CheckArtifacts(ctx context.Context, cfg Config, tags tag.ImageTags, out io.Writer) error { for _, p := range cfg.GetPipelines() { for _, artifact := range p.Build.Artifacts { output.Default.Fprintf(out, "\n%s: %s\n", typeOfArtifact(artifact), artifact.ImageName) @@ -53,11 +54,16 @@ func CheckArtifacts(ctx context.Context, cfg Config, out io.Writer) error { fmt.Fprintf(out, " - Size of the context: %vbytes\n", size) } - timeDeps1, deps, err := timeToListDependencies(ctx, artifact, cfg) + tag, ok := tags[artifact.ImageName] + if !ok { + return fmt.Errorf("getting tag for image %s", artifact.ImageName) + } + + timeDeps1, deps, err := timeToListDependencies(ctx, artifact, tag, cfg) if err != nil { return fmt.Errorf("listing artifact dependencies: %w", err) } - timeDeps2, _, err := timeToListDependencies(ctx, artifact, cfg) + timeDeps2, _, err := timeToListDependencies(ctx, artifact, tag, cfg) if err != nil { return fmt.Errorf("listing artifact dependencies: %w", err) } @@ -119,11 +125,11 @@ func typeOfArtifact(a *latest.Artifact) string { } } -func timeToListDependencies(ctx context.Context, a *latest.Artifact, cfg Config) (string, []string, error) { +func timeToListDependencies(ctx context.Context, a *latest.Artifact, tag string, cfg Config) (string, []string, error) { start := time.Now() g := graph.ToArtifactGraph(cfg.Artifacts()) sourceDependencies := graph.NewSourceDependenciesCache(cfg, nil, g) - paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a, nil) + paths, err := sourceDependencies.SingleArtifactDependencies(ctx, a, tag) return timeutil.Humanize(time.Since(start)), paths, err } diff --git a/pkg/skaffold/diagnose/diagnose_test.go b/pkg/skaffold/diagnose/diagnose_test.go index e79ab7399d8..6580c7a948a 100644 --- a/pkg/skaffold/diagnose/diagnose_test.go +++ b/pkg/skaffold/diagnose/diagnose_test.go @@ -80,7 +80,7 @@ func TestSizeOfDockerContext(t *testing.T) { func TestCheckArtifacts(t *testing.T) { testutil.Run(t, "", func(t *testutil.T) { tmpDir := t.NewTempDir().Write("Dockerfile", "FROM busybox") - + tags := map[string]string{"base": "gcr.io/library/busybox:latest"} err := CheckArtifacts(context.Background(), &mockConfig{ artifacts: []*latest.Artifact{{ ImageName: "base", @@ -91,8 +91,7 @@ func TestCheckArtifacts(t *testing.T) { }, }, }}, - }, io.Discard) - + }, tags, io.Discard) t.CheckNoError(err) }) } diff --git a/pkg/skaffold/docker/dependencies_test.go b/pkg/skaffold/docker/dependencies_test.go index a63dafc0575..939adbdfc62 100644 --- a/pkg/skaffold/docker/dependencies_test.go +++ b/pkg/skaffold/docker/dependencies_test.go @@ -254,27 +254,6 @@ FROM library/ruby:2.3.0 ADD ./file /etc/file ` -type fakeImageFetcher struct{} - -func (f *fakeImageFetcher) fetch(_ context.Context, image string, _ Config) (*v1.ConfigFile, error) { - switch image { - case "ubuntu:14.04", "busybox", "nginx", "golang:1.9.2", "jboss/wildfly:14.0.1.Final", "gcr.io/distroless/base": - return &v1.ConfigFile{}, nil - case "golang:onbuild": - return &v1.ConfigFile{ - Config: v1.Config{ - OnBuild: []string{ - "COPY . /onbuild", - }, - }, - }, nil - case "library/ruby:2.3.0": - return nil, fmt.Errorf("retrieving image \"library/ruby:2.3.0\": unsupported MediaType: \"application/vnd.docker.distribution.manifest.v1+prettyjws\", see https://github.com/google/go-containerregistry/issues/377") - } - - return nil, fmt.Errorf("no image found for %s", image) -} - func TestGetDependencies(t *testing.T) { tests := []struct { description string diff --git a/pkg/skaffold/docker/dockertest.go b/pkg/skaffold/docker/dockertest.go index 54b8c14daa1..4a1fbeadd78 100644 --- a/pkg/skaffold/docker/dockertest.go +++ b/pkg/skaffold/docker/dockertest.go @@ -16,6 +16,14 @@ limitations under the License. package docker +import ( + "context" + "fmt" + + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" + v1 "github.com/google/go-containerregistry/pkg/v1" +) + // MockArtifactResolver mocks docker.ArtifactResolver interface. type mockArtifactResolver struct { m map[string]string @@ -44,3 +52,69 @@ func (s *simpleMockArtifactResolver) GetImageTag(_ string) (string, bool) { func NewSimpleMockArtifactResolver() ArtifactResolver { return &simpleMockArtifactResolver{} } + +// MockConfig is a mock implementation of the Config interface. +type MockConfig struct { + runMode config.RunMode + prune bool +} + +func (m MockConfig) GetKubeContext() string { + return "" +} + +func (m MockConfig) GlobalConfig() string { + return "" +} + +func (m MockConfig) MinikubeProfile() string { + return "" +} + +func (m MockConfig) GetInsecureRegistries() map[string]bool { + return map[string]bool{} +} + +func (m MockConfig) Mode() config.RunMode { + return m.runMode +} + +func (m MockConfig) Prune() bool { + return m.prune +} + +func (m MockConfig) ContainerDebugging() bool { + return false +} + +func NewMockConfig(mode config.RunMode, prune bool) Config { + return &MockConfig{runMode: mode, prune: prune} +} + +type fakeImageFetcher struct{} + +func (f *fakeImageFetcher) fetch(_ context.Context, image string, _ Config) (*v1.ConfigFile, error) { + switch image { + case "ubuntu:14.04", "busybox", "nginx", "golang:1.9.2", "jboss/wildfly:14.0.1.Final", "gcr.io/distroless/base", "gcr.io/distroless/base:latest": + return &v1.ConfigFile{}, nil + case "golang:onbuild": + return &v1.ConfigFile{ + Config: v1.Config{ + OnBuild: []string{ + "COPY . /onbuild", + }, + }, + }, nil + case "library/ruby:2.3.0": + return nil, fmt.Errorf("retrieving image \"library/ruby:2.3.0\": unsupported MediaType: \"application/vnd.docker.distribution.manifest.v1+prettyjws\", see https://github.com/google/go-containerregistry/issues/377") + } + + return nil, fmt.Errorf("no image found for %s", image) +} + +type FetchImage func(context.Context, string, Config) (*v1.ConfigFile, error) + +func NewFakeImageFetcher() FetchImage { + fetcher := fakeImageFetcher{} + return fetcher.fetch +} diff --git a/pkg/skaffold/graph/dependencies.go b/pkg/skaffold/graph/dependencies.go index 0c05a4096ea..8fb10a8bc11 100644 --- a/pkg/skaffold/graph/dependencies.go +++ b/pkg/skaffold/graph/dependencies.go @@ -43,7 +43,7 @@ type SourceDependenciesCache interface { TransitiveArtifactDependencies(ctx context.Context, a *latest.Artifact) ([]string, error) // SingleArtifactDependencies returns the source dependencies for only the target artifact. // The result (even if an error) is cached so that the function is evaluated only once for every artifact. The cache is reset before the start of the next devloop. - SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, envTags map[string]string) ([]string, error) + SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tag string) ([]string, error) // Reset removes the cached source dependencies for all artifacts Reset() } @@ -69,11 +69,8 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont if !ok { return nil, fmt.Errorf("unable to resolve tag for image: %s", a.ImageName) } - envTags, err := EnvTags(tag) - if err != nil { - return nil, fmt.Errorf("unable to parse build args from tag %s: err %w", tag, err) - } - deps, err := r.SingleArtifactDependencies(ctx, a, envTags) + + deps, err := r.SingleArtifactDependencies(ctx, a, tag) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err @@ -89,14 +86,14 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return deps, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, envTags map[string]string) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tag string) ([]string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "SingleArtifactDependencies", map[string]string{ "ArtifactName": instrumentation.PII(a.ImageName), }) defer endTrace() res, err := r.cache.Exec(a.ImageName, func() ([]string, error) { - return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver, envTags) + return getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver, tag) }) if err != nil { endTrace(instrumentation.TraceEndError(err)) @@ -126,7 +123,7 @@ func EnvTags(tag string) (map[string]string, error) { } // sourceDependenciesForArtifact returns the build dependencies for the current artifact. -func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver, envTags map[string]string) ([]string, error) { +func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver, tag string) ([]string, error) { var ( paths []string err error @@ -140,6 +137,11 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg // For `dev` it will succeed on the first dev loop and list any additional dependencies found from the base artifact's ONBUILD instructions as a file added instead of modified (see `filemon.Events`) deps := docker.ResolveDependencyImages(a.Dependencies, r, false) + envTags, err := EnvTags(tag) + if err != nil { + return nil, fmt.Errorf("unable to create build args: %w", err) + } + args, evalErr := docker.EvalBuildArgsWithEnv(cfg.Mode(), a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, deps, envTags) if evalErr != nil { return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr) diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 61583e7d3de..32ce6cfa73f 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -21,6 +21,7 @@ import ( "path/filepath" "testing" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" "github.com/google/go-cmp/cmp/cmpopts" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker" @@ -43,7 +44,7 @@ func TestSourceDependenciesCache(t *testing.T) { "img4": {"file41", "file42"}, } counts := map[string]int{"img1": 0, "img2": 0, "img3": 0, "img4": 0} - t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver, _ map[string]string) ([]string, error) { + t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver, _ string) ([]string, error) { counts[a.ImageName]++ return deps[a.ImageName], nil }) @@ -67,10 +68,13 @@ func TestSourceDependenciesForArtifact(t *testing.T) { "dir2/frob.go", ) tests := []struct { - description string - artifact *latest.Artifact - dockerConfig docker.Config - expectedPaths []string + description string + artifact *latest.Artifact + tag string + dockerConfig docker.Config + dockerBuildArgs map[string]string + dockerFileContents string + expectedPaths []string }{ { description: "ko default dependencies", @@ -85,11 +89,55 @@ func TestSourceDependenciesForArtifact(t *testing.T) { filepath.Join(tmpDir.Root(), "bar.go"), }, }, + { + description: "docker default dependencies", + artifact: &latest.Artifact{ + ImageName: "img1", + ArtifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{ + DockerfilePath: "Dockerfile", + }, + }, + Workspace: tmpDir.Root(), + Dependencies: []*latest.ArtifactDependency{{ImageName: "img2", Alias: "BASE"}}, + }, + dockerBuildArgs: map[string]string{ + "IMAGE_REPO": "{{.IMAGE_REPO}}", + "IMAGE_NAME": "{{.IMAGE_NAME}}", + "IMAGE_TAG": "{{.IMAGE_TAG}}", + }, + dockerConfig: docker.NewMockConfig(config.RunModes.Build, false), + dockerFileContents: `ARG IMAGE_REPO +ARG IMAGE_NAME +ARG IMAGE_TAG +FROM $IMAGE_REPO/$IMAGE_NAME:$IMAGE_TAG +COPY bar.go . +`, + expectedPaths: []string{ + filepath.Join(tmpDir.Root(), "Dockerfile"), + filepath.Join(tmpDir.Root(), "bar.go"), + }, + tag: "gcr.io/distroless/base:latest", + }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, nil, nil) + + t.Override(&docker.RetrieveImage, docker.NewFakeImageFetcher()) + + d := docker.NewSimpleMockArtifactResolver() + tmpDir.Write("Dockerfile", test.dockerFileContents) + if test.dockerBuildArgs != nil { + args := map[string]*string{} + for k, v := range test.dockerBuildArgs { + args[k] = &v + } + test.artifact.DockerArtifact.BuildArgs = args + } + + paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, d, test.tag) t.CheckNoError(err) + t.CheckDeepEqual(test.expectedPaths, paths, cmpopts.SortSlices(func(x, y string) bool { return x < y })) }) diff --git a/pkg/skaffold/runner/build.go b/pkg/skaffold/runner/build.go index c3acd4ea547..b4ed64a7c03 100644 --- a/pkg/skaffold/runner/build.go +++ b/pkg/skaffold/runner/build.go @@ -21,10 +21,6 @@ import ( "fmt" "io" "os" - "runtime" - "time" - - "golang.org/x/sync/semaphore" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/build/cache" @@ -33,12 +29,10 @@ import ( eventV2 "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/event/v2" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/graph" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/platform" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" - timeutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util/time" ) func NewBuilder(builder build.Builder, tagger tag.Tagger, platforms platform.Resolver, cache cache.Cache, runCtx *runcontext.RunContext) *Builder { @@ -82,7 +76,7 @@ func (r *Builder) Build(ctx context.Context, out io.Writer, artifacts []*latest. return nil, err } - tags, err := r.imageTags(ctx, out, artifacts) + tags, err := deployutil.ImageTags(ctx, r.runCtx, r.tagger, out, artifacts) if err != nil { eventV2.TaskFailed(constants.Build, err) return nil, err @@ -130,6 +124,11 @@ func (r *Builder) Build(ctx context.Context, out io.Writer, artifacts []*latest. return bRes, nil } +// ApplyDefaultRepo applies the default repo to a given image tag. +func (r *Builder) ApplyDefaultRepo(tag string) (string, error) { + return deployutil.ApplyDefaultRepo(r.runCtx.GlobalConfig(), r.runCtx.DefaultRepo(), tag) +} + // HasBuilt returns true if this runner has built something. func (r *Builder) HasBuilt() bool { return r.hasBuilt @@ -148,90 +147,6 @@ func artifactsWithTags(tags tag.ImageTags, artifacts []*latest.Artifact) []graph return bRes } -type tagErr struct { - tag string - err error -} - -// ApplyDefaultRepo applies the default repo to a given image tag. -func (r *Builder) ApplyDefaultRepo(tag string) (string, error) { - return deployutil.ApplyDefaultRepo(r.runCtx.GlobalConfig(), r.runCtx.DefaultRepo(), tag) -} - -// imageTags generates tags for a list of artifacts -func (r *Builder) imageTags(ctx context.Context, out io.Writer, artifacts []*latest.Artifact) (tag.ImageTags, error) { - start := time.Now() - maxWorkers := runtime.GOMAXPROCS(0) - - if len(artifacts) > 0 { - output.Default.Fprintln(out, "Generating tags...") - } else { - output.Default.Fprintln(out, "No tags generated") - } - - tagErrs := make([]chan tagErr, len(artifacts)) - - // Use a weighted semaphore as a counting semaphore to limit the number of simultaneous taggers. - sem := semaphore.NewWeighted(int64(maxWorkers)) - for i := range artifacts { - tagErrs[i] = make(chan tagErr, 1) - - if err := sem.Acquire(ctx, 1); err != nil { - return nil, err - } - - i := i - go func() { - defer sem.Release(1) - _tag, err := tag.GenerateFullyQualifiedImageName(ctx, r.tagger, *artifacts[i]) - tagErrs[i] <- tagErr{tag: _tag, err: err} - }() - } - - imageTags := make(tag.ImageTags, len(artifacts)) - showWarning := false - - for i, artifact := range artifacts { - imageName := artifact.ImageName - out, ctx := output.WithEventContext(ctx, out, constants.Build, imageName) - output.Default.Fprintf(out, " - %s -> ", imageName) - - select { - case <-ctx.Done(): - return nil, context.Canceled - - case t := <-tagErrs[i]: - if t.err != nil { - log.Entry(ctx).Debug(t.err) - log.Entry(ctx).Debug("Using a fall-back tagger") - - fallbackTag, err := tag.GenerateFullyQualifiedImageName(ctx, &tag.ChecksumTagger{}, *artifact) - if err != nil { - return nil, fmt.Errorf("generating checksum as fall-back tag for %q: %w", imageName, err) - } - - t.tag = fallbackTag - showWarning = true - } - - _tag, err := r.ApplyDefaultRepo(t.tag) - if err != nil { - return nil, err - } - - fmt.Fprintln(out, _tag) - imageTags[imageName] = _tag - } - } - - if showWarning { - output.Yellow.Fprintln(out, "Some taggers failed. Rerun with -vdebug for errors.") - } - - log.Entry(ctx).Infoln("Tags generated in", timeutil.Humanize(time.Since(start))) - return imageTags, nil -} - func CheckWorkspaces(artifacts []*latest.Artifact) error { for _, a := range artifacts { if a.Workspace != "" { diff --git a/pkg/skaffold/runner/listen_test.go b/pkg/skaffold/runner/listen_test.go index f8d6af8a302..f2fde4f733d 100644 --- a/pkg/skaffold/runner/listen_test.go +++ b/pkg/skaffold/runner/listen_test.go @@ -59,7 +59,7 @@ func (f *fakeDepsResolver) TransitiveArtifactDependencies(context.Context, *late return nil, nil } -func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact, map[string]string) ([]string, error) { +func (f *fakeDepsResolver) SingleArtifactDependencies(context.Context, *latest.Artifact, string) ([]string, error) { return nil, nil } diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index 15b1b5bff40..f73dd9d8aab 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -117,11 +117,11 @@ func NewForConfig(ctx context.Context, runCtx *runcontext.RunContext) (*Skaffold return nil, fmt.Errorf("creating actiosn runner: %w", err) } - depLister := func(ctx context.Context, artifact *latest.Artifact) ([]string, error) { + depLister := func(ctx context.Context, artifact *latest.Artifact, tag string) ([]string, error) { ctx, endTrace := instrumentation.StartTrace(ctx, "NewForConfig_depLister") defer endTrace() - buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, nil) + buildDependencies, err := sourceDependencies.SingleArtifactDependencies(ctx, artifact, tag) if err != nil { endTrace(instrumentation.TraceEndError(err)) return nil, err diff --git a/pkg/skaffold/tag/custom_template_test.go b/pkg/skaffold/tag/custom_template_test.go index 80553912a8e..aa84c4a7d31 100644 --- a/pkg/skaffold/tag/custom_template_test.go +++ b/pkg/skaffold/tag/custom_template_test.go @@ -35,7 +35,7 @@ func (r *dependencyResolverImpl) TransitiveArtifactDependencies(ctx context.Cont return []string{}, nil } -func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, envTags map[string]string) ([]string, error) { +func (r *dependencyResolverImpl) SingleArtifactDependencies(ctx context.Context, a *latest.Artifact, tag string) ([]string, error) { return []string{}, nil } From d3556f72b02ebbad0b55c0eda1abb36a5ae36693 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 13:51:21 +0000 Subject: [PATCH 33/44] remove xtra file --- = | 30 ------------------------------ 1 file changed, 30 deletions(-) delete mode 100644 = diff --git a/= b/= deleted file mode 100644 index 6077aff7d3a..00000000000 --- a/= +++ /dev/null @@ -1,30 +0,0 @@ -ARGC: 1 -ARGIND: 0 -ARGV: array, 1 elements -BINMODE: 0 -CONVFMT: "%.6g" -ENVIRON: array, 73 elements -ERRNO: "" -FIELDWIDTHS: "" -FILENAME: "-" -FNR: 22 -FPAT: "[^[:space:]]+" -FS: " " -FUNCTAB: array, 42 elements -IGNORECASE: 0 -LINT: 0 -NF: 2 -NR: 22 -OFMT: "%.6g" -OFS: " " -ORS: "\n" -PREC: 53 -PROCINFO: array, 140 elements -RLENGTH: 0 -ROUNDMODE: "N" -RS: "\n" -RSTART: 0 -RT: "\n" -SUBSEP: "\034" -SYMTAB: array, 28 elements -TEXTDOMAIN: "messages" From b042e3009b4217571a6b899422b5071d3cf2a6e7 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 14:11:05 +0000 Subject: [PATCH 34/44] fix lint --- pkg/skaffold/deploy/util/util.go | 8 ++++---- pkg/skaffold/docker/dockertest.go | 3 ++- pkg/skaffold/graph/dependencies_test.go | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/skaffold/deploy/util/util.go b/pkg/skaffold/deploy/util/util.go index 071fa107d0d..12a89db14da 100644 --- a/pkg/skaffold/deploy/util/util.go +++ b/pkg/skaffold/deploy/util/util.go @@ -25,10 +25,6 @@ import ( "runtime" "time" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" - timeutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util/time" "github.com/buildpacks/lifecycle/cmd" "golang.org/x/sync/semaphore" "k8s.io/apimachinery/pkg/runtime/schema" @@ -43,9 +39,13 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/initializer/prompt" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/manifest" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util/stringset" + timeutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util/time" ) var ( diff --git a/pkg/skaffold/docker/dockertest.go b/pkg/skaffold/docker/dockertest.go index 4a1fbeadd78..32193c66d6e 100644 --- a/pkg/skaffold/docker/dockertest.go +++ b/pkg/skaffold/docker/dockertest.go @@ -20,8 +20,9 @@ import ( "context" "fmt" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" v1 "github.com/google/go-containerregistry/pkg/v1" + + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" ) // MockArtifactResolver mocks docker.ArtifactResolver interface. diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 32ce6cfa73f..7b943c3fc22 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -21,9 +21,9 @@ import ( "path/filepath" "testing" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/config" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/v2/testutil" From c052c83dc87eca75677588c4a0510fdab46b71e8 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 14:19:48 +0000 Subject: [PATCH 35/44] test that the build arg used as part of a filename is also replaced --- pkg/skaffold/graph/dependencies_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 7b943c3fc22..603876c764a 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -64,6 +64,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { tmpDir := testutil.NewTempDir(t).Touch( "foo.java", "bar.go", + "latest.go", "dir1/baz.java", "dir2/frob.go", ) @@ -87,6 +88,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { expectedPaths: []string{ filepath.Join(tmpDir.Root(), "dir2/frob.go"), filepath.Join(tmpDir.Root(), "bar.go"), + filepath.Join(tmpDir.Root(), "latest.go"), }, }, { @@ -112,10 +114,12 @@ ARG IMAGE_NAME ARG IMAGE_TAG FROM $IMAGE_REPO/$IMAGE_NAME:$IMAGE_TAG COPY bar.go . +COPY $IMAGE_TAG.go . `, expectedPaths: []string{ filepath.Join(tmpDir.Root(), "Dockerfile"), filepath.Join(tmpDir.Root(), "bar.go"), + filepath.Join(tmpDir.Root(), "latest.go"), }, tag: "gcr.io/distroless/base:latest", }, From 004c9432fabd1e93932c66097edc3b44abdc99e1 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 14:38:06 +0000 Subject: [PATCH 36/44] use stub instead of mock for test objects --- pkg/skaffold/docker/build_args_test.go | 4 +-- pkg/skaffold/docker/dockertest.go | 42 ++++++++++++------------- pkg/skaffold/graph/dependencies_test.go | 6 ++-- pkg/skaffold/tag/input_digest.go | 2 +- 4 files changed, 27 insertions(+), 27 deletions(-) diff --git a/pkg/skaffold/docker/build_args_test.go b/pkg/skaffold/docker/build_args_test.go index bad76137011..a6b839b30d0 100644 --- a/pkg/skaffold/docker/build_args_test.go +++ b/pkg/skaffold/docker/build_args_test.go @@ -226,13 +226,13 @@ func TestCreateBuildArgsFromArtifacts(t *testing.T) { }{ { description: "can resolve artifacts", - r: NewMockArtifactResolver(map[string]string{"img1": "tag1", "img2": "tag2", "img3": "tag3", "img4": "tag4"}), + r: NewStubArtifactResolver(map[string]string{"img1": "tag1", "img2": "tag2", "img3": "tag3", "img4": "tag4"}), deps: []*latest.ArtifactDependency{{ImageName: "img3", Alias: "alias3"}, {ImageName: "img4", Alias: "alias4"}}, args: map[string]*string{"alias3": util.Ptr("tag3"), "alias4": util.Ptr("tag4")}, }, { description: "cannot resolve artifacts", - r: NewMockArtifactResolver(map[string]string{}), + r: NewStubArtifactResolver(map[string]string{}), args: map[string]*string{"alias3": nil, "alias4": nil}, deps: []*latest.ArtifactDependency{{ImageName: "img3", Alias: "alias3"}, {ImageName: "img4", Alias: "alias4"}}, }, diff --git a/pkg/skaffold/docker/dockertest.go b/pkg/skaffold/docker/dockertest.go index 32193c66d6e..833c51b713b 100644 --- a/pkg/skaffold/docker/dockertest.go +++ b/pkg/skaffold/docker/dockertest.go @@ -26,70 +26,70 @@ import ( ) // MockArtifactResolver mocks docker.ArtifactResolver interface. -type mockArtifactResolver struct { +type stubArtifactResolver struct { m map[string]string } -// NewMockArtifactResolver returns a mock ArtifactResolver for testing. -func NewMockArtifactResolver(m map[string]string) *mockArtifactResolver { - return &mockArtifactResolver{m} +// NewStubArtifactResolver returns a mock ArtifactResolver for testing. +func NewStubArtifactResolver(m map[string]string) *stubArtifactResolver { + return &stubArtifactResolver{m} } -func (r mockArtifactResolver) GetImageTag(imageName string) (string, bool) { +func (r stubArtifactResolver) GetImageTag(imageName string) (string, bool) { val, found := r.m[imageName] return val, found } -// simpleMockArtifactResolver is an implementation of docker.ArtifactResolver +// simpleStubArtifactResolver is an implementation of docker.ArtifactResolver // that returns the same value for any key -type simpleMockArtifactResolver struct{} +type simpleStubArtifactResolver struct{} // GetImageTag is an implementation of docker.ArtifactResolver that // always returns the same tag. -func (s *simpleMockArtifactResolver) GetImageTag(_ string) (string, bool) { +func (s *simpleStubArtifactResolver) GetImageTag(_ string) (string, bool) { return "image:latest", true } -func NewSimpleMockArtifactResolver() ArtifactResolver { - return &simpleMockArtifactResolver{} +func NewSimpleStubArtifactResolver() ArtifactResolver { + return &simpleStubArtifactResolver{} } -// MockConfig is a mock implementation of the Config interface. -type MockConfig struct { +// configStub is a mock implementation of the Config interface. +type configStub struct { runMode config.RunMode prune bool } -func (m MockConfig) GetKubeContext() string { +func (m configStub) GetKubeContext() string { return "" } -func (m MockConfig) GlobalConfig() string { +func (m configStub) GlobalConfig() string { return "" } -func (m MockConfig) MinikubeProfile() string { +func (m configStub) MinikubeProfile() string { return "" } -func (m MockConfig) GetInsecureRegistries() map[string]bool { +func (m configStub) GetInsecureRegistries() map[string]bool { return map[string]bool{} } -func (m MockConfig) Mode() config.RunMode { +func (m configStub) Mode() config.RunMode { return m.runMode } -func (m MockConfig) Prune() bool { +func (m configStub) Prune() bool { return m.prune } -func (m MockConfig) ContainerDebugging() bool { +func (m configStub) ContainerDebugging() bool { return false } -func NewMockConfig(mode config.RunMode, prune bool) Config { - return &MockConfig{runMode: mode, prune: prune} +func NewConfigStub(mode config.RunMode, prune bool) Config { + return &configStub{runMode: mode, prune: prune} } type fakeImageFetcher struct{} diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 603876c764a..4e7d5c98a8f 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -49,7 +49,7 @@ func TestSourceDependenciesCache(t *testing.T) { return deps[a.ImageName], nil }) - r := NewSourceDependenciesCache(nil, docker.NewSimpleMockArtifactResolver(), g) + r := NewSourceDependenciesCache(nil, docker.NewSimpleStubArtifactResolver(), g) d, err := r.TransitiveArtifactDependencies(context.Background(), g["img1"]) t.CheckNoError(err) expectedDeps := []string{"file11", "file12", "file21", "file22", "file31", "file32", "file41", "file42", "file41", "file42"} @@ -108,7 +108,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { "IMAGE_NAME": "{{.IMAGE_NAME}}", "IMAGE_TAG": "{{.IMAGE_TAG}}", }, - dockerConfig: docker.NewMockConfig(config.RunModes.Build, false), + dockerConfig: docker.NewConfigStub(config.RunModes.Build, false), dockerFileContents: `ARG IMAGE_REPO ARG IMAGE_NAME ARG IMAGE_TAG @@ -129,7 +129,7 @@ COPY $IMAGE_TAG.go . t.Override(&docker.RetrieveImage, docker.NewFakeImageFetcher()) - d := docker.NewSimpleMockArtifactResolver() + d := docker.NewSimpleStubArtifactResolver() tmpDir.Write("Dockerfile", test.dockerFileContents) if test.dockerBuildArgs != nil { args := map[string]*string{} diff --git a/pkg/skaffold/tag/input_digest.go b/pkg/skaffold/tag/input_digest.go index 4273640dd4d..8223b761c9e 100644 --- a/pkg/skaffold/tag/input_digest.go +++ b/pkg/skaffold/tag/input_digest.go @@ -40,7 +40,7 @@ type inputDigestTagger struct { } func NewInputDigestTagger(cfg docker.Config, ag graph.ArtifactGraph) (Tagger, error) { - return NewInputDigestTaggerWithSourceCache(cfg, graph.NewSourceDependenciesCache(cfg, docker.NewSimpleMockArtifactResolver(), ag)) + return NewInputDigestTaggerWithSourceCache(cfg, graph.NewSourceDependenciesCache(cfg, docker.NewSimpleStubArtifactResolver(), ag)) } func NewInputDigestTaggerWithSourceCache(cfg docker.Config, cache graph.SourceDependenciesCache) (Tagger, error) { From 8c4e2e4166303262a782e40ac35d146733cfe9ea Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 14:53:31 +0000 Subject: [PATCH 37/44] use empty string in place of tag because hash_test unit tests mock SingleArtifactDependencies so the value doesn't matter --- pkg/skaffold/build/cache/hash_test.go | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index a90dceba5d8..04445ed406c 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -50,8 +50,6 @@ var fakeArtifactConfig = func(a *latest.Artifact) (string, error) { return "", nil } -var testTag = "gcr.io/my-repo/my-image:latest" - const Dockerfile = "Dockerfile" func TestGetHashForArtifact(t *testing.T) { @@ -164,7 +162,7 @@ func TestGetHashForArtifact(t *testing.T) { } depLister := stubDependencyLister(test.dependencies) - actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms, testTag) + actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms, "") t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -248,7 +246,7 @@ func TestGetHashForArtifactWithDependencies(t *testing.T) { return test.fileDeps[a.ImageName], nil } - actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}, testTag) + actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}, "") t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -311,21 +309,21 @@ func TestBuildArgs(t *testing.T) { } t.Override(&fileHasherFunc, mockCacheHasher) t.Override(&artifactConfigFunc, fakeArtifactConfig) - actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) + actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change order of buildargs artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"two": util.Ptr("2"), "one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change build args, get different hash artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") t.CheckNoError(err) if actual == test.expected { @@ -358,7 +356,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { t.Override(&artifactConfigFunc, fakeArtifactConfig) depLister := stubDependencyLister([]string{"graph"}) - hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) + hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") t.CheckNoError(err) @@ -368,7 +366,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { return []string{"FOO=baz"} } - hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) + hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") t.CheckNoError(err) if hash1 == hash2 { @@ -425,7 +423,7 @@ func TestCacheHasher(t *testing.T) { path := originalFile depLister := stubDependencyLister([]string{tmpDir.Path(originalFile)}) - oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, testTag) + oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, "") t.CheckNoError(err) test.update(originalFile, tmpDir) @@ -434,7 +432,7 @@ func TestCacheHasher(t *testing.T) { } depLister = stubDependencyLister([]string{tmpDir.Path(path)}) - newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, testTag) + newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, "") t.CheckNoError(err) t.CheckFalse(test.differentHash && oldHash == newHash) From 4a345a20a36ff069d8ba7dd07e231215cb821f48 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 16:18:00 +0000 Subject: [PATCH 38/44] add integration test to ensure that the build int BUILD args can be used as part of a file --- integration/build_test.go | 34 +++++++++++++------ .../base/Dockerfile | 0 .../base/go.mod | 0 .../base/main.go | 0 .../child/Dockerfile | 0 .../skaffold.yaml | 0 .../single-artifact/Dockerfile | 5 +++ .../single-artifact/script-latest.sh | 1 + .../single-artifact/skaffold.yaml | 18 ++++++++++ 9 files changed, 47 insertions(+), 11 deletions(-) rename integration/testdata/docker-run-with-build-args/{ => artifact-with-dependency}/base/Dockerfile (100%) rename integration/testdata/docker-run-with-build-args/{ => artifact-with-dependency}/base/go.mod (100%) rename integration/testdata/docker-run-with-build-args/{ => artifact-with-dependency}/base/main.go (100%) rename integration/testdata/docker-run-with-build-args/{ => artifact-with-dependency}/child/Dockerfile (100%) rename integration/testdata/docker-run-with-build-args/{ => artifact-with-dependency}/skaffold.yaml (100%) create mode 100644 integration/testdata/docker-run-with-build-args/single-artifact/Dockerfile create mode 100755 integration/testdata/docker-run-with-build-args/single-artifact/script-latest.sh create mode 100644 integration/testdata/docker-run-with-build-args/single-artifact/skaffold.yaml diff --git a/integration/build_test.go b/integration/build_test.go index 23276c038c5..461454fec80 100644 --- a/integration/build_test.go +++ b/integration/build_test.go @@ -335,34 +335,46 @@ func failNowIfError(t Fataler, err error) { func TestRunWithDockerAndBuildArgs(t *testing.T) { tests := []struct { - description string - projectDir string - args []string + description string + projectDir string + skaffoldArgs []string + dockerRunArgs []string + wantOutput string }{ { - description: "IMAGE_REPO, IMAGE_TAG, and IMAGE_NAME are passed to the docker build args", - projectDir: "testdata/docker-run-with-build-args", - args: []string{"--kube-context", "default"}, + description: "IMAGE_REPO, IMAGE_TAG, and IMAGE_NAME are passed to Docker build as build args", + projectDir: "testdata/docker-run-with-build-args/artifact-with-dependency", + skaffoldArgs: []string{"--kube-context", "default"}, + dockerRunArgs: []string{"run", "child:latest"}, + wantOutput: "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest", + }, + { + description: "IMAGE_TAG can be used as a part of a filename in the Dockerfile", + projectDir: "testdata/docker-run-with-build-args/single-artifact", + skaffoldArgs: []string{"--kube-context", "default"}, + dockerRunArgs: []string{"run", "example:latest"}, + wantOutput: "HELLO WORLD", }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - skaffold.Build(test.args...).InDir(test.projectDir).Run(t.T) defer skaffold.Delete().InDir(test.projectDir).Run(t.T) + if err := skaffold.Build(test.skaffoldArgs...).InDir(test.projectDir).Run(t.T); err != nil { + t.Errorf("skaffold build args: %v working directory:%s returned unexpected error: %v", test.skaffoldArgs, test.projectDir, err) + } - expected := "IMAGE_REPO: gcr.io/k8s-skaffold, IMAGE_NAME: skaffold, IMAGE_TAG:latest" got := "" err := wait.PollImmediate(time.Millisecond*500, 1*time.Minute, func() (bool, error) { - out, _ := exec.Command("docker", "run", "child:latest").Output() + out, _ := exec.Command("docker", test.dockerRunArgs...).Output() t.Logf("Output:[%s]\n", out) got = strings.Trim(string(out), " \n") - return got == expected, nil + return got == test.wantOutput, nil }) if err != nil { - t.Errorf("docker run produced incorrect output, got:[%s], want:[%s], err: %v", got, expected, err) + t.Errorf("docker run produced incorrect output, got:[%s], want:[%s], err: %v", got, test.wantOutput, err) } failNowIfError(t, err) }) diff --git a/integration/testdata/docker-run-with-build-args/base/Dockerfile b/integration/testdata/docker-run-with-build-args/artifact-with-dependency/base/Dockerfile similarity index 100% rename from integration/testdata/docker-run-with-build-args/base/Dockerfile rename to integration/testdata/docker-run-with-build-args/artifact-with-dependency/base/Dockerfile diff --git a/integration/testdata/docker-run-with-build-args/base/go.mod b/integration/testdata/docker-run-with-build-args/artifact-with-dependency/base/go.mod similarity index 100% rename from integration/testdata/docker-run-with-build-args/base/go.mod rename to integration/testdata/docker-run-with-build-args/artifact-with-dependency/base/go.mod diff --git a/integration/testdata/docker-run-with-build-args/base/main.go b/integration/testdata/docker-run-with-build-args/artifact-with-dependency/base/main.go similarity index 100% rename from integration/testdata/docker-run-with-build-args/base/main.go rename to integration/testdata/docker-run-with-build-args/artifact-with-dependency/base/main.go diff --git a/integration/testdata/docker-run-with-build-args/child/Dockerfile b/integration/testdata/docker-run-with-build-args/artifact-with-dependency/child/Dockerfile similarity index 100% rename from integration/testdata/docker-run-with-build-args/child/Dockerfile rename to integration/testdata/docker-run-with-build-args/artifact-with-dependency/child/Dockerfile diff --git a/integration/testdata/docker-run-with-build-args/skaffold.yaml b/integration/testdata/docker-run-with-build-args/artifact-with-dependency/skaffold.yaml similarity index 100% rename from integration/testdata/docker-run-with-build-args/skaffold.yaml rename to integration/testdata/docker-run-with-build-args/artifact-with-dependency/skaffold.yaml diff --git a/integration/testdata/docker-run-with-build-args/single-artifact/Dockerfile b/integration/testdata/docker-run-with-build-args/single-artifact/Dockerfile new file mode 100644 index 00000000000..7daf2952870 --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/single-artifact/Dockerfile @@ -0,0 +1,5 @@ +FROM busybox +ARG IMAGE_TAG +RUN echo $IMAGE_TAG +COPY "script-${IMAGE_TAG}.sh" script.sh +CMD ["/bin/sh","script.sh"] diff --git a/integration/testdata/docker-run-with-build-args/single-artifact/script-latest.sh b/integration/testdata/docker-run-with-build-args/single-artifact/script-latest.sh new file mode 100755 index 00000000000..dfc0eb82378 --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/single-artifact/script-latest.sh @@ -0,0 +1 @@ +echo "HELLO WORLD" \ No newline at end of file diff --git a/integration/testdata/docker-run-with-build-args/single-artifact/skaffold.yaml b/integration/testdata/docker-run-with-build-args/single-artifact/skaffold.yaml new file mode 100644 index 00000000000..20ee43ce74a --- /dev/null +++ b/integration/testdata/docker-run-with-build-args/single-artifact/skaffold.yaml @@ -0,0 +1,18 @@ +apiVersion: skaffold/v4beta12 +kind: Config +build: + tagPolicy: + sha256: {} + local: + push: false + useDockerCLI: true + artifacts: + - image: example + docker: + dockerfile: Dockerfile + noCache: true + buildArgs: + IMAGE_TAG: '{{.IMAGE_TAG}}' +deploy: + docker: + images: [example] From 6ff04ca75f8cc577c6a1b9449583ae9bc1b9c874 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 16:32:50 +0000 Subject: [PATCH 39/44] fix imports, remove github action workflow used for debugging --- .github/workflows/test-linux.yml | 138 ------------------------------- cmd/skaffold/app/cmd/diagnose.go | 4 +- 2 files changed, 2 insertions(+), 140 deletions(-) delete mode 100644 .github/workflows/test-linux.yml diff --git a/.github/workflows/test-linux.yml b/.github/workflows/test-linux.yml deleted file mode 100644 index a3495c63e73..00000000000 --- a/.github/workflows/test-linux.yml +++ /dev/null @@ -1,138 +0,0 @@ -name: Test Docker run - -# Triggers the workflow on push or pull request events -on: [push, pull_request] - -permissions: read-all - -concurrency: - group: build-${{ github.event.pull_request.number || github.ref }}-${{github.workflow}} - cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} - -jobs: - - build: - name: PR integration tests (linux) - runs-on: ubuntu-latest - strategy: - matrix: - kustomize_version: [5.5.0] - ko_version: [0.17.1] - kompose_version: [1.34.0] - kpt_version: [1.0.0-beta.55] - minikube_version: [1.34.0] - gcloud_sdk_version: [502.0.0] - container_structure_tests_version: [1.19.3] - java: [21] - steps: - - - name: Check out code into the Go module directory - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - - name: Set up Go - uses: actions/setup-go@v5 - with: - go-version: 1.23.* - id: go - - - name: Set up Java - uses: actions/setup-java@v4 - with: - distribution: 'temurin' - java-version: ${{ matrix.java }} - - # Retrieve build locations with `go env` - # - - id: go-cache-paths - run: | - echo "go-build=$(go env GOCACHE)" >> $GITHUB_OUTPUT - echo "go-mod=$(go env GOMODCACHE)" >> $GITHUB_OUTPUT - - - uses: actions/cache@v4 - with: - path: | - ${{ steps.go-cache-paths.outputs.go-build }} - ${{ steps.go-cache-paths.outputs.go-mod }} - key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} - restore-keys: | - ${{ runner.os }}-go- - - # Skip integration tests for `docs`-only changes (only works for PR-based dev workflows like Skaffold's). - # NOTE: grep output is stored in env var with `|| true` as the run command cannot fail or action will fail - - name: Check if only docs changes were made in this PR - run: | - echo ${{ github.event.before }} - echo ${{ github.event.after }} - NON_DOCS_FILES_CHANGED=$(git diff --name-only ${{ github.event.pull_request.base.sha }}..${{ github.sha }}| grep -v '^docs/' || true) - echo "NON_DOCS_FILES_CHANGED=${#NON_DOCS_FILES_CHANGED}" >> $GITHUB_ENV # get the char len of diff output (used later) - - - name: Install Kustomize - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - wget -O kustomize.tar.gz https://github.com/kubernetes-sigs/kustomize/releases/download/kustomize/v${{ matrix.kustomize_version }}/kustomize_v${{ matrix.kustomize_version }}_linux_amd64.tar.gz - sudo tar -xvf kustomize.tar.gz -C /usr/local/bin/ - - - name: Install Ko - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - wget -O ko.tar.gz https://github.com/google/ko/releases/download/v${{ matrix.ko_version }}/ko_${{ matrix.ko_version }}_Linux_x86_64.tar.gz - sudo tar -xvf ko.tar.gz -C /usr/local/bin/ - - - name: Install Kompose - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - wget -O kompose https://github.com/kubernetes/kompose/releases/download/v${{ matrix.kompose_version }}/kompose-linux-amd64 && chmod +x kompose - sudo mv kompose /usr/local/bin/ - - - name: Install Kpt - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - wget -O kpt https://github.com/GoogleContainerTools/kpt/releases/download/v${{ matrix.kpt_version }}/kpt_linux_amd64 && chmod +x kpt - sudo mv kpt /usr/local/bin/ - - - name: Install GCloud - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - wget -O gcloud.tar.gz https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud-sdk-${{ matrix.gcloud_sdk_version }}-linux-x86_64.tar.gz - tar -xvf gcloud.tar.gz -C ${HOME}/ - ${HOME}/google-cloud-sdk/install.sh --usage-reporting=false --bash-completion=false --disable-installation-options - echo "${HOME}/google-cloud-sdk/bin" >> $GITHUB_PATH - - - name: Configure GCloud with Docker - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: gcloud auth configure-docker - - - name: Install Container Structure Test - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - wget -O container-structure-test https://github.com/GoogleContainerTools/container-structure-test/releases/download/v${{ matrix.container_structure_tests_version }}/container-structure-test-linux-amd64 && chmod +x container-structure-test - sudo mv container-structure-test /usr/local/bin/ - - - name: Setup other files and permissions - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - sudo chown $(whoami):docker ${HOME}/.docker -R - sudo chmod g+rw ${HOME}/.docker -R - echo '{}' > ${HOME}/.docker/config.json - mkdir -p ${HOME}/.m2/ && cp ./hack/maven/settings.xml ${HOME}/.m2/settings.xml - - - name: Install Minikube and start cluster - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - curl -Lo minikube https://github.com/kubernetes/minikube/releases/download/v${{ matrix.minikube_version }}/minikube-linux-amd64 - sudo install minikube /usr/local/bin/minikube - minikube start --profile=minikube --driver=docker - - - name: Make and install Skaffold binary from current PR - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} # non docs files were changed, skaffold build needed - run: | - make - sudo install "${HOME}/work/skaffold/skaffold/out/skaffold" /usr/local/bin/skaffold - - - name: Run integration tests - if: ${{ env.NON_DOCS_FILES_CHANGED != 0 }} - run: | - skaffold config set --global collect-metrics false - INTEGRATION_TEST_ARGS="-run=TestRunWithDockerAndBuildArgs/" make integration diff --git a/cmd/skaffold/app/cmd/diagnose.go b/cmd/skaffold/app/cmd/diagnose.go index abc9baf890a..c99dc23c536 100644 --- a/cmd/skaffold/app/cmd/diagnose.go +++ b/cmd/skaffold/app/cmd/diagnose.go @@ -22,16 +22,16 @@ import ( "io" "os" - deployutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/util" - "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" "github.com/spf13/cobra" + deployutil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/util" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/diagnose" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/parser" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/runner/runcontext" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" schemaUtil "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/util" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tag" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/tags" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/version" From c4f74ffb6e579b108e41a56c90707860b5f9668c Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 17:34:17 +0000 Subject: [PATCH 40/44] fix lint --- pkg/skaffold/graph/dependencies.go | 4 ++-- pkg/skaffold/graph/dependencies_test.go | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/skaffold/graph/dependencies.go b/pkg/skaffold/graph/dependencies.go index 8fb10a8bc11..eb84edf2aad 100644 --- a/pkg/skaffold/graph/dependencies.go +++ b/pkg/skaffold/graph/dependencies.go @@ -137,8 +137,8 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg // For `dev` it will succeed on the first dev loop and list any additional dependencies found from the base artifact's ONBUILD instructions as a file added instead of modified (see `filemon.Events`) deps := docker.ResolveDependencyImages(a.Dependencies, r, false) - envTags, err := EnvTags(tag) - if err != nil { + envTags, evalErr := EnvTags(tag) + if evalErr != nil { return nil, fmt.Errorf("unable to create build args: %w", err) } diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index 4e7d5c98a8f..cbbea68fde6 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -126,9 +126,7 @@ COPY $IMAGE_TAG.go . } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - t.Override(&docker.RetrieveImage, docker.NewFakeImageFetcher()) - d := docker.NewSimpleStubArtifactResolver() tmpDir.Write("Dockerfile", test.dockerFileContents) if test.dockerBuildArgs != nil { @@ -138,10 +136,8 @@ COPY $IMAGE_TAG.go . } test.artifact.DockerArtifact.BuildArgs = args } - paths, err := sourceDependenciesForArtifact(context.Background(), test.artifact, test.dockerConfig, d, test.tag) t.CheckNoError(err) - t.CheckDeepEqual(test.expectedPaths, paths, cmpopts.SortSlices(func(x, y string) bool { return x < y })) }) From 0ddf270a6d298f4e7e547cbe5d93fb7c56f82edd Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 21:42:54 +0000 Subject: [PATCH 41/44] use one function everywhere in the codebase to generate the image info tags --- pkg/skaffold/build/cluster/kaniko.go | 13 +++++-------- pkg/skaffold/build/docker/docker.go | 7 +------ pkg/skaffold/docker/build_args.go | 16 ++++++++++++++++ pkg/skaffold/docker/image.go | 7 +------ pkg/skaffold/graph/dependencies.go | 13 ------------- 5 files changed, 23 insertions(+), 33 deletions(-) diff --git a/pkg/skaffold/build/cluster/kaniko.go b/pkg/skaffold/build/cluster/kaniko.go index 80f3690907e..703befbd979 100644 --- a/pkg/skaffold/build/cluster/kaniko.go +++ b/pkg/skaffold/build/cluster/kaniko.go @@ -269,17 +269,14 @@ func envMapFromVars(env []v1.EnvVar) map[string]string { } func generateEnvFromImage(imageStr string) ([]v1.EnvVar, error) { - imgRef, err := docker.ParseReference(imageStr) + envMap, err := docker.EnvTags(imageStr) if err != nil { - return nil, err - } - if imgRef.Tag == "" { - imgRef.Tag = "latest" + return nil, fmt.Errorf("unable to get image tags: %w", err) } var generatedEnvs []v1.EnvVar - generatedEnvs = append(generatedEnvs, v1.EnvVar{Name: "IMAGE_REPO", Value: imgRef.Repo}) - generatedEnvs = append(generatedEnvs, v1.EnvVar{Name: "IMAGE_NAME", Value: imgRef.Name}) - generatedEnvs = append(generatedEnvs, v1.EnvVar{Name: "IMAGE_TAG", Value: imgRef.Tag}) + for k, v := range envMap { + generatedEnvs = append(generatedEnvs, v1.EnvVar{Name: k, Value: v}) + } return generatedEnvs, nil } diff --git a/pkg/skaffold/build/docker/docker.go b/pkg/skaffold/build/docker/docker.go index 53166e303a3..9e9f826435d 100644 --- a/pkg/skaffold/build/docker/docker.go +++ b/pkg/skaffold/build/docker/docker.go @@ -93,15 +93,10 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, a *latest.Artifact, func (b *Builder) dockerCLIBuild(ctx context.Context, out io.Writer, name string, workspace string, dockerfilePath string, a *latest.DockerArtifact, opts docker.BuildOptions, pl v1.Platform) (string, error) { args := []string{"build", workspace, "--file", dockerfilePath, "-t", opts.Tag} - imgRef, err := docker.ParseReference(opts.Tag) + imageInfoEnv, err := docker.EnvTags(opts.Tag) if err != nil { return "", fmt.Errorf("couldn't parse image tag: %w", err) } - imageInfoEnv := map[string]string{ - "IMAGE_REPO": imgRef.Repo, - "IMAGE_NAME": imgRef.Name, - "IMAGE_TAG": imgRef.Tag, - } ba, err := docker.EvalBuildArgsWithEnv(b.cfg.Mode(), workspace, a.DockerfilePath, a.BuildArgs, opts.ExtraBuildArgs, imageInfoEnv) if err != nil { return "", fmt.Errorf("unable to evaluate build args: %w", err) diff --git a/pkg/skaffold/docker/build_args.go b/pkg/skaffold/docker/build_args.go index fa45af54520..73c0cf32d0a 100644 --- a/pkg/skaffold/docker/build_args.go +++ b/pkg/skaffold/docker/build_args.go @@ -114,3 +114,19 @@ func ResolveDependencyImages(deps []*latest.ArtifactDependency, r ArtifactResolv } return m } + +// EnvTags generate a set of build tags from the docker image name. +func EnvTags(tag string) (map[string]string, error) { + imgRef, err := ParseReference(tag) + if err != nil { + return nil, fmt.Errorf("couldn't parse image tag %s %w", tag, err) + } + if imgRef.Tag == "" { + imgRef.Tag = "latest" + } + return map[string]string{ + "IMAGE_REPO": imgRef.Repo, + "IMAGE_NAME": imgRef.Name, + "IMAGE_TAG": imgRef.Tag, + }, nil +} diff --git a/pkg/skaffold/docker/image.go b/pkg/skaffold/docker/image.go index 02455ea3495..1ac35b86b5d 100644 --- a/pkg/skaffold/docker/image.go +++ b/pkg/skaffold/docker/image.go @@ -323,15 +323,10 @@ func (l *localDaemon) Build(ctx context.Context, out io.Writer, workspace string if err := l.CheckCompatible(a); err != nil { return "", err } - imgRef, err := ParseReference(opts.Tag) + imageInfoEnv, err := EnvTags(opts.Tag) if err != nil { return "", fmt.Errorf("couldn't parse image tag: %w", err) } - imageInfoEnv := map[string]string{ - "IMAGE_REPO": imgRef.Repo, - "IMAGE_NAME": imgRef.Name, - "IMAGE_TAG": imgRef.Tag, - } buildArgs, err := EvalBuildArgsWithEnv(opts.Mode, workspace, a.DockerfilePath, a.BuildArgs, opts.ExtraBuildArgs, imageInfoEnv) if err != nil { return "", fmt.Errorf("unable to evaluate build args: %w", err) diff --git a/pkg/skaffold/graph/dependencies.go b/pkg/skaffold/graph/dependencies.go index eb84edf2aad..e41a77b82c3 100644 --- a/pkg/skaffold/graph/dependencies.go +++ b/pkg/skaffold/graph/dependencies.go @@ -109,19 +109,6 @@ func (r *dependencyResolverImpl) Reset() { r.cache = util.NewSyncStore[[]string]() } -// EnvTags generate a set of build tags from the docker image name. -func EnvTags(tag string) (map[string]string, error) { - imgRef, err := docker.ParseReference(tag) - if err != nil { - return nil, fmt.Errorf("couldn't parse image tag %s %w", tag, err) - } - return map[string]string{ - "IMAGE_REPO": imgRef.Repo, - "IMAGE_NAME": imgRef.Name, - "IMAGE_TAG": imgRef.Tag, - }, nil -} - // sourceDependenciesForArtifact returns the build dependencies for the current artifact. func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver, tag string) ([]string, error) { var ( From 7ca8dbc7315b52d1e2d2f275206c04c0c92464d9 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 22:25:19 +0000 Subject: [PATCH 42/44] Replace use of EvalBuildArgs with EvalBuildArgsWithEnv since they invoke the same function --- pkg/skaffold/build/cache/hash.go | 14 ++++++++++---- pkg/skaffold/build/cache/retrieve_test.go | 12 ------------ pkg/skaffold/build/gcb/docker.go | 2 +- pkg/skaffold/build/gcb/docker_test.go | 4 ++-- pkg/skaffold/build/gcb/kaniko.go | 6 +++++- pkg/skaffold/build/gcb/kaniko_test.go | 2 +- pkg/skaffold/docker/build_args_test.go | 4 ++-- pkg/skaffold/graph/dependencies.go | 12 ++++++------ 8 files changed, 27 insertions(+), 29 deletions(-) diff --git a/pkg/skaffold/build/cache/hash.go b/pkg/skaffold/build/cache/hash.go index 5bfb2715a0c..e103fdd2463 100644 --- a/pkg/skaffold/build/cache/hash.go +++ b/pkg/skaffold/build/cache/hash.go @@ -133,7 +133,7 @@ func singleArtifactHash(ctx context.Context, out io.Writer, depLister Dependency } // add build args for the artifact if specified - args, err := hashBuildArgs(out, a, mode) + args, err := hashBuildArgs(out, a, tag, mode) if err != nil { return "", fmt.Errorf("hashing build args: %w", err) } @@ -171,16 +171,22 @@ func artifactConfig(a *latest.Artifact) (string, error) { return string(buf), nil } -func hashBuildArgs(out io.Writer, artifact *latest.Artifact, mode config.RunMode) ([]string, error) { +func hashBuildArgs(out io.Writer, artifact *latest.Artifact, tag string, mode config.RunMode) ([]string, error) { // only one of args or env is ever populated var args map[string]*string var env map[string]string var err error + + envTags, evalErr := docker.EnvTags(tag) + if evalErr != nil { + return nil, fmt.Errorf("unable to create build args: %w", err) + } + switch { case artifact.DockerArtifact != nil: - args, err = docker.EvalBuildArgs(mode, artifact.Workspace, artifact.DockerArtifact.DockerfilePath, artifact.DockerArtifact.BuildArgs, nil) + args, err = docker.EvalBuildArgsWithEnv(mode, artifact.Workspace, artifact.DockerArtifact.DockerfilePath, artifact.DockerArtifact.BuildArgs, nil, envTags) case artifact.KanikoArtifact != nil: - args, err = docker.EvalBuildArgs(mode, kaniko.GetContext(artifact.KanikoArtifact, artifact.Workspace), artifact.KanikoArtifact.DockerfilePath, artifact.KanikoArtifact.BuildArgs, nil) + args, err = docker.EvalBuildArgsWithEnv(mode, kaniko.GetContext(artifact.KanikoArtifact, artifact.Workspace), artifact.KanikoArtifact.DockerfilePath, artifact.KanikoArtifact.BuildArgs, nil, envTags) case artifact.BuildpackArtifact != nil: env, err = buildpacks.GetEnv(out, artifact, mode) case artifact.CustomArtifact != nil && artifact.CustomArtifact.Dependencies.Dockerfile != nil: diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index 171a0873474..7abbf80b97c 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -138,10 +138,6 @@ func TestCacheBuildLocal(t *testing.T) { return dockerDaemon, nil }) - // Mock args builder - t.Override(&docker.EvalBuildArgs, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string) (map[string]*string, error) { - return args, nil - }) t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string, _ map[string]string) (map[string]*string, error) { return args, nil }) @@ -238,10 +234,6 @@ func TestCacheBuildRemote(t *testing.T) { } }) - // Mock args builder - t.Override(&docker.EvalBuildArgs, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string) (map[string]*string, error) { - return args, nil - }) t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string, _ map[string]string) (map[string]*string, error) { return args, nil }) @@ -326,10 +318,6 @@ func TestCacheFindMissing(t *testing.T) { } }) - // Mock args builder - t.Override(&docker.EvalBuildArgs, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string) (map[string]*string, error) { - return args, nil - }) t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string, _ map[string]string) (map[string]*string, error) { return args, nil }) diff --git a/pkg/skaffold/build/gcb/docker.go b/pkg/skaffold/build/gcb/docker.go index b23431cac4b..01c64634d71 100644 --- a/pkg/skaffold/build/gcb/docker.go +++ b/pkg/skaffold/build/gcb/docker.go @@ -103,7 +103,7 @@ func (b *Builder) dockerBuildArgs(a *latest.Artifact, tag string, deps []*latest return nil, errors.New("docker build options, secrets and ssh, are not currently supported in GCB builds") } requiredImages := docker.ResolveDependencyImages(deps, b.artifactStore, true) - buildArgs, err := docker.EvalBuildArgs(b.cfg.Mode(), a.Workspace, d.DockerfilePath, d.BuildArgs, requiredImages) + buildArgs, err := docker.EvalBuildArgsWithEnv(b.cfg.Mode(), a.Workspace, d.DockerfilePath, d.BuildArgs, requiredImages, nil) if err != nil { return nil, fmt.Errorf("unable to evaluate build args: %w", err) } diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go index c27aab2da16..bd8926c3b9c 100644 --- a/pkg/skaffold/build/gcb/docker_test.go +++ b/pkg/skaffold/build/gcb/docker_test.go @@ -175,7 +175,7 @@ func TestDockerBuildSpec(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - t.Override(&docker.EvalBuildArgs, func(_ config.RunMode, _ string, _ string, args map[string]*string, extra map[string]*string) (map[string]*string, error) { + t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, extra map[string]*string, _ map[string]string) (map[string]*string, error) { m := make(map[string]*string) for k, v := range args { m[k] = v @@ -290,7 +290,7 @@ func TestPullCacheFrom(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - t.Override(&docker.EvalBuildArgs, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string) (map[string]*string, error) { + t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string, _ map[string]*string) (map[string]*string, error) { return args, nil }) builder := NewBuilder(&mockBuilderContext{}, &latest.GoogleCloudBuild{ diff --git a/pkg/skaffold/build/gcb/kaniko.go b/pkg/skaffold/build/gcb/kaniko.go index 2d64f03266c..5cbe0b39b30 100644 --- a/pkg/skaffold/build/gcb/kaniko.go +++ b/pkg/skaffold/build/gcb/kaniko.go @@ -32,7 +32,11 @@ func (b *Builder) kanikoBuildSpec(a *latest.Artifact, tag string) (cloudbuild.Bu k := a.KanikoArtifact requiredImages := docker.ResolveDependencyImages(a.Dependencies, b.artifactStore, true) // add required artifacts as build args - buildArgs, err := docker.EvalBuildArgs(b.cfg.Mode(), kaniko.GetContext(a.KanikoArtifact, a.Workspace), k.DockerfilePath, k.BuildArgs, requiredImages) + envTags, err := docker.EnvTags(tag) + if err != nil { + return cloudbuild.Build{}, fmt.Errorf("unable to create build args: %w", err) + } + buildArgs, err := docker.EvalBuildArgsWithEnv(b.cfg.Mode(), kaniko.GetContext(a.KanikoArtifact, a.Workspace), k.DockerfilePath, k.BuildArgs, requiredImages, envTags) if err != nil { return cloudbuild.Build{}, fmt.Errorf("unable to evaluate build args: %w", err) } diff --git a/pkg/skaffold/build/gcb/kaniko_test.go b/pkg/skaffold/build/gcb/kaniko_test.go index 6a464d6f615..fbb927ccfed 100644 --- a/pkg/skaffold/build/gcb/kaniko_test.go +++ b/pkg/skaffold/build/gcb/kaniko_test.go @@ -467,7 +467,7 @@ func TestKanikoBuildSpec(t *testing.T) { imageArgs := []string{kaniko.BuildArgsFlag, "IMG2=img2:tag", kaniko.BuildArgsFlag, "IMG3=img3:tag"} - t.Override(&docker.EvalBuildArgs, func(_ config.RunMode, _ string, _ string, args map[string]*string, extra map[string]*string) (map[string]*string, error) { + t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, extra map[string]*string, _ map[string]string) (map[string]*string, error) { m := make(map[string]*string) for k, v := range args { m[k] = v diff --git a/pkg/skaffold/docker/build_args_test.go b/pkg/skaffold/docker/build_args_test.go index a6b839b30d0..f5002b8458a 100644 --- a/pkg/skaffold/docker/build_args_test.go +++ b/pkg/skaffold/docker/build_args_test.go @@ -210,7 +210,7 @@ FROM bar1`, tmpDir.Write("./Dockerfile", test.dockerfile) workspace := tmpDir.Path(".") - actual, err := EvalBuildArgs(test.mode, workspace, artifact.DockerfilePath, artifact.BuildArgs, test.extra) + actual, err := EvalBuildArgsWithEnv(test.mode, workspace, artifact.DockerfilePath, artifact.BuildArgs, test.extra, nil) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) }) @@ -274,7 +274,7 @@ func TestBuildArgTemplating(t *testing.T) { tmpDir.Write("./Dockerfile", dockerFile) workspace := tmpDir.Path(".") - filledMap, err := EvalBuildArgs(config.RunModes.Dev, workspace, artifact.DockerfilePath, artifact.BuildArgs, nil) + filledMap, err := EvalBuildArgsWithEnv(config.RunModes.Dev, workspace, artifact.DockerfilePath, artifact.BuildArgs, nil, nil) t.CheckNil(err) t.CheckMatches(*filledMap["MY_KEY"], "abc123") diff --git a/pkg/skaffold/graph/dependencies.go b/pkg/skaffold/graph/dependencies.go index e41a77b82c3..1854d19f2a8 100644 --- a/pkg/skaffold/graph/dependencies.go +++ b/pkg/skaffold/graph/dependencies.go @@ -116,6 +116,11 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg err error ) + envTags, evalErr := docker.EnvTags(tag) + if evalErr != nil { + return nil, fmt.Errorf("unable to create build args: %w", err) + } + switch { case a.DockerArtifact != nil: // Required artifacts cannot be resolved when `ResolveDependencyImages` runs prior to a completed build sequence (like `skaffold build` or the first iteration of `skaffold dev`). @@ -124,11 +129,6 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg // For `dev` it will succeed on the first dev loop and list any additional dependencies found from the base artifact's ONBUILD instructions as a file added instead of modified (see `filemon.Events`) deps := docker.ResolveDependencyImages(a.Dependencies, r, false) - envTags, evalErr := EnvTags(tag) - if evalErr != nil { - return nil, fmt.Errorf("unable to create build args: %w", err) - } - args, evalErr := docker.EvalBuildArgsWithEnv(cfg.Mode(), a.Workspace, a.DockerArtifact.DockerfilePath, a.DockerArtifact.BuildArgs, deps, envTags) if evalErr != nil { return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr) @@ -137,7 +137,7 @@ func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg case a.KanikoArtifact != nil: deps := docker.ResolveDependencyImages(a.Dependencies, r, false) - args, evalErr := docker.EvalBuildArgs(cfg.Mode(), kaniko.GetContext(a.KanikoArtifact, a.Workspace), a.KanikoArtifact.DockerfilePath, a.KanikoArtifact.BuildArgs, deps) + args, evalErr := docker.EvalBuildArgsWithEnv(cfg.Mode(), kaniko.GetContext(a.KanikoArtifact, a.Workspace), a.KanikoArtifact.DockerfilePath, a.KanikoArtifact.BuildArgs, deps, envTags) if evalErr != nil { return nil, fmt.Errorf("unable to evaluate build args: %w", evalErr) } From 9fd1b348f606159fd6d54d314964d51c07ea201a Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 23:17:46 +0000 Subject: [PATCH 43/44] add unit test for hash package --- pkg/skaffold/build/cache/hash_test.go | 128 ++++++++++++++++++++++-- pkg/skaffold/build/gcb/docker_test.go | 2 +- pkg/skaffold/graph/dependencies_test.go | 1 + 3 files changed, 119 insertions(+), 12 deletions(-) diff --git a/pkg/skaffold/build/cache/hash_test.go b/pkg/skaffold/build/cache/hash_test.go index 04445ed406c..75c3b6ad654 100644 --- a/pkg/skaffold/build/cache/hash_test.go +++ b/pkg/skaffold/build/cache/hash_test.go @@ -50,6 +50,8 @@ var fakeArtifactConfig = func(a *latest.Artifact) (string, error) { return "", nil } +var testTag = "gcr.io/k8s-skaffold/skaffold:latest" + const Dockerfile = "Dockerfile" func TestGetHashForArtifact(t *testing.T) { @@ -162,7 +164,7 @@ func TestGetHashForArtifact(t *testing.T) { } depLister := stubDependencyLister(test.dependencies) - actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms, "") + actual, err := newArtifactHasher(nil, depLister, test.mode).hash(context.Background(), io.Discard, test.artifact, test.platforms, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -246,7 +248,7 @@ func TestGetHashForArtifactWithDependencies(t *testing.T) { return test.fileDeps[a.ImageName], nil } - actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}, "") + actual, err := newArtifactHasher(g, depLister, test.mode).hash(context.Background(), io.Discard, test.artifacts[0], platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) @@ -309,21 +311,21 @@ func TestBuildArgs(t *testing.T) { } t.Override(&fileHasherFunc, mockCacheHasher) t.Override(&artifactConfigFunc, fakeArtifactConfig) - actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") + actual, err := newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change order of buildargs artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"two": util.Ptr("2"), "one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckDeepEqual(test.expected, actual) // Change build args, get different hash artifact.ArtifactType.DockerArtifact.BuildArgs = map[string]*string{"one": util.Ptr("1")} - actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") + actual, err = newArtifactHasher(nil, stubDependencyLister(nil), test.mode).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) if actual == test.expected { @@ -356,7 +358,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { t.Override(&artifactConfigFunc, fakeArtifactConfig) depLister := stubDependencyLister([]string{"graph"}) - hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") + hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) @@ -366,7 +368,7 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { return []string{"FOO=baz"} } - hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, "") + hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, artifact, platform.Resolver{}, testTag) t.CheckNoError(err) if hash1 == hash2 { @@ -375,6 +377,78 @@ func TestBuildArgsEnvSubstitution(t *testing.T) { }) } +func TestBuildArgsImageInfoSubstitution(t *testing.T) { + tests := []struct { + description string + artifactType *latest.Artifact + originalTag string + newTag string + }{ + { + description: "hash differs if image repo changes", + artifactType: &latest.Artifact{ + ArtifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{ + BuildArgs: map[string]*string{"IMAGE_REPO": util.Ptr("${{.IMAGE_REPO}}")}, + DockerfilePath: Dockerfile, + }, + }, + }, + originalTag: "gcr.io/k8s-skaffold/skaffold:latest", + newTag: "us.gcr.io/k8s-skaffold/skaffold:latest", + }, + { + description: "hash differs if image name changes", + artifactType: &latest.Artifact{ + ArtifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{ + BuildArgs: map[string]*string{"IMAGE_NAME": util.Ptr("${{.IMAGE_NAME}}")}, + DockerfilePath: Dockerfile, + }, + }, + }, + originalTag: "gcr.io/k8s-skaffold/skaffold:latest", + newTag: "gcr.io/k8s-skaffold/new-skaffold:latest", + }, + { + description: "hash differs if image tag changes", + artifactType: &latest.Artifact{ + ArtifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{ + BuildArgs: map[string]*string{"IMAGE_TAG": util.Ptr("${{.IMAGE_TAG}}")}, + DockerfilePath: Dockerfile, + }, + }, + }, + originalTag: "gcr.io/k8s-skaffold/skaffold:v1", + newTag: "gcr.io/k8s-skaffold/skaffold:v2", + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + tmpDir := t.NewTempDir() + tmpDir.Write("./Dockerfile", "ARG SKAFFOLD_GO_GCFLAGS\nFROM foo") + test.artifactType.Workspace = tmpDir.Path(".") + + t.Override(&fileHasherFunc, mockCacheHasher) + t.Override(&artifactConfigFunc, fakeArtifactConfig) + + depLister := stubDependencyLister([]string{"graph"}) + hash1, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, test.artifactType, platform.Resolver{}, test.originalTag) + + t.CheckNoError(err) + + hash2, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, test.artifactType, platform.Resolver{}, test.newTag) + + t.CheckNoError(err) + if hash1 == hash2 { + t.Fatal("hashes are the same even though build arg changed") + } + }) + } +} + func TestCacheHasher(t *testing.T) { tests := []struct { description string @@ -423,7 +497,7 @@ func TestCacheHasher(t *testing.T) { path := originalFile depLister := stubDependencyLister([]string{tmpDir.Path(originalFile)}) - oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, "") + oldHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, testTag) t.CheckNoError(err) test.update(originalFile, tmpDir) @@ -432,7 +506,7 @@ func TestCacheHasher(t *testing.T) { } depLister = stubDependencyLister([]string{tmpDir.Path(path)}) - newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, "") + newHash, err := newArtifactHasher(nil, depLister, config.RunModes.Build).hash(context.Background(), io.Discard, &latest.Artifact{}, platform.Resolver{}, testTag) t.CheckNoError(err) t.CheckFalse(test.differentHash && oldHash == newHash) @@ -445,6 +519,7 @@ func TestHashBuildArgs(t *testing.T) { tests := []struct { description string artifactType latest.ArtifactType + tag string expected []string mode config.RunMode }{ @@ -547,6 +622,33 @@ func TestHashBuildArgs(t *testing.T) { }, }, }, + { + description: "docker artifact with image info build args", + artifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{ + BuildArgs: map[string]*string{ + "IMAGE_REPO": util.Ptr("{{.IMAGE_REPO}}"), + "IMAGE_NAME": util.Ptr("{{.IMAGE_NAME}}"), + "IMAGE_TAG": util.Ptr("{{.IMAGE_TAG}}"), + }, + }, + }, + mode: config.RunModes.Debug, + expected: []string{"SKAFFOLD_GO_GCFLAGS=all=-N -l", "IMAGE_REPO=gcr.io/k8s-skaffold", "IMAGE_NAME=skaffold", "IMAGE_TAG=latest"}, + }, + { + description: "the tag segment has 'latest' as default if no tag segment was provided", + tag: "busybox", + artifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{ + BuildArgs: map[string]*string{ + "IMAGE_TAG": util.Ptr("{{.IMAGE_TAG}}"), + }, + }, + }, + mode: config.RunModes.Debug, + expected: []string{"SKAFFOLD_GO_GCFLAGS=all=-N -l", "IMAGE_TAG=latest"}, + }, } for _, test := range tests { @@ -566,9 +668,13 @@ func TestHashBuildArgs(t *testing.T) { a.Workspace = tmpDir.Path(".") a.ArtifactType.KanikoArtifact.DockerfilePath = Dockerfile } - actual, err := hashBuildArgs(io.Discard, a, test.mode) + + if test.tag == "" { + test.tag = testTag + } + actual, err := hashBuildArgs(io.Discard, a, testTag, test.mode) t.CheckNoError(err) - t.CheckDeepEqual(test.expected, actual) + t.CheckElementsMatch(test.expected, actual) }) } } diff --git a/pkg/skaffold/build/gcb/docker_test.go b/pkg/skaffold/build/gcb/docker_test.go index bd8926c3b9c..6c8d0e36049 100644 --- a/pkg/skaffold/build/gcb/docker_test.go +++ b/pkg/skaffold/build/gcb/docker_test.go @@ -290,7 +290,7 @@ func TestPullCacheFrom(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string, _ map[string]*string) (map[string]*string, error) { + t.Override(&docker.EvalBuildArgsWithEnv, func(_ config.RunMode, _ string, _ string, args map[string]*string, _ map[string]*string, _ map[string]string) (map[string]*string, error) { return args, nil }) builder := NewBuilder(&mockBuilderContext{}, &latest.GoogleCloudBuild{ diff --git a/pkg/skaffold/graph/dependencies_test.go b/pkg/skaffold/graph/dependencies_test.go index cbbea68fde6..0137f8ff7a1 100644 --- a/pkg/skaffold/graph/dependencies_test.go +++ b/pkg/skaffold/graph/dependencies_test.go @@ -85,6 +85,7 @@ func TestSourceDependenciesForArtifact(t *testing.T) { }, Workspace: tmpDir.Root(), }, + tag: "gcr.io/distroless/base:latest", expectedPaths: []string{ filepath.Join(tmpDir.Root(), "dir2/frob.go"), filepath.Join(tmpDir.Root(), "bar.go"), From c6298fe0358940d77677436d58768294c01abdf0 Mon Sep 17 00:00:00 2001 From: Angel Montero Date: Wed, 22 Jan 2025 23:31:03 +0000 Subject: [PATCH 44/44] add test for TestCheckArtifacts --- pkg/skaffold/diagnose/diagnose_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pkg/skaffold/diagnose/diagnose_test.go b/pkg/skaffold/diagnose/diagnose_test.go index 6580c7a948a..29d5e12f4e1 100644 --- a/pkg/skaffold/diagnose/diagnose_test.go +++ b/pkg/skaffold/diagnose/diagnose_test.go @@ -96,6 +96,24 @@ func TestCheckArtifacts(t *testing.T) { }) } +func TestCheckArtifactsFailure(t *testing.T) { + testutil.Run(t, "", func(t *testutil.T) { + tmpDir := t.NewTempDir().Write("Dockerfile", "FROM busybox") + err := CheckArtifacts(context.Background(), &mockConfig{ + artifacts: []*latest.Artifact{{ + ImageName: "base", + Workspace: tmpDir.Root(), + ArtifactType: latest.ArtifactType{ + DockerArtifact: &latest.DockerArtifact{ + DockerfilePath: "Dockerfile", + }, + }, + }}, + }, nil, io.Discard) + t.CheckError(true, err) + }) +} + type mockConfig struct { runcontext.RunContext // Embedded to provide the default values. artifacts []*latest.Artifact