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.

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

Add support for building Maven multimodule projects #1152

Merged
merged 13 commits into from
Oct 19, 2018

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Oct 12, 2018

Adds support for building Maven multi-module projects. This implementation requires that the specified module must bind a jib goal to the package phase. This jib goal must correspond to the push state as defined in the skaffold.yaml file.

This PR also:

  • Changes the single-module build to use --non-recursive to be on the safe side.
  • Introduces util.NonEmptyLines() to extract non-empty lines from a byte array using bufio.Scanner, used in getDependencies() and this new functionality too

Part of #1096
Fixes #1159

@briandealwis
Copy link
Member Author

PTAL @loosebazooka @coollog

@codecov-io
Copy link

codecov-io commented Oct 12, 2018

Codecov Report

Merging #1152 into master will increase coverage by 0.27%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1152      +/-   ##
==========================================
+ Coverage   41.08%   41.35%   +0.27%     
==========================================
  Files          94       94              
  Lines        4126     4157      +31     
==========================================
+ Hits         1695     1719      +24     
- Misses       2262     2268       +6     
- Partials      169      170       +1
Impacted Files Coverage Δ
pkg/skaffold/jib/jib_maven.go 100% <100%> (+15.38%) ⬆️
pkg/skaffold/util/util.go 52.13% <100%> (+3.04%) ⬆️
pkg/skaffold/jib/jib.go 83.87% <100%> (-0.98%) ⬇️
pkg/skaffold/build/local/jib_maven.go 41.5% <70.37%> (+14.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a396f4...709fd4f. Read the comment docs.

@loosebazooka
Copy link
Member

loosebazooka commented Oct 12, 2018

We should probably also update getDependencies to be multimodule aware (use -N if module is not specified, -am if it is specified) in pkg/skaffold/jib/jib_maven.go

@briandealwis
Copy link
Member Author

Oops! 🤦‍♂️

@briandealwis
Copy link
Member Author

PTAL

@coollog coollog mentioned this pull request Oct 15, 2018
11 tasks
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?

@loosebazooka
Copy link
Member

lgtm: logic seems good to me. We should get skaffold team to look at go code and structure.

@briandealwis
Copy link
Member Author

PTAL @dgageot

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

A few nits but the logic seems good to me.

pkg/skaffold/util/util.go Outdated Show resolved Hide resolved
result = append(result, line)
}
}
return result
Copy link
Contributor

Choose a reason for hiding this comment

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

You should handle scanner.Err() here

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggestions on what should happen? The input is a bag o' bytes rather than real I/O.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just ignore my comment then.

pkg/skaffold/jib/jib_maven.go Show resolved Hide resolved
pkg/skaffold/build/local/jib_maven.go Outdated Show resolved Hide resolved
pkg/skaffold/build/local/jib_maven.go Outdated Show resolved Hide resolved
pkg/skaffold/build/local/jib_maven.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

just a few small nits, but this looks good

pkg/skaffold/build/local/jib_maven.go Outdated Show resolved Hide resolved
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)

for _, tt := range testCases {
util.DefaultExecCommand = testutil.NewFakeCmdOut("mvn --quiet --projects module jib:_skaffold-package-goals", 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()?

@briandealwis
Copy link
Member Author

PTAL: @dgageot

@dgageot dgageot merged commit 62b3327 into GoogleContainerTools:master Oct 19, 2018
@briandealwis briandealwis deleted the mmm branch August 30, 2019 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants