-
Notifications
You must be signed in to change notification settings - Fork 35
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
jib-layer-filter-extension-maven: optionally create separate layers for dependencies from parent POM #76
Conversation
to support better error handling
injection of extensions shows how to use dependency injection in an extension
And formatting and logging.
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
=========================================
Coverage ? 84.89%
Complexity ? 154
=========================================
Files ? 14
Lines ? 629
Branches ? 71
=========================================
Hits ? 534
Misses ? 83
Partials ? 12 Continue to review full report at Codecov.
|
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.
Thank a lot for this! I did a quick scan on this and added comments about some technical stuff. I will continue to look into the PR.
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
@chanseokoh , thank you for your detailed review. I will walk through it in the very next days. |
...aven/src/main/java/com/google/cloud/tools/jib/maven/extension/layerfilter/Configuration.java
Outdated
Show resolved
Hide resolved
...aven/src/main/java/com/google/cloud/tools/jib/maven/extension/layerfilter/Configuration.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
still open for further consideration.
Reminder to myself: When resolving parent dependencies, active profiles have to be considered. ... Okay, this does already work, since the active profiles come form the |
to not depend on "app/libs", which is connfigurable in jib
use source file name for matching instead of filename from extraction path
Especially on the Artifact side that means that the filename is not "constructed" anymore by the extension. We make no assumption about the structure of the source file name or path. The only assumption that is left is, that Maven will resolve to the same file path, if the same artifact coordinates are resolved multiple times within a build / session.
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.
LGTM in general. Only some minor comments.
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...ain/java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtension.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
...java/com/google/cloud/tools/jib/maven/extension/layerfilter/JibLayerFilterExtensionTest.java
Outdated
Show resolved
Hide resolved
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.
LGTM. Thanks for this awesome contribution!
I will merge this later this week to give you some time for any last-minute changes, just in case.
That sounds great. Thank you for supporting this PR and helping to improve it. |
You're welcome. Just asking: are you interested in updating the "Writing Your Own Extensions" doc in a separate PR to add the new JSR350 / Sisu approach (while leaving the JDK service locator method for now)? |
Yes, I think I should do this. Will try in the next days. |
Thanks! |
…eating parent dependency layers in jib-layer-filter-extension-maven (#81) * Documentation for GoogleContainerTools/jib#3036 and #76 * Update README.md Co-authored-by: Chanseok Oh <chanseok@google.com>
I would like to achieve the following:
The idea is that the parent pom defines the "company / project standard" which rarely changes.
While the existing jib-layer-filter-extension-maven is already quite useful, it would be still quite cumbersome to write globs for all direct and transitive dependencies.
For example, think about having some Spring Boot starters in the parent pom for all your microservice, but some other Spring Boot starters are optional and only used by specific microservices. You not only would need quite exact globs for the Spring Boot starters then, but also for their manyfold transitive dependencies.
See also GoogleContainerTools/jib#3036 which is required for this pull request to work.