-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
Changes from 17 commits
1657f18
147d21b
35a37c1
f6411bf
354b03c
a61996c
13849cf
d506f62
c3bcf4d
67014b3
5076965
5324000
2784556
630979a
65af3c9
7e67078
ae699c0
0b19012
da11b02
0787e5a
10ed9ec
104f908
0469ddd
20c9e2a
6090a40
a212113
d52f744
8f7960a
a0569f3
09b820a
19cc68e
6ea296a
fd7d94d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
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. | ||
type mockArtifactResolver struct { | ||
m map[string]string | ||
} | ||
|
||
// NewMockArtifactResolver returns a mock ArtifactResolver for testing. | ||
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{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably call this stubArtifactResolver. Googling "stub vs mock" leads to 100s of pages with a variety of explanations, but here's a somewhat reasonable one:
|
||
|
||
// 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{} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
Comment on lines
+68
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there are no unit tests for this new behavior |
||
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,8 +112,21 @@ 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) ([]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 | ||
|
@@ -118,7 +139,8 @@ 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) | ||
|
||
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) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hash_test doesn't test this new behavior at all