Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make IMAGE_TAG available in buildArgs when used in docker FROM #9664

Merged
merged 46 commits into from
Jan 23, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
1657f18
provide a docker.ImageReference in dependency resolution step
abe-winter Jun 19, 2024
147d21b
pass nils
abe-winter Jun 19, 2024
35a37c1
other plumbing
abe-winter Jun 19, 2024
f6411bf
fix: add back out io.Writer field
alphanota Jan 14, 2025
354b03c
Merge branch 'GoogleContainerTools:main' into abe-winter-changes
alphanota Jan 14, 2025
a61996c
fix bugs
alphanota Jan 14, 2025
13849cf
Merge branch 'GoogleContainerTools:main' into abe-winter-changes
alphanota Jan 15, 2025
d506f62
fix lookup and hash tests
alphanota Jan 15, 2025
c3bcf4d
fix tag test
alphanota Jan 15, 2025
67014b3
fix skaffold/runner
alphanota Jan 15, 2025
5076965
fix skaffold/graph
alphanota Jan 15, 2025
5324000
remove the tags field where its not needed
alphanota Jan 15, 2025
2784556
make fixes to test
alphanota Jan 16, 2025
630979a
fix tests
alphanota Jan 16, 2025
65af3c9
fix another typo
alphanota Jan 16, 2025
7e67078
add license boilerplate
alphanota Jan 16, 2025
ae699c0
add 2025 as a valid year
alphanota Jan 16, 2025
0b19012
chore:add integration test
alphanota Jan 16, 2025
da11b02
format go file
alphanota Jan 16, 2025
0787e5a
fix: lint and test
alphanota Jan 16, 2025
10ed9ec
fix test
alphanota Jan 16, 2025
104f908
fix:lint
alphanota Jan 16, 2025
0469ddd
add debug output
alphanota Jan 17, 2025
20c9e2a
add test action for debugging
alphanota Jan 17, 2025
6090a40
add test
alphanota Jan 17, 2025
a212113
add test
alphanota Jan 17, 2025
d52f744
add test
alphanota Jan 17, 2025
8f7960a
add test
alphanota Jan 17, 2025
a0569f3
add test
alphanota Jan 17, 2025
09b820a
use the default context when building the image
alphanota Jan 17, 2025
19cc68e
fix imports
alphanota Jan 17, 2025
6ea296a
fix: flaky helm deploy unit test by making the order of helm command …
alphanota Jan 17, 2025
fd7d94d
fix lint
alphanota Jan 18, 2025
954da5c
add unit tests and fix existing tests
alphanota Jan 22, 2025
d3556f7
remove xtra file
alphanota Jan 22, 2025
b042e30
fix lint
alphanota Jan 22, 2025
c052c83
test that the build arg used as part of a filename is also replaced
alphanota Jan 22, 2025
004c943
use stub instead of mock for test objects
alphanota Jan 22, 2025
8c4e2e4
use empty string in place of tag because hash_test unit tests mock Si…
alphanota Jan 22, 2025
4a345a2
add integration test to ensure that the build int BUILD args can be u…
alphanota Jan 22, 2025
6ff04ca
fix imports, remove github action workflow used for debugging
alphanota Jan 22, 2025
c4f74ff
fix lint
alphanota Jan 22, 2025
0ddf270
use one function everywhere in the codebase to generate the image inf…
alphanota Jan 22, 2025
7ca8dbc
Replace use of EvalBuildArgs with EvalBuildArgsWithEnv since they inv…
alphanota Jan 22, 2025
9fd1b34
add unit test for hash package
alphanota Jan 22, 2025
c6298fe
add test for TestCheckArtifacts
alphanota Jan 22, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/skaffold/build/cache/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions pkg/skaffold/build/cache/hash.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
)

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 {
Expand All @@ -67,20 +67,20 @@
}
}

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
Expand All @@ -94,15 +94,15 @@
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
Expand All @@ -113,7 +113,7 @@
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)
}
Expand All @@ -133,7 +133,7 @@
}

// add build args for the artifact if specified
args, err := hashBuildArgs(out, a, mode)

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR integration tests (linux) (5.5.0, 0.17.1, 1.34.0, 1.0.0-beta.55, 1.34.0, 502.0.0, 1.19.3, 21, 0)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR integration tests (linux) (5.5.0, 0.17.1, 1.34.0, 1.0.0-beta.55, 1.34.0, 502.0.0, 1.19.3, 21, 2)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR integration tests (linux) (5.5.0, 0.17.1, 1.34.0, 1.0.0-beta.55, 1.34.0, 502.0.0, 1.19.3, 21, 3)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR integration tests (linux) (5.5.0, 0.17.1, 1.34.0, 1.0.0-beta.55, 1.34.0, 502.0.0, 1.19.3, 21, 1)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR linters, checks

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR unit tests (windows)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR unit tests (darwin)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR integration tests (linux) (5.5.0, 0.17.1, 1.34.0, 1.0.0-beta.55, 1.34.0, 502.0.0, 1.19.3, 21, 0)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR integration tests (linux) (5.5.0, 0.17.1, 1.34.0, 1.0.0-beta.55, 1.34.0, 502.0.0, 1.19.3, 21, 2)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR linters, checks

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR unit tests (darwin)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / PR unit tests (windows)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out

Check failure on line 136 in pkg/skaffold/build/cache/hash.go

View workflow job for this annotation

GitHub Actions / unit tests (linux)

undefined: out
if err != nil {
return "", fmt.Errorf("hashing build args: %w", err)
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/skaffold/build/cache/lookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}()
}
Expand All @@ -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)}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/build/gcb/cloud_build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion pkg/skaffold/diagnose/diagnose.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
29 changes: 23 additions & 6 deletions pkg/skaffold/graph/dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand All @@ -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
Expand All @@ -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))
Expand All @@ -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
Expand All @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/runner/new.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading