fix image name matching logic

Author: kterui9019

pkg/skaffold/deploy/docker/deploy.go

@@ -636,6 +636,28 @@ func (d *Deployer) getComposeFilePath() string {
 	return "docker-compose.yml"
 }
 
+// imageNameMatches checks if a compose image name matches an artifact's built image.
+// It handles cases like:
+//   - "frontend" matching "frontend:tag" or "gcr.io/project/frontend:tag"
+//   - "gcr.io/project/frontend" matching "gcr.io/project/frontend:tag"
+// But rejects false positives like:
+//   - "app" should NOT match "my-app"
+func imageNameMatches(composeImage, artifactTag string) bool {
+	// Remove tags from both images
+	composeBase := strings.Split(composeImage, ":")[0]
+	artifactBase := strings.Split(artifactTag, ":")[0]
+
+	// Exact match (most common case)
+	if composeBase == artifactBase {
+		return true
+	}
+
+	// Check if artifact image ends with compose image (handles registry prefixes)
+	// e.g., "frontend" matches "gcr.io/project/frontend"
+	// But "app" does NOT match "my-app" (no "/" separator)
+	return strings.HasSuffix(artifactBase, "/"+composeBase)
+}
+
 // replaceComposeImages replaces image names in compose config with skaffold-built images
 func (d *Deployer) replaceComposeImages(composeConfig map[string]interface{}, artifact graph.Artifact) error {
 	services, ok := composeConfig["services"].(map[string]interface{})
@@ -652,8 +674,8 @@ func (d *Deployer) replaceComposeImages(composeConfig map[string]interface{}, ar
 
 		// Check if service has an image field
 		if imageName, ok := service["image"].(string); ok {
-			// Check if this image matches the artifact's image name
-			if imageName == artifact.ImageName || strings.Contains(artifact.ImageName, imageName) {
+			// Check if this image matches the artifact's built image
+			if imageNameMatches(imageName, artifact.Tag) {
 				olog.Entry(context.Background()).Debugf("Replacing image for service %s: %s -> %s", serviceName, imageName, artifact.Tag)
 				service["image"] = artifact.Tag
 			}

pkg/skaffold/deploy/docker/deploy_test.go

@@ -19,7 +19,6 @@ package docker
 import (
 	"context"
 	"os"
-	"strings"
 	"testing"
 
 	"github.com/docker/docker/api/types/container"
@@ -30,10 +29,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/debug/types"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/label"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker/debugger"
-	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker/tracker"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/graph"
-	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/manifest"
-	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/log"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/util"
 	"github.com/GoogleContainerTools/skaffold/v2/testutil"
@@ -284,6 +280,9 @@ func (m mockConfig) ContainerDebugging() bool               { return true }
 func (m mockConfig) GetInsecureRegistries() map[string]bool { return nil }
 func (m mockConfig) GetKubeContext() string                 { return "" }
 func (m mockConfig) GlobalConfig() string                   { return "" }
+func (m mockConfig) MinikubeProfile() string                { return "" }
+func (m mockConfig) Mode() config.RunMode                   { return "" }
+func (m mockConfig) Prune() bool                            { return false }
 
 // Tests for Docker Compose deployer
 
@@ -407,6 +406,72 @@ func TestReplaceComposeImages(t *testing.T) {
 			},
 			shouldErr: false,
 		},
+		{
+			name: "reject false positive: 'app' should not match 'my-app'",
+			composeConfig: map[string]interface{}{
+				"services": map[string]interface{}{
+					"myapp": map[string]interface{}{
+						"image": "app",
+					},
+				},
+			},
+			artifact: graph.Artifact{
+				ImageName: "my-app",
+				Tag:       "my-app:v1.0.0",
+			},
+			expectedConfig: map[string]interface{}{
+				"services": map[string]interface{}{
+					"myapp": map[string]interface{}{
+						"image": "app", // Should NOT be replaced
+					},
+				},
+			},
+			shouldErr: false,
+		},
+		{
+			name: "match with registry prefix",
+			composeConfig: map[string]interface{}{
+				"services": map[string]interface{}{
+					"frontend": map[string]interface{}{
+						"image": "frontend",
+					},
+				},
+			},
+			artifact: graph.Artifact{
+				ImageName: "frontend",
+				Tag:       "gcr.io/project/frontend:sha256-abc",
+			},
+			expectedConfig: map[string]interface{}{
+				"services": map[string]interface{}{
+					"frontend": map[string]interface{}{
+						"image": "gcr.io/project/frontend:sha256-abc",
+					},
+				},
+			},
+			shouldErr: false,
+		},
+		{
+			name: "match with tag in compose file",
+			composeConfig: map[string]interface{}{
+				"services": map[string]interface{}{
+					"backend": map[string]interface{}{
+						"image": "backend:latest",
+					},
+				},
+			},
+			artifact: graph.Artifact{
+				ImageName: "backend",
+				Tag:       "backend:v2.0.0",
+			},
+			expectedConfig: map[string]interface{}{
+				"services": map[string]interface{}{
+					"backend": map[string]interface{}{
+						"image": "backend:v2.0.0",
+					},
+				},
+			},
+			shouldErr: false,
+		},
 	}
 
 	for _, test := range tests {
@@ -542,146 +607,3 @@ func TestDeployWithComposeFileOperations(t *testing.T) {
 		})
 	}
 }
-
-func (m mockConfig) MinikubeProfile() string { return "" }
-func (m mockConfig) Mode() config.RunMode    { return "" }
-func (m mockConfig) Prune() bool             { return false }
-
-// createMockDockerCommand creates a fake 'docker' command that records invocations
-// This allows us to test command execution without modifying production code
-func createMockDockerCommand(t *testing.T, tmpDir string) (binPath string, counterFile string) {
-	t.Helper()
-
-	// Create a bin directory for our fake docker command
-	binDir := tmpDir + "/bin"
-	if err := os.MkdirAll(binDir, 0755); err != nil {
-		t.Fatalf("Failed to create bin directory: %v", err)
-	}
-
-	// Counter file to track invocations
-	counterFile = tmpDir + "/docker-call-count.txt"
-
-	// Create a fake docker script that records calls
-	dockerScript := binDir + "/docker"
-	scriptContent := `#!/bin/bash
-# Mock docker command that tracks invocations
-echo "1" >> "` + counterFile + `"
-
-# Log the arguments for debugging
-echo "$@" >> "` + tmpDir + `/docker-args.log"
-
-# Exit successfully
-exit 0
-`
-	if err := os.WriteFile(dockerScript, []byte(scriptContent), 0755); err != nil {
-		t.Fatalf("Failed to create mock docker script: %v", err)
-	}
-
-	return binDir, counterFile
-}
-
-// TestDeployWithCompose_MultipleArtifacts_CallsComposeUpOnce verifies that docker compose up
-// is called only once when deploying multiple artifacts, with all image replacements done together.
-// This test uses a mock docker command to track invocations without modifying production code.
-func TestDeployWithCompose_MultipleArtifacts_CallsComposeUpOnce(t *testing.T) {
-	testutil.Run(t, "multiple artifacts trigger compose up only once", func(tt *testutil.T) {
-		// Create temporary directory for test files
-		tmpDir := t.TempDir()
-
-		// Create mock docker command
-		binDir, counterFile := createMockDockerCommand(t, tmpDir)
-
-		// Modify PATH to use our mock docker command
-		originalPath := os.Getenv("PATH")
-		os.Setenv("PATH", binDir+":"+originalPath)
-		defer os.Setenv("PATH", originalPath)
-
-		// Create a temporary compose file for testing
-		composeFile := tmpDir + "/docker-compose.yml"
-		composeContent := `version: '3'
-services:
-  frontend:
-    image: frontend
-  backend:
-    image: backend
-`
-		if err := os.WriteFile(composeFile, []byte(composeContent), 0644); err != nil {
-			t.Fatalf("Failed to write compose file: %v", err)
-		}
-
-		// Set environment variable to use our test compose file
-		originalEnv := os.Getenv("SKAFFOLD_COMPOSE_FILE")
-		os.Setenv("SKAFFOLD_COMPOSE_FILE", composeFile)
-		defer func() {
-			if originalEnv != "" {
-				os.Setenv("SKAFFOLD_COMPOSE_FILE", originalEnv)
-			} else {
-				os.Unsetenv("SKAFFOLD_COMPOSE_FILE")
-			}
-		}()
-
-		// Create a minimal deployer
-		d := &Deployer{
-			cfg: &latest.DockerDeploy{
-				UseCompose: true,
-				Images:     []string{"frontend", "backend"},
-			},
-			labeller: &label.DefaultLabeller{},
-			tracker:  tracker.NewContainerTracker(),
-			logger:   &log.NoopLogger{},
-		}
-
-		// Skip network creation
-		d.once.Do(func() {})
-
-		// Create multiple artifacts
-		builds := []graph.Artifact{
-			{
-				ImageName: "frontend",
-				Tag:       "frontend:v1.0.0",
-			},
-			{
-				ImageName: "backend",
-				Tag:       "backend:v1.0.0",
-			},
-		}
-
-		// Call Deploy
-		err := d.Deploy(context.Background(), os.Stdout, builds, manifest.ManifestListByConfig{})
-		if err != nil {
-			t.Fatalf("Deploy failed: %v", err)
-		}
-
-		// Read the counter file to check how many times docker was called
-		counterData, err := os.ReadFile(counterFile)
-		if err != nil {
-			t.Fatalf("Failed to read counter file: %v", err)
-		}
-
-		// Count lines (each invocation adds one line)
-		lines := 0
-		for _, b := range counterData {
-			if b == '\n' {
-				lines++
-			}
-		}
-
-		// FIXED: docker compose up should be called only once with all artifacts
-		if lines != 1 {
-			t.Errorf("Expected docker compose up to be called 1 time, but was called %d times", lines)
-		}
-
-		// Log arguments for verification
-		argsData, _ := os.ReadFile(tmpDir + "/docker-args.log")
-		t.Logf("SUCCESS: docker compose up was called %d time for %d artifacts", lines, len(builds))
-		t.Logf("Docker command arguments:\n%s", string(argsData))
-
-		// Verify it was a compose up command
-		if lines == 1 {
-			argsStr := string(argsData)
-			if !strings.Contains(argsStr, "compose") || !strings.Contains(argsStr, "up") {
-				t.Errorf("Expected 'docker compose up' command, got: %s", argsStr)
-			}
-		}
-	})
-}