Add support for building Maven multimodule projects

pkg/skaffold/build/local/jib_maven.go

@@ -30,8 +30,11 @@ import (
 )
 
 func (b *Builder) buildJibMavenToDocker(ctx context.Context, out io.Writer, workspace string, a *latest.JibMavenArtifact) (string, error) {
+	// If this is a multi-module project, we require `package` be bound to jib:dockerBuild
 	if a.Module != "" {
-		return "", errors.New("maven multi-modules not supported yet")
+		if err := verifyJibPackageGoal(ctx, "dockerBuild", workspace, a); err != nil {
+			return "", err
+		}
 	}
 
 	skaffoldImage := generateJibImageRef(workspace, a.Module)
@@ -45,8 +48,11 @@ func (b *Builder) buildJibMavenToDocker(ctx context.Context, out io.Writer, work
 }
 
 func (b *Builder) buildJibMavenToRegistry(ctx context.Context, out io.Writer, workspace string, artifact *latest.Artifact) (string, error) {
+	// If this is a multi-module project, we require `package` be bound to jib:build
 	if artifact.JibMavenArtifact.Module != "" {
-		return "", errors.New("maven multi-modules not supported yet")
+		if err := verifyJibPackageGoal(ctx, "build", workspace, artifact.JibMavenArtifact); err != nil {
+			return "", err
+		}
 	}
 
 	initialTag := util.RandomID()
@@ -62,14 +68,46 @@ func (b *Builder) buildJibMavenToRegistry(ctx context.Context, out io.Writer, wo
 
 // generateMavenArgs generates the arguments to Maven for building the project as an image called `skaffoldImage`.
 func generateMavenArgs(goal string, skaffoldImage string, a *latest.JibMavenArtifact) []string {
-	command := []string{"prepare-package", "jib:" + goal, "-Dimage=" + skaffoldImage}
+	var command []string
+	if a.Module == "" {
+		// single-module project
+		command = []string{"--non-recursive", "prepare-package", "jib:" + goal, "-Dimage=" + skaffoldImage}
+	} else {
+		// multi-module project: we assume `package` is boujd to `jib:<goal>`
+		command = []string{"--projects", a.Module, "--also-make", "package", "-Dimage=" + skaffoldImage}
+	}
 	if a.Profile != "" {
-		command = append(command, "-P"+a.Profile)
+		command = append(command, "--activate-profiles", a.Profile)
 	}
 
 	return command
 }
 
+// verifyJibPackageGoal verifies that the referenced module has `package` bound to a single jib goal.
+// It returns `nil` if the goal is matched, and an error if there is a mismatch.
+func verifyJibPackageGoal(ctx context.Context, requiredGoal string, workspace string, a *latest.JibMavenArtifact) error {
+	// cannot use --non-recursive
+	command := []string{"--projects", a.Module, "jib:_skaffold-package-goals", "--quiet"}
+	if a.Profile != "" {
+		command = append(command, "--activate-profiles", a.Profile)
+	}
+
+	cmd := jib.MavenCommand.CreateCommand(ctx, workspace, command)
+	logrus.Debugf("Looking for jib bound package goals for %s: %s, %v", workspace, cmd.Path, cmd.Args)
+	stdout, err := util.RunCmdOut(cmd)
+	if err != nil {
+		return errors.Wrap(err, "could not obtain jib package goals")
+	}
+	goals := util.NonEmptyLines(stdout)
+	logrus.Debugf("jib bound package goals for %s %s: %v (%d)", workspace, a.Module, goals, len(goals))
+	if len(goals) != 1 {
+		return errors.New("skaffold requires a single jib goal bound to 'package'")
+	} else if goals[0] != requiredGoal {
+		return errors.New(fmt.Sprintf("skaffold `push` setting requires 'package' be bound to 'jib:%s'", requiredGoal))
+	}
+	return nil
+}
+
 func runMavenCommand(ctx context.Context, out io.Writer, workspace string, args []string) error {
 	cmd := jib.MavenCommand.CreateCommand(ctx, workspace, args)
 	cmd.Stdout = out

pkg/skaffold/build/local/jib_test.go

@@ -18,10 +18,10 @@ package local
 
 import (
 	"context"
-	"io/ioutil"
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -30,8 +30,10 @@ func TestGenerateMavenArgs(t *testing.T) {
 		in  latest.JibMavenArtifact
 		out []string
 	}{
-		{latest.JibMavenArtifact{}, []string{"prepare-package", "jib:goal", "-Dimage=image"}},
-		{latest.JibMavenArtifact{Profile: "profile"}, []string{"prepare-package", "jib:goal", "-Dimage=image", "-Pprofile"}},
+		{latest.JibMavenArtifact{}, []string{"--non-recursive", "prepare-package", "jib:goal", "-Dimage=image"}},
+		{latest.JibMavenArtifact{Profile: "profile"}, []string{"--non-recursive", "prepare-package", "jib:goal", "-Dimage=image", "--activate-profiles", "profile"}},
+		{latest.JibMavenArtifact{Module: "module"}, []string{"--projects", "module", "--also-make", "package", "-Dimage=image"}},
+		{latest.JibMavenArtifact{Module: "module", Profile: "profile"}, []string{"--projects", "module", "--also-make", "package", "-Dimage=image", "--activate-profiles", "profile"}},
 	}
 
 	for _, tt := range testCases {
@@ -41,14 +43,33 @@ func TestGenerateMavenArgs(t *testing.T) {
 	}
 }
 
-func TestMultiModulesNotSupported(t *testing.T) {
-	builder := &Builder{}
+func TestMavenVerifyJibPackageGoal(t *testing.T) {
+	var testCases = []struct {
+		requiredGoal string
+		mavenOutput  string
+		shouldError  bool
+	}{
+		{"xxx", "", true},   // no goals should fail
+		{"xxx", "\n", true}, // no goals should faill; newline stripped
+		{"dockerBuild", "dockerBuild", false},
+		{"dockerBuild", "dockerBuild\n", false}, // newline stripped
+		{"dockerBuild", "build\n", true},
+		{"dockerBuild", "build\ndockerBuild\n", true},
+	}
+
+	defer func(c util.Command) { util.DefaultExecCommand = c }(util.DefaultExecCommand)
+	defer func(previous bool) { util.SkipWrapperCheck = previous }(util.SkipWrapperCheck)
+	util.SkipWrapperCheck = true
 
-	_, err := builder.buildJibMavenToDocker(context.Background(), ioutil.Discard, ".", &latest.JibMavenArtifact{
-		Module: "module",
-	})
+	for _, tt := range testCases {
+		util.DefaultExecCommand = testutil.NewFakeCmdOut("mvn --projects module jib:_skaffold-package-goals --quiet", tt.mavenOutput, nil)
+
+		err := verifyJibPackageGoal(context.TODO(), tt.requiredGoal, ".", &latest.JibMavenArtifact{Module: "module"})
+		if hasError := err != nil; tt.shouldError != hasError {
+			t.Error("Unexpected return result")
+		}
+	}
 
-	testutil.CheckError(t, true, err)
 }
 
 func TestGenerateGradleArgs(t *testing.T) {

pkg/skaffold/jib/jib.go

@@ -19,7 +19,6 @@ package jib
 import (
 	"os/exec"
 	"sort"
-	"strings"
 
 	"os"
 
@@ -36,7 +35,7 @@ func getDependencies(cmd *exec.Cmd) ([]string, error) {
 	}
 
 	// Parses stdout for the dependencies, one per line
-	lines := strings.Split(string(stdout), "\n")
+	lines := util.NonEmptyLines(stdout)
 
 	var deps []string
 	for _, dep := range lines {

pkg/skaffold/jib/jib_maven.go

@@ -31,10 +31,6 @@ var MavenCommand = util.CommandWrapper{Executable: "mvn", Wrapper: "mvnw"}
 // GetDependenciesMaven finds the source dependencies for the given jib-maven artifact.
 // All paths are absolute.
 func GetDependenciesMaven(ctx context.Context, workspace string, a *latest.JibMavenArtifact) ([]string, error) {
-	if a.Module != "" {
-		// TODO(coollog): Add support for multi-module projects.
-		return nil, errors.New("Maven multi-modules not supported yet")
-	}
 	deps, err := getDependencies(getCommandMaven(ctx, workspace, a))
 	if err != nil {
 		return nil, errors.Wrapf(err, "getting jibMaven dependencies")
@@ -44,9 +40,15 @@ func GetDependenciesMaven(ctx context.Context, workspace string, a *latest.JibMa
 }
 
 func getCommandMaven(ctx context.Context, workspace string, a *latest.JibMavenArtifact) *exec.Cmd {
-	args := []string{"jib:_skaffold-files", "-q"}
+	var args []string
+	if a.Module == "" {
+		// single-module project
+		args = []string{"--non-recursive", "jib:_skaffold-files", "--quiet"}
+	} else {
+		args = []string{"--projects", a.Module, "--also-make", "jib:_skaffold-files", "--quiet"}
+	}
 	if a.Profile != "" {
-		args = append(args, "-P", a.Profile)
+		args = append(args, "--activate-profiles", a.Profile)
 	}
 
 	return MavenCommand.CreateCommand(ctx, workspace, args)

pkg/skaffold/jib/jib_maven_test.go

@@ -96,39 +96,47 @@ func TestGetCommandMaven(t *testing.T) {
 			jibMavenArtifact: latest.JibMavenArtifact{},
 			filesInWorkspace: []string{},
 			expectedCmd: func(workspace string) *exec.Cmd {
-				return MavenCommand.CreateCommand(ctx, workspace, []string{"jib:_skaffold-files", "-q"})
+				return MavenCommand.CreateCommand(ctx, workspace, []string{"--non-recursive", "jib:_skaffold-files", "--quiet"})
 			},
 		},
 		{
 			description:      "maven with profile",
 			jibMavenArtifact: latest.JibMavenArtifact{Profile: "profile"},
 			filesInWorkspace: []string{},
 			expectedCmd: func(workspace string) *exec.Cmd {
-				return MavenCommand.CreateCommand(ctx, workspace, []string{"jib:_skaffold-files", "-q", "-P", "profile"})
+				return MavenCommand.CreateCommand(ctx, workspace, []string{"--non-recursive", "jib:_skaffold-files", "--quiet", "--activate-profiles", "profile"})
 			},
 		},
 		{
 			description:      "maven with wrapper no profile",
 			jibMavenArtifact: latest.JibMavenArtifact{},
 			filesInWorkspace: []string{"mvnw", "mvnw.bat"},
 			expectedCmd: func(workspace string) *exec.Cmd {
-				return MavenCommand.CreateCommand(ctx, workspace, []string{"jib:_skaffold-files", "-q"})
+				return MavenCommand.CreateCommand(ctx, workspace, []string{"--non-recursive", "jib:_skaffold-files", "--quiet"})
 			},
 		},
 		{
 			description:      "maven with wrapper no profile",
 			jibMavenArtifact: latest.JibMavenArtifact{},
 			filesInWorkspace: []string{"mvnw", "mvnw.cmd"},
 			expectedCmd: func(workspace string) *exec.Cmd {
-				return MavenCommand.CreateCommand(ctx, workspace, []string{"jib:_skaffold-files", "-q"})
+				return MavenCommand.CreateCommand(ctx, workspace, []string{"--non-recursive", "jib:_skaffold-files", "--quiet"})
 			},
 		},
 		{
 			description:      "maven with wrapper and profile",
 			jibMavenArtifact: latest.JibMavenArtifact{Profile: "profile"},
 			filesInWorkspace: []string{"mvnw", "mvnw.bat"},
 			expectedCmd: func(workspace string) *exec.Cmd {
-				return MavenCommand.CreateCommand(ctx, workspace, []string{"jib:_skaffold-files", "-q", "-P", "profile"})
+				return MavenCommand.CreateCommand(ctx, workspace, []string{"--non-recursive", "jib:_skaffold-files", "--quiet", "--activate-profiles", "profile"})
+			},
+		},
+		{
+			description:      "maven with multi-modules",
+			jibMavenArtifact: latest.JibMavenArtifact{Module: "module"},
+			filesInWorkspace: []string{"mvnw", "mvnw.bat"},
+			expectedCmd: func(workspace string) *exec.Cmd {
+				return MavenCommand.CreateCommand(ctx, workspace, []string{"--projects", "module", "--also-make", "jib:_skaffold-files", "--quiet"})
 			},
 		},
 	}

pkg/skaffold/util/util.go

@@ -17,6 +17,8 @@ limitations under the License.
 package util
 
 import (
+	"bufio"
+	"bytes"
 	"crypto/rand"
 	"fmt"
 	"io/ioutil"
@@ -227,3 +229,15 @@ func AbsFile(workspace string, filename string) (string, error) {
 	}
 	return filepath.Abs(file)
 }
+
+// NonEmptyLines scans the provided input and returns the non-empty strings found as an array
+func NonEmptyLines(input []byte) []string {
+	var result = []string{}
+	scanner := bufio.NewScanner(bytes.NewReader(input))
+	for scanner.Scan() {
+		if line := scanner.Text(); len(line) > 0 {
+			result = append(result, line)
+		}
+	}
+	return result
+}

pkg/skaffold/util/util_test.go

@@ -193,3 +193,23 @@ func TestAbsFile(t *testing.T) {
 	_, err = AbsFile(tmpDir.Root(), "does-not-exist")
 	testutil.CheckError(t, true, err)
 }
+
+func TestNonEmptyLines(t *testing.T) {
+	var testCases = []struct {
+		in  string
+		out []string
+	}{
+		{"", []string{}},
+		{"a\n", []string{"a"}},
+		{"a\r\n", []string{"a"}},
+		{"a\r\nb", []string{"a", "b"}},
+		{"a\r\nb\n\n", []string{"a", "b"}},
+		{"\na\r\n\n\n", []string{"a"}},
+	}
+	for _, tt := range testCases {
+		t.Run(tt.in, func(t *testing.T) {
+			result := NonEmptyLines([]byte(tt.in))
+			testutil.CheckDeepEqual(t, tt.out, result)
+		})
+	}
+}