-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve Maven/Jib multimodule builds between Minikube and remote clusters #2122
Improve Maven/Jib multimodule builds between Minikube and remote clusters #2122
Conversation
PR will break since 1.3.0 is not released. |
To test: I bumped the |
The integration test is still failing Thanks |
Thanks for taking a look @tejal29 but the tests won't work until we release Jib 1.3.0. I'll update this PR when that happens. |
- move example changes to integration/examples - adopt new testutil changes in changes
Jib 1.3.0 has been released and this is now ready for review. |
pkg/skaffold/jib/jib_test.go
Outdated
{"1.2.0", true}, | ||
{"1.2.0-SNAPSHOT", true}, | ||
{"1.3.0", false}, | ||
{"1.3.0-SNAPSHOT", false}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will it work with our potential RCs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm no. Will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
integration/testdata/jib/pom.xml
Outdated
@@ -9,6 +9,7 @@ | |||
|
|||
<properties> | |||
<java.version>1.8</java.version> | |||
<jib.maven-plugin-version>1.3.0</jib.maven-plugin-version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this (and other instances of this) be defined in a parent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh jeepers where would we define it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. Maybe don't do this. Just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth doing in the integration tests but the examples should be standalone. I'll leave it for now: these values were hard-coded previously and making them properties makes testing easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, should we have a go
person go over it?
But also, maybe we should do a jib team experiment on a local build of this. Testing out all scenarios since we expect some breakages for existing users?
Jib 1.4.0 has been released with the new requiredVersion functionality. I added a test to verify that a build will fail when the Jib version is too old. PTAL. |
Do you think the Kokoro failure is related?
|
I wouldn't have thought so, but that test output is not helpful :-( |
pkg/skaffold/jib/jib_gradle.go
Outdated
@@ -27,6 +27,9 @@ import ( | |||
"github.com/sirupsen/logrus" | |||
) | |||
|
|||
// Skaffold-Jib depends on functionality introduced after Jib-Gradle 1.4.0 | |||
const MinimumJibGradleVersion = "(1.4.0,)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, shouldn't this be [1.4.0,)
(i.e., includes 1.4.0)? But tests are not failing??
pkg/skaffold/jib/jib_maven.go
Outdated
@@ -26,6 +26,9 @@ import ( | |||
"github.com/sirupsen/logrus" | |||
) | |||
|
|||
// Skaffold-Jib depends on functionality introduced after Jib-Maven 1.4.0 | |||
const MinimumJibMavenVersion = "(1.4.0,)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Thanks for the details look @chanseokoh — you caught a bunch of things. The GCB failure is related: the version range should be left-closed (:blush:). |
integration/build_test.go
Outdated
t.Skip("skipping test that is not gcp only") | ||
} | ||
|
||
err := skaffold.Build(test.args...).WithConfig(test.filename).InDir(test.dir).Run(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think .RunOrFail
does that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RunOrFail fails the test on error, whereas the error is a sign of success here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this test to check for expected error text as it would spuriously pass if (say) the wrong skaffold version was picked up ("Your Skaffold version might be too old").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, right!
Fixes #1876
Jib for Maven
1.3.01.4.0 will allow restricting Jib's image-building goals to a specific artifact. This ability means there is no additionalpackage
-binding step required for Skaffold-JibMaven projects. Maven/Jib multi-module project will need updating, and these changes are not compatible with previous versions of Jib.This PR reworks the Jib support to:
1.3.01.4.0)Projects using older versions of Jib will be prompted to update to Jib
1.3.01.4.0. For versions prior to 1.4.0, the build will fail due to a new goal introduced to check the required version:For later versions of Jib, when we bump this required version, they will see failures like:
Technically, only Maven multi-module projects require updating to 1.4.0, but for simplicity we're bumping the required version for Gradle too. If a user is willing to switch to a newer version of Skaffold, switching to a newer Jib should not present a problem either.