Skip to content

Fix target in digest tagger generation #9826 #9867

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.

Sign up for GitHub

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

Draft
reingart wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix target in digest tagger generation #9826 #9867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e23f5ca
fix: honor target in input digest tagger
reingart Sep 9, 2025
ee9f445
test: UT for target in input digest
reingart Sep 9, 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
6 changes: 6 additions & 0 deletions pkg/skaffold/tag/input_digest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,16 @@ func (t *inputDigestTagger) GenerateTag(ctx context.Context, image latest.Artifa

if image.DockerArtifact != nil {
srcFiles = append(srcFiles, image.DockerArtifact.DockerfilePath)
if image.DockerArtifact.Target != "" {
inputs = append(inputs, image.DockerArtifact.Target)
}
}

if image.KanikoArtifact != nil {
srcFiles = append(srcFiles, image.KanikoArtifact.DockerfilePath)
if image.KanikoArtifact.Target != "" {
inputs = append(inputs, image.KanikoArtifact.Target)
}
}
Comment on lines 60 to 72
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic for handling DockerArtifact and KanikoArtifact is identical, leading to code duplication. To improve maintainability, you can refactor this to avoid repetition.

Assuming that an artifact can only be of one type (e.g., either DockerArtifact or KanikoArtifact, but not both), which is suggested by the oneOf=artifact struct tag, you can use an if/else if structure to determine the dockerfilePath and target first, and then process them in a single block.

Suggested change
if image.DockerArtifact != nil {
srcFiles = append(srcFiles, image.DockerArtifact.DockerfilePath)
if image.DockerArtifact.Target != "" {
inputs = append(inputs, image.DockerArtifact.Target)
}
}
if image.KanikoArtifact != nil {
srcFiles = append(srcFiles, image.KanikoArtifact.DockerfilePath)
if image.KanikoArtifact.Target != "" {
inputs = append(inputs, image.KanikoArtifact.Target)
}
}
var dockerfilePath, target string
var hasDockerfileArtifact bool
if image.DockerArtifact != nil {
dockerfilePath = image.DockerArtifact.DockerfilePath
target = image.DockerArtifact.Target
hasDockerfileArtifact = true
} else if image.KanikoArtifact != nil {
dockerfilePath = image.KanikoArtifact.DockerfilePath
target = image.KanikoArtifact.Target
hasDockerfileArtifact = true
}
if hasDockerfileArtifact {
srcFiles = append(srcFiles, dockerfilePath)
if target != "" {
inputs = append(inputs, target)
}
}


if image.CustomArtifact != nil && image.CustomArtifact.Dependencies != nil && image.CustomArtifact.Dependencies.Dockerfile != nil {
Expand Down
65 changes: 65 additions & 0 deletions pkg/skaffold/tag/input_digest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,3 +153,68 @@ CMD [ "true" ]
}
})
}

func TestGenerateTag_DifferentTarget(t *testing.T) {
runCtx := &runcontext.RunContext{}
dockerfilePath := filepath.Join(t.TempDir(), "Dockerfile")
if err := os.WriteFile(dockerfilePath, []byte("FROM busybox\nCMD [\"ps\", \"faux\"]\n"), 0644); err != nil {
t.Fatalf("failed to write dockerfile: %v", err)
}

digestExample, _ := NewInputDigestTagger(runCtx, graph.ToArtifactGraph(runCtx.Artifacts()))
tag1, err := digestExample.GenerateTag(context.Background(), latest.Artifact{
Workspace: t.TempDir(),
ArtifactType: latest.ArtifactType{
DockerArtifact: &latest.DockerArtifact{
DockerfilePath: dockerfilePath,
Target: "target1",
},
},
})
if err != nil {
t.Fatalf("GenerateTag failed for target1: %v", err)
}

digestExample, _ = NewInputDigestTagger(runCtx, graph.ToArtifactGraph(runCtx.Artifacts()))
tag2, err := digestExample.GenerateTag(context.Background(), latest.Artifact{
Workspace: t.TempDir(),
ArtifactType: latest.ArtifactType{
DockerArtifact: &latest.DockerArtifact{
DockerfilePath: dockerfilePath,
Target: "target2",
},
},
})
if err != nil {
t.Fatalf("GenerateTag failed for target2: %v", err)
}

if tag1 == tag2 {
t.Errorf("expected different tags for different targets, got same: %s", tag1)
}
}
Comment on lines +157 to +195
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This test has a couple of issues in its setup:

  1. The inputDigestTagger is instantiated twice unnecessarily. The tagger instance can be reused for generating both tags since the operation is stateless for these calls.
  2. The workspace directory is not handled correctly. t.TempDir() is called multiple times, creating a new directory each time. The Dockerfile is in one temporary directory, while the artifacts are configured with different temporary directories as their workspace. This causes filepath.Rel to fail in fileHasher, which then falls back to using absolute paths. For better test hermeticity, you should create a single temporary directory at the start of the test and use it for both the Dockerfile and the artifact workspaces.
