Skip to content
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

Jump to bottom

Add support for building Maven multimodule projects #1152

Merged
merged 13 commits into from
Oct 19, 2018
Merged

Add support for building Maven multimodule projects #1152

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
26d84c3
Add initial support for multimodule maven projects
briandealwis Oct 11, 2018
089bde8
Simplify and add tests
briandealwis Oct 12, 2018
c072783
rename
briandealwis Oct 12, 2018
a05469f
goimports
briandealwis Oct 12, 2018
a9c1a29
Use util.SkipWrapperCheck
briandealwis Oct 12, 2018
bd8d7ea
Update getDependencies() and expand maven options
briandealwis Oct 12, 2018
35d8c71
Handle Windows \r\n
briandealwis Oct 15, 2018
5406d7b
forgot to fix util test
briandealwis Oct 15, 2018
a430c70
goimports
briandealwis Oct 15, 2018
7fe792b
Merge remote-tracking branch 'origin/master' into jib
briandealwis Oct 15, 2018
44e66dc
remove unnecessary code
briandealwis Oct 15, 2018
6e295be
Address review comments and tweak command-line argument order
briandealwis Oct 16, 2018
709fd4f
review nits
briandealwis Oct 17, 2018
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
48 changes: 44 additions & 4 deletions pkg/skaffold/build/local/jib_maven.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package local
import (
"context"
"io"
"strings"

"fmt"

Expand All @@ -30,8 +31,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 != "" {
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand All @@ -45,8 +49,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()
Expand All @@ -62,14 +69,47 @@ 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}
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
} else {
// multi-module project: we assume `package` is boujd to `jib:<goal>`
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
command = []string{"--projects", a.Module, "--also-make", "package", "-Dimage=" + skaffoldImage}
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
}
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")
}
// need to trim last newline
goals := strings.Split(strings.TrimSpace(string(stdout)), "\n")
Copy link
Member

Choose a reason for hiding this comment

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

also probably need to handle windows here?

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 {
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
return errors.New(fmt.Sprintf("skaffold `push` setting requires 'package' be bound to 'jib:%s'", requiredGoal))
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
}
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
Expand Down
39 changes: 30 additions & 9 deletions pkg/skaffold/build/local/jib_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo (s/faill/fail)

{"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"})
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to use context.TODO() here instead of context.Background()?

if hasError := err != nil; tt.shouldError != hasError {
t.Error("Unexpected return result")
}
}

testutil.CheckError(t, true, err)
}

func TestGenerateGradleArgs(t *testing.T) {
Expand Down
14 changes: 8 additions & 6 deletions pkg/skaffold/jib/jib_maven.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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"}
}
briandealwis marked this conversation as resolved.
Show resolved Hide resolved
if a.Profile != "" {
args = append(args, "-P", a.Profile)
args = append(args, "--activate-profiles", a.Profile)
}

return MavenCommand.CreateCommand(ctx, workspace, args)
Expand Down
18 changes: 13 additions & 5 deletions pkg/skaffold/jib/jib_maven_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"})
},
},
}
Expand Down