-
Notifications
You must be signed in to change notification settings - Fork 202
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1675 +/- ##
============================================
+ Coverage 33.49% 33.93% +0.43%
- Complexity 1097 1105 +8
============================================
Files 187 189 +2
Lines 10535 10704 +169
Branches 1724 1745 +21
============================================
+ Hits 3529 3632 +103
- Misses 6592 6650 +58
- Partials 414 422 +8 |
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'm sending fixes to these changes myself along with some other corrections.
core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildConfiguration.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildConfiguration.java
Outdated
Show resolved
Hide resolved
Hi @theexplorist ,
You need to run the above command on your local clone to add licence header on the new code files being added. Add a separate commit and push it. |
while reviewing, make sure you enable the isJib flag in pom. Also, make sure you also try building using Jib with docker daemon as inactive. |
If you want to get just tarImage then just add isJib flag to be true in xmlconfig and the tarImage will be found at target/docker, its use case is if creds are not configured or wrong creds are configured. If you want to see automatic registry push in action then you have to add a separate target Imagname in xmlconfig either under generator section to use the generator or under imageconfiguration. And the credentials must be configured as well. |
@theexplorist can you squash these commits into a single commit ? |
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.
This is my first round of review, I would look closely and try to run this and share more feedback whenever I get time. For now, please address these comments and add some documentation/tests.
@@ -116,7 +116,7 @@ public boolean isOpenShift(Logger log) { | |||
} catch (KubernetesClientException exp) { | |||
Throwable cause = exp.getCause(); | |||
String prefix = cause instanceof UnknownHostException ? "Unknown host " : ""; | |||
log.warn("Cannot access cluster for detecting mode: %s%s", | |||
log.warn("Cannot access OpenShift cluster for detecting mode: %s%s", |
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.
At this point, cluster can be either an OpenShift or kubernetes cluster. Why are you hardcoding Openshift?
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.
Oops I added by mistake. @theexplorist can you take care of this ?
/** | ||
* Enrichers used for enricher build objects | ||
* Enrichers used for enri @Parameter(property = "fabric8.to", defaultValue = "") |
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.
This doesn't look right, maybe you commented some parameter?
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.
No this is not needed , I will replace it!
@@ -78,6 +79,17 @@ | |||
</labels> | |||
</resources> | |||
|
|||
<generator> |
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.
why not add a separate sample for JIB demonstration?
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.
Maybe we can make a seperate execution profile for Jib demonstration. A seperate sample isn't really required imo. @theexplorist make note.
|
||
public class JibBuildService implements BuildService { | ||
|
||
// TODO ADD LOGGING |
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 you please resolve this TODO also? Maybe you forgot to resolve 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.
Logging is implemented just comment is needed to be removed.
core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildConfiguration.java
Outdated
Show resolved
Hide resolved
1cbc40c
to
6cf8bc9
Compare
534ce29
to
9b59441
Compare
Thorntail sample is crashing even for normal docker build, the container starts and crashes! https:/fabric8io/fabric8-maven-plugin/blob/master/samples/thorntail/pom.xml#L96 |
The JAVA_OPTION system property is carried out in the generated yaml fragments and then injected into the container when run as part of a kubernetes/openshift deployment. The main cause of crashes is the AB_OFF system property... which needs to be set to true incase if thorntail sample right now For reference, https:/fabric8io-images/java/blob/master/images/alpine/openjdk8/jdk/README.md |
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.
Apart from the comments, I faced problems while trying to build the samples provided in this PR. It only works when image name has your docker hub username attached to image section. Plus I'm still confused about why we are pushing image during the build goal.
<config> | ||
<spring-boot> | ||
<!-- Target Image name to which Jib Container will build to. Change ${image.user} with registry username --> | ||
<name>${image.user}/${project.artifactId}:${project.version}</name> |
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.
where is image.user
defined?
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.
image.user needs to be changed with the registry username in case you want to build a registry image
<plugin> | ||
<groupId>io.fabric8</groupId> | ||
<artifactId>fabric8-maven-plugin</artifactId> | ||
<version>4.2-SNAPSHOT</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.
update version to 4.3-SNAPSHOT, or use ${project.version}
import io.fabric8.maven.core.util.FatJarDetector; | ||
import io.fabric8.maven.docker.access.AuthConfig; | ||
import io.fabric8.maven.docker.config.BuildImageConfiguration; | ||
import io.fabric8.maven.docker.config.ImageConfiguration; |
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.
unused import
contBuild = contBuild.setEntrypoint(entrypointList); | ||
} | ||
|
||
if (credential != null) { |
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.
what happens when credential
is null? I see username and password are crucial dependencies for image generation.
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.
A tar image is generated locally
try { | ||
Containerizer containerizer = Containerizer.to(registryImage.addCredential(username, password)); | ||
JibContainer jibRegistryImageContainer = contBuild.containerize(containerizer); | ||
log.info("Image %s successfully built and pushed.", targetImage); |
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.
In my opinion, the default behavior should be generation of image locally. Why are we doing that as a fallback? Shouldn't image push step go in PushMojo
?
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.
Jib auto pushes the images to registry if the creds are verified! That's how it works! If registry is not verified a tar archive is generated and then you can use that archive for push goal!
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.
That's what a RegistryImage is meant to do in JIB, that's why they have this type of image!
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 like the idea of pushing image during build goal as it does not align with mojo's standard functionality. I would prefer finding some offline solution if possible.
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.
We have the offline solution, Jib supports three kinds
1.DockerDaemonImage - We don't need it as we are making it daemonless.
2.RegistryImage - It will do auto push that's Jib's functionality. That's why it is RegistryImage.
3.TarImage - This option is already added. Here you can use it for push goal.No need to configure creds etc here. You will get a tar archive.
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.
Does the user get to choose these build options? If not, we can provide some way to override the default behavior. The default behavior should be building tarball.
import java.util.List; | ||
import java.util.Map; | ||
|
||
public class JibBuildConfiguration { |
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'd love to see a JavaDoc for each of these classes explaining how they fit together for the benefit of future developers.
|
||
public static class Builder { | ||
private final JibBuildConfiguration configutil; | ||
private final Logger logger; |
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.
What's the purpose of having a logger field inside a builder?
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.
Its need to consumed by a static method of FatJarDetector https:/fabric8io/fabric8-maven-plugin/pull/1675/files/aa9ed76e932483ddeaaaa471698f99b20410237c#diff-15957aeb41cce2019190e8b80fb7180eR119
|
||
public Arguments getEntryPoint() {return entrypoint;} | ||
|
||
public String getTargetDir() {return targetDir;} |
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.
nit, spaces between braces and expressions. i.e { return targetDir; }
* implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
package io.fabric8.maven.core.service.kubernetes; |
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.
Shouldn't this class go to io.fabric8.maven.core.config
package?
* implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
package io.fabric8.maven.core.service.kubernetes; |
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.
Shouldn't this class go to io.fabric8.maven.core.util
package?
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.
Its not a generic utility. So I thought its okay for it to be here
When the system is offline, Jib build crashes while building image tarball. I think we're blocked on this GoogleContainerTools/jib#1881 right now. |
56df1ba
to
138a289
Compare
Update, Jib provides an offline mode as well. So our use case is sorted for now. |
73ea5c6
to
c7b87a7
Compare
log.warn("Registry Exception occured : %s", re.getMessage()); | ||
log.warn("Credentials are probably either not configured or are incorrect."); | ||
log.info("Building Image Tarball at %s.", imageTarName); | ||
JibContainer jibContainer = buildContainer(contBuild, tarImage, log, 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.
@dev-gaur : Can we make this a flag so that the user can also override in case he wants an offline build?
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.
Tested and seems to be working fine for me.
Nice work @theexplorist @dev-gaur !
eac7056
to
e73884d
Compare
This ports Jib support added in FMP in fabric8io/fabric8-maven-plugin#1675 with better logging.
This ports Jib support added in FMP in fabric8io/fabric8-maven-plugin#1675 with better logging.
This ports Jib support added in FMP in fabric8io/fabric8-maven-plugin#1675 with better logging.
This pull request shall port the following PRs: fabric8io/fabric8-maven-plugin#1675 fabric8io/fabric8-maven-plugin#1734 fabric8io/fabric8-maven-plugin#1739 fabric8io/fabric8-maven-plugin#1740
This pull request shall port the following PRs: fabric8io/fabric8-maven-plugin#1675 fabric8io/fabric8-maven-plugin#1734 fabric8io/fabric8-maven-plugin#1739 fabric8io/fabric8-maven-plugin#1740
This pull request shall port the following PRs: fabric8io/fabric8-maven-plugin#1675 fabric8io/fabric8-maven-plugin#1734 fabric8io/fabric8-maven-plugin#1739 fabric8io/fabric8-maven-plugin#1740
PR Summary
This PR is in referance to GSoC project 'Adding More Options In Fabric8-Maven-Plugin for building images".
PR Checklist