func TestGenerateTag_DifferentTarget(t *testing.T) {
	runCtx := &runcontext.RunContext{}
	dir := t.TempDir()
	dockerfilePath := filepath.Join(dir, "Dockerfile")
	if err := os.WriteFile(dockerfilePath, []byte("FROM busybox\nCMD [\"ps\", \"faux\"]\n"), 0644); err != nil {
		t.Fatalf("failed to write dockerfile: %v", err)
	}

	digestExample, _ := NewInputDigestTagger(runCtx, graph.ToArtifactGraph(runCtx.Artifacts()))
	tag1, err := digestExample.GenerateTag(context.Background(), latest.Artifact{
		Workspace: dir,
		ArtifactType: latest.ArtifactType{
			DockerArtifact: &latest.DockerArtifact{
				DockerfilePath: dockerfilePath,
				Target:         "target1",
			},
		},
	})
	if err != nil {
		t.Fatalf("GenerateTag failed for target1: %v", err)
	}

	tag2, err := digestExample.GenerateTag(context.Background(), latest.Artifact{
		Workspace: dir,
		ArtifactType: latest.ArtifactType{
			DockerArtifact: &latest.DockerArtifact{
				DockerfilePath: dockerfilePath,
				Target:         "target2",
			},
		},
	})
	if err != nil {
		t.Fatalf("GenerateTag failed for target2: %v", err)
	}

	if tag1 == tag2 {
		t.Errorf("expected different tags for different targets, got same: %s", tag1)
	}
}


func TestGenerateTag_NoTarget(t *testing.T) {
runCtx := &runcontext.RunContext{}
dockerfilePath := filepath.Join(t.TempDir(), "Dockerfile")
if err := os.WriteFile(dockerfilePath, []byte("FROM busybox\nCMD [\"ps\", \"faux\"]\n"), 0644); err != nil {
t.Fatalf("failed to write dockerfile: %v", err)
}

digestExample, _ := NewInputDigestTagger(runCtx, graph.ToArtifactGraph(runCtx.Artifacts()))
tag, err := digestExample.GenerateTag(context.Background(), latest.Artifact{
Workspace: t.TempDir(),
ArtifactType: latest.ArtifactType{
DockerArtifact: &latest.DockerArtifact{
DockerfilePath: dockerfilePath,
// No Target field set
},
},
})
if err != nil {
t.Fatalf("GenerateTag failed when no target: %v", err)
}
if tag == "" {
t.Errorf("expected a non-empty tag when no target is set")
}
}
Comment on lines +197 to +220
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to TestGenerateTag_DifferentTarget, this test incorrectly uses t.TempDir(). It's called once to create the Dockerfile's directory and then again for the artifact's workspace, resulting in two different directories. This should be corrected to use a single directory for consistency and to ensure fileHasher can compute a relative path correctly.

func TestGenerateTag_NoTarget(t *testing.T) {
	runCtx := &runcontext.RunContext{}
	dir := t.TempDir()
	dockerfilePath := filepath.Join(dir, "Dockerfile")
	if err := os.WriteFile(dockerfilePath, []byte("FROM busybox\nCMD [\"ps\", \"faux\"]\n"), 0644); err != nil {
		t.Fatalf("failed to write dockerfile: %v", err)
	}

	digestExample, _ := NewInputDigestTagger(runCtx, graph.ToArtifactGraph(runCtx.Artifacts()))
	tag, err := digestExample.GenerateTag(context.Background(), latest.Artifact{
		Workspace: dir,
		ArtifactType: latest.ArtifactType{
			DockerArtifact: &latest.DockerArtifact{
				DockerfilePath: dockerfilePath,
				// No Target field set
			},
		},
	})
	if err != nil {
		t.Fatalf("GenerateTag failed when no target: %v", err)
	}
	if tag == "" {
		t.Errorf("expected a non-empty tag when no target is set")
	}
}