Add support for building Maven multimodule projects#1152
Add support for building Maven multimodule projects#1152dgageot merged 13 commits intoGoogleContainerTools:masterfrom
Conversation
briandealwis
requested review from
balopat,
dgageot,
nkubala and
r2d4
as code owners
|
PTAL @loosebazooka @coollog |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
We should probably also update getDependencies to be multimodule aware (use |
|
Oops! 🤦♂️ |
|
PTAL |
| return errors.Wrap(err, "could not obtain jib package goals") | ||
| } | ||
| // need to trim last newline | ||
| goals := strings.Split(strings.TrimSpace(string(stdout)), "\n") |
There was a problem hiding this comment.
also probably need to handle windows here?
|
lgtm: logic seems good to me. We should get skaffold team to look at go code and structure. |
|
PTAL @dgageot |
dgageot
left a comment
dgageot
left a comment
There was a problem hiding this comment.
A few nits but the logic seems good to me.
| result = append(result, line) | ||
| } | ||
| } | ||
| return result |
There was a problem hiding this comment.
You should handle scanner.Err() here
There was a problem hiding this comment.
Suggestions on what should happen? The input is a bag o' bytes rather than real I/O.
There was a problem hiding this comment.
Maybe just ignore my comment then.
nkubala
left a comment
nkubala
left a comment
There was a problem hiding this comment.
just a few small nits, but this looks good
pkg/skaffold/build/local/jib_test.go
Outdated
| shouldError bool | ||
| }{ | ||
| {"xxx", "", true}, // no goals should fail | ||
| {"xxx", "\n", true}, // no goals should faill; newline stripped |
pkg/skaffold/build/local/jib_test.go
Outdated
| 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"}) |
There was a problem hiding this comment.
any reason to use context.TODO() here instead of context.Background()?
|
PTAL: @dgageot |
6 participants
Adds support for building Maven multi-module projects. This implementation requires that the specified module must bind a jib goal to the
packagephase. This jib goal must correspond to thepushstate as defined in theskaffold.yamlfile.This PR also:
--non-recursiveto be on the safe side.util.NonEmptyLines()to extract non-empty lines from a byte array usingbufio.Scanner, used ingetDependencies()and this new functionality tooPart of #1096
Fixes #1159