Jib refactor #1766

devang-gaur · 2019-12-03T11:20:02Z

blocked on fabric8io/docker-maven-plugin#1309

Edit: Not blocked anymore.

devang-gaur · 2019-12-08T18:02:42Z

fixes #1763 , #1764 , #1738

CHANGELOG.md

core/src/main/java/io/fabric8/maven/core/service/BuildService.java

core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildService.java

manusa · 2020-01-14T11:32:40Z

fabric8-maven-plugin/core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildService.java

Line 36 in acc895c

private JibBuildService() { }

This constructor is redundant, please remove (public constructor with parameters automatically removes the possibility of a public no-args constructor for the class)

manusa · 2020-01-14T12:00:06Z

fabric8-maven-plugin/core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildService.java

Lines 49 to 51 in acc895c

    
           } catch (Exception ex) { 
        
               throw new UnsupportedOperationException(ex); 
        
           }

UnsupportedOperationExceptionis too generic, use at least Fabric8ServiceException like the rest of implementing classes.

core/src/main/java/io/fabric8/maven/core/util/JibServiceUtil.java

devang-gaur · 2020-01-30T02:20:43Z

fixes #1765 , #1757

generator/api/src/main/java/io/fabric8/maven/generator/api/support/BaseGenerator.java

rohanKanojia

Looks good overall with few minor comments.

Also, it would be nice if we could add some more documentation about this feature's usage.

CHANGELOG.md

core/src/main/java/io/fabric8/maven/core/service/Fabric8ServiceHub.java

core/src/main/java/io/fabric8/maven/core/service/kubernetes/jib/JibServiceUtil.java

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/PushMojo.java

samples/spring-boot/pom.xml

rohanKanojia · 2020-02-20T13:38:46Z

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/PushMojo.java

+    private void jibPush(ImageConfiguration imageConfiguration) throws MojoExecutionException {
+        BuildImageConfiguration buildImageConfiguration = imageConfiguration.getBuildConfiguration();
+
+        String outputDir = preapareAbsoluteOutputDirPath(EMPTY_STRING).getAbsolutePath();


Something looks fishy here, Why are you passing an empty string to get output directory path?

rohanKanojia · 2020-02-20T13:51:27Z

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/PushMojo.java

+        }
+    }
+
+    private void jibPush(ImageConfiguration imageConfiguration) throws MojoExecutionException {


In my opinion, this method should go to JibService or JibServiceUtil since it's doing everything related to JIB here. WDYT?

This function uses a lot of pushmojo class properties here and actually uses the JibServiceUtil.pushImage function.
I don't think there's gonna be improvement in clarity by shifting this function to JibServiceUtil and making it unnecessarily static.

@manusa thoughts?

Ideally PushMojo should be agnostic to the ImageService that implements it, so it doesn't make sense to have here a method named jibPush neither dockerPush neither buildahPush

On the other hand, our PushMojo is extending DMP PushMojo, so it's difficult to override the current ServiceHubFactory and its implementations in AbstractDockerMojo without a major refactor to have something similar to what's in BuildMojo(Fabric8ServiceHub).

So, only for FMP, I would make it static and move it to a helper class. This only requires passing 4 fields: MavenProject, ImageConfiguration, RegistryConfig and Logger.

Once we port this to JKube we'll perform a deeper refactor. It doesn't make much sense to work on this for FMP

rohanKanojia · 2020-02-20T14:26:05Z

While testing this feature one thing I noticed is the change in logging during JIB mode. Somehow it seems to be messed up right now. Earlier it was more interactive and dynamic, now it seems redundant:

Current behavior(as per on this PR):

~/work/repos/jib-helloworld-sample : $ mvn fabric8:build
[INFO] Scanning for projects...
[INFO] 
[INFO] ------------< io.fabric8.dmp.samples:dmp-sample-helloworld >------------
[INFO] Building dmp-sample-helloworld 4.5-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- fabric8-maven-plugin:4.5-SNAPSHOT:build (default-cli) @ dmp-sample-helloworld ---
[INFO] F8: Running in Kubernetes mode
[INFO] F8: Building Container image with JIB(Java Image Builder) in Kubernetes mode
[INFO] F8: JIB image build started
[INFO] F8: Preparing assembly files
[INFO] Copying files to /home/rohaan/work/repos/jib-helloworld-sample/target/docker/rohankanojia/helloworld-java/build/maven
[INFO] F8: Building Image Tarball at rohankanojia/helloworld-java.tar ...
[WARNING] F8: Base image 'openjdk' does not use a specific image digest - build may not be reproducible
[INFO] F8: Containerizing application with the following files:
[INFO] F8: 	:
[INFO] F8: 		/home/rohaan/work/repos/jib-helloworld-sample/target/docker/rohankanojia/helloworld-java/work/maven/dmp-sample-helloworld-4.5-SNAPSHOT.jar
[INFO] F8: Executing tasks:
[INFO] F8: [                              ] 0.0% complete
[INFO] F8: > building image to tar file
[INFO] F8: Executing tasks:
[INFO] F8: Executing tasks:
[INFO] F8: [                              ] 0.0% complete
[INFO] F8: > pulling base image manifest
[INFO] F8: > launching application layer builders
[INFO] F8: [                              ] 0.0% complete
[INFO] F8: > pulling base image manifest
[INFO] F8: Executing tasks:
[INFO] F8: [                              ] 0.0% complete
[INFO] F8: > pulling base image manifest
[INFO] F8: > building  layer
[INFO] F8: Executing tasks:
[INFO] F8: [======                        ] 20.0% complete
[INFO] F8: > pulling base image manifest
[INFO] F8: Executing tasks:
[INFO] F8: [======                        ] 20.0% complete
[INFO] F8: > retrieving credentials for registry-1.docker.io
[INFO] F8: No credentials could be retrieved for registry registry-1.docker.io
[INFO] F8: Executing tasks:
[INFO] F8: [=========                     ] 30.0% complete
[INFO] F8: > pulling base image manifest
[INFO] F8: Executing tasks:
[INFO] F8: [=========                     ] 30.0% complete
[INFO] F8: > pull container configuration sha256:6adc576f6a5804a5a8be24a959bda91821030adcb70437d4e3cf8fe32d60e36c
[INFO] F8: Executing tasks:
[INFO] F8: [============                  ] 40.0% complete
[INFO] F8: > building image to tar file
[INFO] F8: Executing tasks:
[INFO] F8: [============                  ] 40.0% complete
[INFO] F8: > launching base image layer pullers
[INFO] F8: Executing tasks:
[INFO] F8: [============                  ] 40.0% complete
[INFO] F8: > checking base image layer sha256:a1d217c4e16034b1dca5abb6ea80aabded93c73f35686d806d9f2aa8555743da
[INFO] F8: Executing tasks:
[INFO] F8: [==============                ] 46.7% complete
[INFO] F8: > launching base image layer pullers
[INFO] F8: Executing tasks:
[INFO] F8: [==============                ] 46.7% complete
[INFO] F8: > checking base image layer sha256:86607bb85307f4e017c6cc236573951482f1de084e0987480fecea9295c2f923
[INFO] F8: Executing tasks:
[INFO] F8: Executing tasks:
[INFO] F8: [================              ] 53.3% complete
[INFO] F8: > checking base image layer sha256:9595022a553604b9baa8a02558174325cd4be3fb6b7920f86a7e425fc4539de5
[INFO] F8: [==============                ] 46.7% complete
[INFO] F8: > checking base image layer sha256:86607bb85307f4e017c6cc236573951482f1de084e0987480fecea9295c2f923
[INFO] F8: > checking base image layer sha256:9595022a553604b9baa8a02558174325cd4be3fb6b7920f86a7e425fc4539de5
[INFO] F8: Executing tasks:
[INFO] F8: [==================            ] 60.0% complete
[INFO] F8: > building image to tar file
[INFO] F8: Executing tasks:
[INFO] F8: [==================            ] 60.0% complete
[INFO] F8: > building image format
[INFO] F8: Executing tasks:
[INFO] F8: [========================      ] 80.0% complete
[INFO] F8: > building image to tar file
[INFO] F8: Executing tasks:
[INFO] F8: [========================      ] 80.0% complete
[INFO] F8: > writing to tar file
[INFO] F8: Executing tasks:
[INFO] F8: [==============================] 100.0% complete
[INFO] F8:  /home/rohaan/work/repos/jib-helloworld-sample/target/docker/rohankanojia/helloworld-java.tar successfully built
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  12.819 s
[INFO] Finished at: 2020-02-20T19:52:25+05:30
[INFO] ------------------------------------------------------------------------
~/work/repos/jib-helloworld-sample : $

Behavior on 4.4.0:

manusa · 2020-02-20T14:44:40Z

While testing this feature one thing I noticed is the change in logging during JIB mode. Somehow it seems to be messed up right now. Earlier it was more interactive and dynamic, now it seems redundant...

This is something that I changed because previous logging was relying on JIB's own logger which in order to be instantiated required the creation of fake Executors which is very nasty and dirty.

Logging can be improved but will probably need some changed in our own logger implementations. My suggestion is to leave this as is for FMP, port it to JKube, and then improve the UX in a separate issue.

samples/spring-boot/pom.xml

…onfiguration.registry" This reverts commit 9b2a241.

…o v12.0.0 due to errors

devang-gaur · 2020-02-26T08:19:09Z

CHANGELOG.md

@@ -23,6 +23,9 @@ Usage:
 ```

 ### 4.5-SNAPSHOT
+* Refactor #1766: Jib Refactor
+* Revert #1737 : Reverted "Wrong Jib output directory in Maven multi-module build"


@rohanKanojia made changes, how about this?

manusa

LGTM

rohanKanojia · 2020-03-02T07:37:26Z

samples/spring-boot/pom.xml

@@ -178,6 +179,39 @@
                                    </fmp-service>
                                </config>
                            </enricher>
+                            <!-- sample assembly configuration. make sure all the directories configured exist. -->
+                            <!--<images>
+                                <image>


Is it okay to leave this code as commented?

I have knowingly added this here in the sample pom. We actually don't have any sample demonstrating AssemblyConfig feature so I thought this will be helpful.

rohanKanojia · 2020-03-02T07:40:54Z

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/PushMojo.java

+        if (value == null) {
+            value = project.getProperties().getProperty(key);
+        }
+        return value;


This looks like a util function, I think there should already be some method available in EnvUtil for this

This method should be inherited from the AbstractFabric8Mojo class, but since BuildMojo and PushMojo don't inherit the AbstractFabric8Mojo this method has been defined separately for just these two cases.

devang-gaur added the pr/wip Work in Progress, do not merge label Dec 3, 2019

devang-gaur mentioned this pull request Dec 3, 2019

Change parent class of PushMojo for extensibility in FMP fabric8io/docker-maven-plugin#1309

Closed

devang-gaur force-pushed the jib-refactor branch from 66bca4a to 3d1541d Compare December 3, 2019 21:18

devang-gaur mentioned this pull request Dec 4, 2019

API support for TarImage push to registry GoogleContainerTools/jib#2104

Closed

devang-gaur force-pushed the jib-refactor branch 3 times, most recently from d61d5a0 to 000458b Compare December 6, 2019 13:42

devang-gaur force-pushed the jib-refactor branch from 7f23557 to acc895c Compare January 6, 2020 01:50

rohanKanojia mentioned this pull request Jan 9, 2020

FMP and JIb integration #1757

Closed

manusa reviewed Jan 14, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

manusa reviewed Jan 14, 2020

View reviewed changes

core/src/main/java/io/fabric8/maven/core/service/BuildService.java Outdated Show resolved Hide resolved

manusa reviewed Jan 14, 2020

View reviewed changes

core/src/main/java/io/fabric8/maven/core/service/kubernetes/JibBuildService.java Outdated Show resolved Hide resolved

manusa reviewed Jan 14, 2020

View reviewed changes

core/src/main/java/io/fabric8/maven/core/util/JibServiceUtil.java Outdated Show resolved Hide resolved

devang-gaur force-pushed the jib-refactor branch 2 times, most recently from cb77c35 to ccf6bf6 Compare January 30, 2020 02:01

devang-gaur added jkube/pending The issue/PR has to be taken care of in JKube https://github.com/eclipse/jkube target/JKube Implementation to be performed in JKube and removed pr/wip Work in Progress, do not merge labels Jan 30, 2020

devang-gaur requested a review from rohanKanojia January 30, 2020 02:18

devang-gaur force-pushed the jib-refactor branch from ccf6bf6 to fc5424e Compare January 30, 2020 09:30

manusa reviewed Jan 30, 2020

View reviewed changes

generator/api/src/main/java/io/fabric8/maven/generator/api/support/BaseGenerator.java Outdated Show resolved Hide resolved

devang-gaur force-pushed the jib-refactor branch 3 times, most recently from 02dc4e2 to 9238397 Compare February 5, 2020 11:21

devang-gaur force-pushed the jib-refactor branch 2 times, most recently from ea80edb to feef207 Compare February 18, 2020 07:53

rohanKanojia reviewed Feb 20, 2020

View reviewed changes

devang-gaur force-pushed the jib-refactor branch from 2083ac1 to 96be717 Compare February 24, 2020 17:01

manusa reviewed Feb 25, 2020

View reviewed changes

samples/spring-boot/pom.xml Outdated Show resolved Hide resolved

manusa reviewed Feb 25, 2020

View reviewed changes

samples/spring-boot/pom.xml Outdated Show resolved Hide resolved

Devang Gaur and others added 8 commits February 25, 2020 14:14

Revert "Fix fabric8io#1738: JibBuildConfiguration ignores BuildImageC…

0b039b2

…onfiguration.registry" This reverts commit 9b2a241.

Jib refactor

6f52d48

Added AssemblyConfiguration support for Jib builds

610bd73

NPE checkon pull registry credentials and Jib-core version rollback t…

3c6ce5c

…o v12.0.0 due to errors

Relocated FatJarDetector Utility to generator/java-exec pacakge

ffe930d

Corrections

6f4e3ed

Remove redundant JibBuildConfiguration

325c672

Remove unneeded executors + structure build code

e208d69

devang-gaur force-pushed the jib-refactor branch 2 times, most recently from 05c01c4 to f307496 Compare February 25, 2020 11:19

devang-gaur requested review from rohanKanojia and manusa February 26, 2020 07:32

Modifications to enable clean up after assemblyconfig processing

d2842f3

devang-gaur force-pushed the jib-refactor branch from f307496 to d2842f3 Compare February 26, 2020 08:18

devang-gaur commented Feb 26, 2020

View reviewed changes

PushMojo refactor

917dad3

manusa approved these changes Feb 28, 2020

View reviewed changes

rohanKanojia reviewed Mar 2, 2020

View reviewed changes

rohanKanojia approved these changes Mar 2, 2020

View reviewed changes

Merge branch 'master' into jib-refactor

e83818c

devang-gaur merged commit 226d78b into fabric8io:master Mar 2, 2020

This was referenced Mar 5, 2020

Jib: Jib build is not packing files from assembly descriptor #1765

Closed

JIB: Unauthorized registry exception for "from" registry #1763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jib refactor #1766

Jib refactor #1766

devang-gaur commented Dec 3, 2019 •

edited

Loading

devang-gaur commented Dec 8, 2019

manusa commented Jan 14, 2020

manusa commented Jan 14, 2020

devang-gaur commented Jan 30, 2020

rohanKanojia left a comment

rohanKanojia Feb 20, 2020

rohanKanojia Feb 20, 2020

devang-gaur Feb 24, 2020

manusa Feb 25, 2020 •

edited

Loading

rohanKanojia commented Feb 20, 2020

manusa commented Feb 20, 2020

devang-gaur Feb 26, 2020

manusa left a comment

rohanKanojia Mar 2, 2020

devang-gaur Mar 2, 2020

rohanKanojia Mar 2, 2020

devang-gaur Mar 2, 2020

Jib refactor #1766

Jib refactor #1766

Conversation

devang-gaur commented Dec 3, 2019 • edited Loading

devang-gaur commented Dec 8, 2019

manusa commented Jan 14, 2020

manusa commented Jan 14, 2020

devang-gaur commented Jan 30, 2020

rohanKanojia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manusa Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

rohanKanojia commented Feb 20, 2020

manusa commented Feb 20, 2020

Choose a reason for hiding this comment

manusa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devang-gaur commented Dec 3, 2019 •

edited

Loading

manusa Feb 25, 2020 •

edited

Loading