Simplify Jib code

Signed-off-by: David Gageot <[email protected]>

pkg/skaffold/build/local/jib_gradle.go

@@ -35,25 +35,26 @@ func (b *Builder) buildJibGradle(ctx context.Context, out io.Writer, workspace s
 	cmd := jib.GradleCommand.CreateCommand(ctx, workspace, commandLine)
 	cmd.Stdout = out
 	cmd.Stderr = out
+
 	logrus.Infof("Building %s: %s, %v", workspace, cmd.Path, cmd.Args)
-	err := util.RunCmd(cmd)
-	if err != nil {
+	if err := util.RunCmd(cmd); err != nil {
 		return "", errors.Wrap(err, "gradle build failed")
 	}
+
 	return skaffoldImage, nil
 }
 
 // generateGradleCommand generates the command-line to pass to gradle for building an
 // project in `workspace`.  The resulting image is added to the local docker daemon
 // and called `skaffoldImage`.
 func generateGradleCommand(_ /*workspace*/ string, skaffoldImage string, a *latest.JibGradleArtifact) []string {
-	var command []string
+	var command string
 	if a.Project == "" {
-		command = []string{":jibDockerBuild"}
+		command = ":jibDockerBuild"
 	} else {
 		// multi-module
-		command = []string{fmt.Sprintf(":%s:jibDockerBuild", a.Project)}
+		command = fmt.Sprintf(":%s:jibDockerBuild", a.Project)
 	}
-	command = append(command, "--image="+skaffoldImage)
-	return command
+
+	return []string{command, "--image=" + skaffoldImage}
 }

pkg/skaffold/build/local/jib_maven.go

@@ -28,34 +28,33 @@ import (
 )
 
 func (b *Builder) buildJibMaven(ctx context.Context, out io.Writer, workspace string, a *latest.JibMavenArtifact) (string, error) {
-	skaffoldImage := generateJibImageRef(workspace, a.Module)
-	commandLine, err := generateMavenCommand(workspace, skaffoldImage, a)
-	if err != nil {
-		return "", err
+	if a.Module != "" {
+		return "", errors.New("maven multi-modules not supported yet")
 	}
 
+	skaffoldImage := generateJibImageRef(workspace, a.Module)
+	commandLine := generateMavenCommand(workspace, skaffoldImage, a)
+
 	cmd := jib.MavenCommand.CreateCommand(ctx, workspace, commandLine)
 	cmd.Stdout = out
 	cmd.Stderr = out
+
 	logrus.Infof("Building %s: %s, %v", workspace, cmd.Path, cmd.Args)
-	err = util.RunCmd(cmd)
-	if err != nil {
+	if err := util.RunCmd(cmd); err != nil {
 		return "", errors.Wrap(err, "maven build failed")
 	}
+
 	return skaffoldImage, nil
 }
 
 // generateMavenCommand generates the command-line to pass to maven for building a
 // project found in `workspace`.  The resulting image is added to the local docker daemon
 // and called `skaffoldImage`.
-func generateMavenCommand(_ /*workspace*/ string, skaffoldImage string, a *latest.JibMavenArtifact) ([]string, error) {
-	if a.Module != "" {
-		// TODO: multi-module
-		return nil, errors.New("Maven multi-modules not supported yet")
-	}
-	commandLine := []string{"prepare-package", "jib:dockerBuild", "-Dimage=" + skaffoldImage}
+func generateMavenCommand(_ /*workspace*/ string, skaffoldImage string, a *latest.JibMavenArtifact) []string {
+	command := []string{"prepare-package", "jib:dockerBuild", "-Dimage=" + skaffoldImage}
 	if a.Profile != "" {
-		commandLine = append(commandLine, "-P"+a.Profile)
+		command = append(command, "-P"+a.Profile)
 	}
-	return commandLine, nil
+
+	return command
 }

pkg/skaffold/build/local/jib_test.go

@@ -17,6 +17,8 @@ limitations under the License.
 package local
 
 import (
+	"context"
+	"io/ioutil"
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
@@ -33,16 +35,18 @@ func TestGenerateMavenCommand(t *testing.T) {
 	}
 
 	for _, tt := range testCases {
-		commandLine, err := generateMavenCommand(".", "image", &tt.in)
+		command := generateMavenCommand(".", "image", &tt.in)
 
-		testutil.CheckError(t, false, err)
-		testutil.CheckDeepEqual(t, tt.out, commandLine)
+		testutil.CheckDeepEqual(t, tt.out, command)
 	}
 }
 
-func TestGenerateMavenCommand_errorWithModule(t *testing.T) {
-	a := latest.JibMavenArtifact{Module: "module"}
-	_, err := generateMavenCommand(".", "image", &a)
+func TestMultiModulesNotSupported(t *testing.T) {
+	builder := &Builder{}
+
+	_, err := builder.buildJibMaven(context.Background(), ioutil.Discard, ".", &latest.JibMavenArtifact{
+		Module: "module",
+	})
 
 	testutil.CheckError(t, true, err)
 }
@@ -57,9 +61,9 @@ func TestGenerateGradleCommand(t *testing.T) {
 	}
 
 	for _, tt := range testCases {
-		commandLine := generateGradleCommand(".", "image", &tt.in)
+		command := generateGradleCommand(".", "image", &tt.in)
 
-		testutil.CheckDeepEqual(t, tt.out, commandLine)
+		testutil.CheckDeepEqual(t, tt.out, command)
 	}
 }
 
@@ -77,8 +81,7 @@ func TestGenerateJibImageRef(t *testing.T) {
 
 	for _, tt := range testCases {
 		computed := generateJibImageRef(tt.workspace, tt.project)
-		if tt.out != computed {
-			t.Errorf("Expected '%s' for '%s'/'%s': '%s'", tt.out, tt.workspace, tt.project, computed)
-		}
+
+		testutil.CheckDeepEqual(t, tt.out, computed)
 	}
 }

pkg/skaffold/jib/jib.go

@@ -18,6 +18,7 @@ package jib
 
 import (
 	"os/exec"
+	"sort"
 	"strings"
 
 	"os"
@@ -36,6 +37,7 @@ func getDependencies(cmd *exec.Cmd) ([]string, error) {
 
 	// Parses stdout for the dependencies, one per line
 	lines := strings.Split(string(stdout), "\n")
+
 	var deps []string
 	for _, dep := range lines {
 		if dep == "" {
@@ -57,22 +59,23 @@ func getDependencies(cmd *exec.Cmd) ([]string, error) {
 			}
 			return nil, errors.Wrapf(err, "unable to stat file %s", dep)
 		}
-		if info.IsDir() {
-			err := godirwalk.Walk(dep, &godirwalk.Options{
-				Unsorted: true,
-				Callback: func(path string, _ *godirwalk.Dirent) error {
-					deps = append(deps, path)
-					return nil
-				},
-			})
 
-			if err != nil {
-				return nil, errors.Wrap(err, "filepath walk")
-			}
+		if !info.IsDir() {
+			deps = append(deps, dep)
 			continue
 		}
 
-		deps = append(deps, dep)
+		if err = godirwalk.Walk(dep, &godirwalk.Options{
+			Unsorted: true,
+			Callback: func(path string, _ *godirwalk.Dirent) error {
+				deps = append(deps, path)
+				return nil
+			},
+		}); err != nil {
+			return nil, errors.Wrap(err, "filepath walk")
+		}
 	}
+
+	sort.Strings(deps)
 	return deps, nil
 }

pkg/skaffold/jib/jib_gradle_test.go

@@ -18,26 +18,20 @@ package jib
 
 import (
 	"context"
+	"fmt"
 	"os/exec"
 	"strings"
 	"testing"
 
-	"fmt"
-	"path/filepath"
-
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 	"github.com/pkg/errors"
 )
 
 func TestGradleWrapperDefinition(t *testing.T) {
-	if GradleCommand.Executable != "gradle" {
-		t.Error("GradleCommand executable should be 'gradle'")
-	}
-	if GradleCommand.Wrapper != "gradlew" {
-		t.Error("GradleCommand wrapper should be 'gradlew'")
-	}
+	testutil.CheckDeepEqual(t, "gradle", GradleCommand.Executable)
+	testutil.CheckDeepEqual(t, "gradlew", GradleCommand.Wrapper)
 }
 
 func TestGetDependenciesGradle(t *testing.T) {
@@ -47,20 +41,21 @@ func TestGetDependenciesGradle(t *testing.T) {
 	tmpDir.Write("dep1", "")
 	tmpDir.Write("dep2", "")
 
-	dep1 := filepath.Join(tmpDir.Root(), "dep1")
-	dep2 := filepath.Join(tmpDir.Root(), "dep2")
+	dep1 := tmpDir.Path("dep1")
+	dep2 := tmpDir.Path("dep2")
 
-	ctx := context.TODO()
+	ctx := context.Background()
 
 	var tests = []struct {
 		description string
 		stdout      string
+		expected    []string
 		err         error
 	}{
 		{
 			description: "success",
 			stdout:      fmt.Sprintf("%s\n%s\n\n\n", dep1, dep2),
-			err:         nil,
+			expected:    []string{dep1, dep2},
 		},
 		{
 			description: "failure",
@@ -71,9 +66,6 @@ func TestGetDependenciesGradle(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			tmpDir, cleanup := testutil.NewTempDir(t)
-			defer cleanup()
-
 			defer func(c util.Command) { util.DefaultExecCommand = c }(util.DefaultExecCommand)
 			util.DefaultExecCommand = testutil.NewFakeCmdOut(
 				strings.Join(getCommandGradle(ctx, tmpDir.Root(), &latest.JibGradleArtifact{}).Args, " "),
@@ -92,7 +84,7 @@ func TestGetDependenciesGradle(t *testing.T) {
 }
 
 func TestGetCommandGradle(t *testing.T) {
-	ctx := context.TODO()
+	ctx := context.Background()
 
 	var tests = []struct {
 		description       string

pkg/skaffold/jib/jib_maven_test.go

@@ -18,26 +18,20 @@ package jib
 
 import (
 	"context"
+	"fmt"
 	"os/exec"
 	"strings"
 	"testing"
 
-	"fmt"
-	"path/filepath"
-
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 	"github.com/pkg/errors"
 )
 
 func TestMavenWrapperDefinition(t *testing.T) {
-	if MavenCommand.Executable != "mvn" {
-		t.Error("GradleCommand executable should be 'mvn'")
-	}
-	if MavenCommand.Wrapper != "mvnw" {
-		t.Error("MavenCommand wrapper should be 'mvnw'")
-	}
+	testutil.CheckDeepEqual(t, "mvn", MavenCommand.Executable)
+	testutil.CheckDeepEqual(t, "mvnw", MavenCommand.Wrapper)
 }
 
 func TestGetDependenciesMaven(t *testing.T) {
@@ -47,20 +41,21 @@ func TestGetDependenciesMaven(t *testing.T) {
 	tmpDir.Write("dep1", "")
 	tmpDir.Write("dep2", "")
 
-	dep1 := filepath.Join(tmpDir.Root(), "dep1")
-	dep2 := filepath.Join(tmpDir.Root(), "dep2")
+	dep1 := tmpDir.Path("dep1")
+	dep2 := tmpDir.Path("dep2")
 
-	ctx := context.TODO()
+	ctx := context.Background()
 
 	var tests = []struct {
 		description string
 		stdout      string
+		expected    []string
 		err         error
 	}{
 		{
 			description: "success",
 			stdout:      fmt.Sprintf("%s\n%s\n\n\n", dep1, dep2),
-			err:         nil,
+			expected:    []string{dep1, dep2},
 		},
 		{
 			description: "failure",
@@ -71,9 +66,6 @@ func TestGetDependenciesMaven(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			tmpDir, cleanup := testutil.NewTempDir(t)
-			defer cleanup()
-
 			defer func(c util.Command) { util.DefaultExecCommand = c }(util.DefaultExecCommand)
 			util.DefaultExecCommand = testutil.NewFakeCmdOut(
 				strings.Join(getCommandMaven(ctx, tmpDir.Root(), &latest.JibMavenArtifact{}).Args, " "),
@@ -85,14 +77,14 @@ func TestGetDependenciesMaven(t *testing.T) {
 			if test.err != nil {
 				testutil.CheckErrorAndDeepEqual(t, true, err, "getting jibMaven dependencies: "+test.err.Error(), err.Error())
 			} else {
-				testutil.CheckDeepEqual(t, []string{dep1, dep2}, deps)
+				testutil.CheckDeepEqual(t, test.expected, deps)
 			}
 		})
 	}
 }
 
 func TestGetCommandMaven(t *testing.T) {
-	ctx := context.TODO()
+	ctx := context.Background()
 	var tests = []struct {
 		description      string
 		jibMavenArtifact latest.JibMavenArtifact

pkg/skaffold/jib/jib_test.go

@@ -17,14 +17,10 @@ limitations under the License.
 package jib
 
 import (
+	"fmt"
 	"os/exec"
 	"testing"
 
-	"fmt"
-	"path/filepath"
-
-	"sort"
-
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
@@ -38,13 +34,13 @@ func TestGetDependencies(t *testing.T) {
 	tmpDir.Write("dep3/fileA", "")
 	tmpDir.Write("dep3/sub/path/fileB", "")
 
-	dep1 := filepath.Join(tmpDir.Root(), "dep1")
-	dep2 := filepath.Join(tmpDir.Root(), "dep2")
-	dep3 := filepath.Join(tmpDir.Root(), "dep3")
-	dep3FileA := filepath.Join(tmpDir.Root(), "dep3/fileA")
-	dep3Sub := filepath.Join(tmpDir.Root(), "dep3/sub")
-	dep3SubPath := filepath.Join(tmpDir.Root(), "dep3/sub/path")
-	dep3SubPathFileB := filepath.Join(tmpDir.Root(), "dep3/sub/path/fileB")
+	dep1 := tmpDir.Path("dep1")
+	dep2 := tmpDir.Path("dep2")
+	dep3 := tmpDir.Path("dep3")
+	dep3FileA := tmpDir.Path("dep3/fileA")
+	dep3Sub := tmpDir.Path("dep3/sub")
+	dep3SubPath := tmpDir.Path("dep3/sub/path")
+	dep3SubPathFileB := tmpDir.Path("dep3/sub/path/fileB")
 
 	var tests = []struct {
 		stdout       string
@@ -98,7 +94,7 @@ func TestGetDependencies(t *testing.T) {
 			)
 
 			deps, err := getDependencies(&exec.Cmd{Args: []string{"ignored"}, Dir: tmpDir.Root()})
-			sort.Strings(deps)
+
 			testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedDeps, deps)
 		})
 	}