Ensure init generates /-delimted paths (#5177)

* Ensure init generates /-delimted paths * Change InitBuilder.ArtifactType() to take the workspace for relative paths * make tests pass * minor tweaks
GoogleContainerTools · Jan 15, 2021 · 36c95e4 · 36c95e4
1 parent 98faaaa
commit 36c95e4
Show file tree

Hide file tree

Showing 16 changed files with 142 additions and 18 deletions.
diff --git a/pkg/skaffold/build/buildpacks/init.go b/pkg/skaffold/build/buildpacks/init.go
@@ -49,7 +49,7 @@ func (c ArtifactConfig) Describe() string {
 }
 
 // ArtifactType returns the type of the artifact to be built.
-func (c ArtifactConfig) ArtifactType() latest.ArtifactType {
+func (c ArtifactConfig) ArtifactType(_ string) latest.ArtifactType {
 	return latest.ArtifactType{
 		BuildpackArtifact: &latest.BuildpackArtifact{
 			Builder: c.Builder,

diff --git a/pkg/skaffold/build/buildpacks/init_test.go b/pkg/skaffold/build/buildpacks/init_test.go
@@ -149,7 +149,7 @@ func TestArtifactType(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			at := test.config.ArtifactType()
+			at := test.config.ArtifactType("ignored") // buildpacks doesn't include file references in its artifacts
 
 			t.CheckDeepEqual(test.expectedType, at)
 		})

diff --git a/pkg/skaffold/build/jib/init.go b/pkg/skaffold/build/jib/init.go
@@ -59,7 +59,7 @@ func (c ArtifactConfig) Describe() string {
 }
 
 // ArtifactType returns the type of the artifact to be built.
-func (c ArtifactConfig) ArtifactType() latest.ArtifactType {
+func (c ArtifactConfig) ArtifactType(_ string) latest.ArtifactType {
 	return latest.ArtifactType{
 		JibArtifact: &latest.JibArtifact{
 			Project: c.Project,

diff --git a/pkg/skaffold/build/jib/init_test.go b/pkg/skaffold/build/jib/init_test.go
@@ -242,7 +242,7 @@ func TestArtifactType(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			at := test.config.ArtifactType()
+			at := test.config.ArtifactType("ignored") // jib doesn't include file references in its artifacts
 
 			t.CheckDeepEqual(test.expectedType, at)
 		})

diff --git a/pkg/skaffold/docker/docker_init.go b/pkg/skaffold/docker/docker_init.go
@@ -52,12 +52,19 @@ func (c ArtifactConfig) Describe() string {
 }
 
 // ArtifactType returns the type of the artifact to be built.
-func (c ArtifactConfig) ArtifactType() latest.ArtifactType {
+func (c ArtifactConfig) ArtifactType(workspace string) latest.ArtifactType {
 	dockerfile := filepath.Base(c.File)
+	if workspace != "" {
+		// attempt to relativize the path
+		if rel, err := filepath.Rel(workspace, c.File); err == nil {
+			dockerfile = rel
+		}
+	}
 
 	return latest.ArtifactType{
 		DockerArtifact: &latest.DockerArtifact{
-			DockerfilePath: dockerfile,
+			// to make skaffold.yaml more portable across OS-es we should always generate /-delimited filePaths
+			DockerfilePath: filepath.ToSlash(dockerfile),
 		},
 	}
 }

diff --git a/pkg/skaffold/docker/docker_init_test.go b/pkg/skaffold/docker/docker_init_test.go
@@ -89,6 +89,7 @@ func TestDescribe(t *testing.T) {
 func TestArtifactType(t *testing.T) {
 	tests := []struct {
 		description  string
+		workspace    string
 		config       ArtifactConfig
 		expectedType latest.ArtifactType
 	}{
@@ -110,10 +111,20 @@ func TestArtifactType(t *testing.T) {
 				},
 			},
 		},
+		{
+			description: "with workspace",
+			config:      ArtifactConfig{File: filepath.Join("path", "to", "Dockerfile")},
+			workspace:   "path",
+			expectedType: latest.ArtifactType{
+				DockerArtifact: &latest.DockerArtifact{
+					DockerfilePath: "to/Dockerfile",
+				},
+			},
+		},
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			at := test.config.ArtifactType()
+			at := test.config.ArtifactType(test.workspace)
 
 			t.CheckDeepEqual(test.expectedType, at)
 		})

diff --git a/pkg/skaffold/initializer/analyze/analyze.go b/pkg/skaffold/initializer/analyze/analyze.go
@@ -152,7 +152,7 @@ func (a *ProjectAnalysis) Analyze(dir string) error {
 			}
 		}
 
-		// to make skaffold.yaml more portable across OS-es we should always generate / based filePaths
+		// to make skaffold.yaml more portable across OS-es we should always generate /-delimited filePaths
 		filePath = strings.ReplaceAll(filePath, string(os.PathSeparator), "/")
 		for _, analyzer := range a.analyzers() {
 			if err := analyzer.analyzeFile(filePath); err != nil {

diff --git a/pkg/skaffold/initializer/analyze/builder.go b/pkg/skaffold/initializer/analyze/builder.go
@@ -79,7 +79,8 @@ func (a *builderAnalyzer) detectBuilders(path string, detectJib bool) ([]build.I
 	if strings.Contains(strings.ToLower(base), "dockerfile") {
 		if docker.Validate(path) {
 			results = append(results, docker.ArtifactConfig{
-				File: path,
+				// Docker expects forward slashes (for Linux containers at least)
+				File: filepath.ToSlash(path),
 			})
 		}
 	}

diff --git a/pkg/skaffold/initializer/build/builders.go b/pkg/skaffold/initializer/build/builders.go
@@ -36,8 +36,9 @@ type InitBuilder interface {
 	// Must be unique between artifacts.
 	Describe() string
 
-	// ArtifactType returns the type of the artifact to be built.
-	ArtifactType() latest.ArtifactType
+	// ArtifactType returns the type of the artifact to be built.  Paths should be relative to the workspace.
+	// To make skaffold.yaml more portable across OS-es we should always generate /-delimited filepaths.
+	ArtifactType(workspace string) latest.ArtifactType
 
 	// ConfiguredImage returns the target image configured by the builder, or an empty string if no image is configured.
 	// This should be a cheap operation.

diff --git a/pkg/skaffold/initializer/build/resolve.go b/pkg/skaffold/initializer/build/resolve.go
@@ -79,7 +79,7 @@ func (d *defaultBuildInitializer) resolveBuilderImagesForcefully() error {
 }
 
 func builderRank(builder InitBuilder) int {
-	a := builder.ArtifactType()
+	a := builder.ArtifactType("")
 	switch {
 	case a.DockerArtifact != nil:
 		return 1

diff --git a/pkg/skaffold/initializer/build/util.go b/pkg/skaffold/initializer/build/util.go
@@ -65,17 +65,18 @@ func Artifacts(artifactInfos []ArtifactInfo) []*latest.Artifact {
 	var artifacts []*latest.Artifact
 
 	for _, info := range artifactInfos {
-		artifact := &latest.Artifact{
-			ImageName:    info.ImageName,
-			ArtifactType: info.Builder.ArtifactType(),
-		}
-
 		workspace := info.Workspace
 		if workspace == "" {
 			workspace = filepath.Dir(info.Builder.Path())
 		}
+		artifact := &latest.Artifact{
+			ImageName:    info.ImageName,
+			ArtifactType: info.Builder.ArtifactType(workspace),
+		}
+
 		if workspace != "." {
-			artifact.Workspace = workspace
+			// to make skaffold.yaml more portable across OS-es we should always generate /-delimited filepaths
+			artifact.Workspace = filepath.ToSlash(workspace)
 		}
 
 		artifacts = append(artifacts, artifact)

diff --git a/pkg/skaffold/initializer/init_test.go b/pkg/skaffold/initializer/init_test.go
@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -35,6 +36,7 @@ import (
 func TestDoInit(t *testing.T) {
 	tests := []struct {
 		name             string
+		shouldSkip       bool
 		dir              string
 		config           initconfig.Config
 		expectedError    string
@@ -89,6 +91,20 @@ func TestDoInit(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:       "windows paths use forward slashes",
+			shouldSkip: runtime.GOOS != "windows",
+			dir:        `testdata\init\windows`,
+			config: initconfig.Config{
+				Force: true,
+				CliArtifacts: []string{
+					`{"builder":"Docker","context":"apps\\web","payload":{"path":"apps\\web\\build\\Dockerfile"},"image":"gcr.io/k8s-skaffold/leeroy-web"}`,
+				},
+				Opts: config.SkaffoldOptions{
+					ConfigurationFile: "skaffold.yaml.out",
+				},
+			},
+		},
 		{
 			name: "CLI artifacts + manifest placeholders",
 			dir:  "testdata/init/allcli",
@@ -212,6 +228,10 @@ See https://skaffold.dev/docs/pipeline-stages/deployers/helm/ for a detailed gui
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.name, func(t *testutil.T) {
+			if test.shouldSkip {
+				t.Logf("Skipped test %q", test.name)
+				return
+			}
 			t.Chdir(test.dir)
 
 			err := DoInit(context.TODO(), os.Stdout, test.config)

diff --git a/pkg/skaffold/initializer/testdata/init/windows/apps/web/build/Dockerfile b/pkg/skaffold/initializer/testdata/init/windows/apps/web/build/Dockerfile
@@ -0,0 +1,7 @@
+FROM golang:1.12.9-alpine3.10 as builder
+COPY web.go .
+RUN go build -o /web .
+
+FROM alpine:3.10
+CMD ["./web"]
+COPY --from=builder /web .
diff --git a/pkg/skaffold/initializer/testdata/init/windows/apps/web/deployment.yaml b/pkg/skaffold/initializer/testdata/init/windows/apps/web/deployment.yaml
@@ -0,0 +1,37 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: leeroy-web
+  labels:
+    app: leeroy-web
+spec:
+  type: NodePort
+  ports:
+    - port: 8080
+      targetPort: 8080
+      protocol: TCP
+      name: leeroy-web
+  selector:
+    app: leeroy-web
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: leeroy-web
+  labels:
+    app: leeroy-web
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: leeroy-web
+  template:
+    metadata:
+      labels:
+        app: leeroy-web
+    spec:
+      containers:
+      - name: leeroy-web
+        image: gcr.io/k8s-skaffold/leeroy-web
+        ports:
+        - containerPort: 8080
diff --git a/pkg/skaffold/initializer/testdata/init/windows/apps/web/web.go b/pkg/skaffold/initializer/testdata/init/windows/apps/web/web.go
@@ -0,0 +1,25 @@
+package main
+
+import (
+	"io"
+	"net/http"
+
+	"log"
+)
+
+func handler(w http.ResponseWriter, r *http.Request) {
+	resp, err := http.Get("http://leeroy-app:50051")
+	if err != nil {
+		panic(err)
+	}
+	defer resp.Body.Close()
+	if _, err := io.Copy(w, resp.Body); err != nil {
+		panic(err)
+	}
+}
+
+func main() {
+	log.Print("leeroy web server ready")
+	http.HandleFunc("/", handler)
+	http.ListenAndServe(":8080", nil)
+}
diff --git a/pkg/skaffold/initializer/testdata/init/windows/skaffold.yaml b/pkg/skaffold/initializer/testdata/init/windows/skaffold.yaml
@@ -0,0 +1,14 @@
+apiVersion: skaffold/v2beta11
+kind: Config
+metadata:
+  name: windows
+build:
+  artifacts:
+  - image: gcr.io/k8s-skaffold/leeroy-web
+    context: apps/web
+    docker:
+      dockerfile: build/Dockerfile
+deploy:
+  kubectl:
+    manifests:
+    - apps/web/deployment.yaml