Fix #825 : More flexible S2I binary build data format #1355

rohanKanojia · 2018-08-14T15:54:13Z

rhuss · 2018-08-14T18:46:33Z

Awesome, thanks a lot @rohanKanojia ! Unfortunately I only can have a look at it as I'm on PTO for the rest of the week (+ Monday next week). I hope that I can then work fulltime on fmp as the last two weeks were eaten by my other work (we are about to release Fuse 7.1).

sorry for that.

rhuss

This is a first round of reviews which you might want to address.

Currently I'm not so happy about the build service in genereal (and how things are moved around), so will wrap my brain around this.

btw, feel free to use lambdas, too (like for filtering the file names).

rhuss · 2018-09-10T11:59:50Z

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

@@ -35,7 +35,7 @@
     *
     * @param imageConfig the image to build
     */
-    void build(ImageConfiguration imageConfig) throws Fabric8ServiceException;
+    void build(ImageConfiguration imageConfig, BinaryInputArchiveBuilder binaryInputArchiveBuilder) throws Fabric8ServiceException;


It's a bad smell to add a parameter which is useful only for a particular implementation. I.e. the BuildService itself shoudl be agnostic of any implementation, but by adding an OpenShift specific build you directly couple it with the OpenShift build mechanism.

rhuss · 2018-09-10T12:01:50Z

core/src/main/java/io/fabric8/maven/core/service/openshift/OpenshiftBuildService.java

-        }
+    private File createEmptyTargetDir() {
+        File targetDir = new File(config.getBuildDirectory(), "docker");
+        targetDir.mkdirs();


you should check the return value

rhuss · 2018-09-10T12:23:30Z

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

+        boolean bAllFilesDeleted = true;
+        String[] files = parentDir.list(filterNameFilter);
+
+        if(files.length == 0) {


files can be null, too

rhuss · 2018-09-10T12:24:10Z

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

@@ -55,4 +56,21 @@ public static String getAbsolutePath(URL url) {
            throw new RuntimeException(e);
        }
    }
+
+    public static boolean deleteFilesInDirectory(File parentDir, FilenameFilter filterNameFilter) {


Return value is not really used

rhuss · 2018-09-10T12:25:09Z

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

+        File fileToDelete = null;
+        for(String file : files) {
+            String absoluteFilePath = parentDir.getAbsolutePath() + File.separator + file;
+            fileToDelete = new File(absoluteFilePath);


what's about just new File(parentDir, file) ?

rhuss · 2018-09-10T13:14:24Z

generator/karaf/src/main/java/io/fabric8/maven/generator/karaf/KarafGenerator.java

+            @Override
+            public void createBinaryInput(ServiceHub hub, File binaryInputDirectory, ImageConfiguration imageConfiguration) throws Fabric8ServiceException {
+                try {
+                    File targetDirectory = new File(config.getBuildDirectory());


The code here and in the WebApp generator look quite the same. Shouldn't we extract it into an external class which then is used here and in the Webapp generator ?

rhuss · 2018-09-10T13:16:41Z

...bapp/src/main/java/io/fabric8/maven/generator/webapp/handler/OpenshiftWildFlyS2IHandler.java

+import java.util.Arrays;
+import java.util.List;
+
+public class OpenshiftWildFlyS2IHandler extends AbstractAppServerHandler {


Not really sure about the name. What's the difference the WildFlyAppServerHandler ?

rhuss · 2018-09-10T13:16:50Z

...bapp/src/main/java/io/fabric8/maven/generator/webapp/handler/OpenshiftWildFlyS2IHandler.java

+
+public class OpenshiftWildFlyS2IHandler extends AbstractAppServerHandler {
+    public OpenshiftWildFlyS2IHandler(MavenProject project) {
+        super("tomcat", project);


"tomcat" sounds fishy here.

rhuss · 2018-09-10T13:17:35Z

...bapp/src/main/java/io/fabric8/maven/generator/webapp/handler/OpenshiftWildFlyS2IHandler.java

+    public boolean isApplicable() {
+        if(PlatformMode.isOpenShiftMode(project.getProperties())) {
+            return hasOneOf("**/META-INF/context.xml") ||
+                    MavenUtil.hasPlugin(project, "org.apache.tomcat.maven:tomcat6-maven-plugin") ||


why is a tomcat maven plugin an indicator for this wildfly handler ?

rhuss · 2018-09-10T13:18:38Z

parent/pom.xml

@@ -73,6 +73,7 @@
    <!-- Upstream -->
    <version.image.java.upstream.docker>1.5</version.image.java.upstream.docker>
    <version.image.java.upstream.s2i>2.3</version.image.java.upstream.s2i>
+    <version.image.webapp.upstream.s2i>latest</version.image.webapp.upstream.s2i>


we shouldn't use latest in our deps if not for a very good reason. Can't we stick to a version, like for the other images ?

not sure, there doesn't seem to be any other tag here: https://hub.docker.com/r/openshift/wildfly-101-centos7/tags/

codecov · 2018-09-17T13:09:44Z

Codecov Report

Merging #1355 into master will decrease coverage by 0.12%.
The diff coverage is 26.86%.

@@             Coverage Diff              @@
##             master    #1355      +/-   ##
============================================
- Coverage     31.85%   31.73%   -0.13%     
- Complexity     1001     1007       +6     
============================================
  Files           174      177       +3     
  Lines          9915    10000      +85     
  Branches       1790     1800      +10     
============================================
+ Hits           3158     3173      +15     
- Misses         6406     6472      +66     
- Partials        351      355       +4

rhuss · 2018-10-02T13:32:41Z

core/src/main/java/io/fabric8/maven/core/service/openshift/OpenshiftBuildService.java

-        } catch (IOException e) {
-            throw new Fabric8ServiceException("Unable to add environment variables to the S2I build archive", e);
-        }
+        File tarFile = File.createTempFile(config.getArtifactId().toString(), ".tar");


Not sure if I understand this: tarFile is a temporary tar filename but the tar archive itself is created with the destFile as name. The return value is then a the temporary name (which doesn't point to any tar archive if I understood the code right). Could you please check this again ?

rhuss · 2018-10-02T13:34:13Z

core/src/main/java/io/fabric8/maven/core/service/openshift/OpenshiftBuildService.java

-        }
+        File tarFile = File.createTempFile(config.getArtifactId().toString(), ".tar");
+        File destFile = new File(targetDir, config.getArtifactId() + ".tar");
+        tarArchiver.setDestFile(destFile);


Also not sure that this works if you put the destination file into the directory that you tar up (as it would be tar up itself, too)

Naiively I would expect a

tarArchiver.setDestFile(tarFile); tarArchiver.createArchive(); return tarFile;

and removing destFile ....

rhuss · 2018-10-02T13:44:35Z

core/src/main/java/io/fabric8/maven/core/service/openshift/OpenshiftBuildService.java

+            File targetDir = createEmptyTargetDir();
+
+            // Fill the directory with the data in the right directory structure
+            binaryInputArchiveBuilder.createBinaryInput(dockerServiceHub, targetDir, imageConfig);


What's about adding a File return value to the BinaryInputArchiveBuilder which then holds the the TAR archive ?

Advantages:

Get rid of createTar() method

Simplify interface to one method

No need for the targetDir parameter. This just would be an implementation details.

If you don't mind I would change it like this and push it to your branch (just to speed up things and to clarify my point).

@rohanKanojia would this be ok for you ?

rhuss · 2018-10-02T13:50:04Z

...or/webapp/src/main/java/io/fabric8/maven/generator/webapp/handler/TomcatAppSeverHandler.java

-        return hasOneOf("**/META-INF/context.xml") ||
-                MavenUtil.hasPlugin(project, "org.apache.tomcat.maven:tomcat6-maven-plugin") ||
-                MavenUtil.hasPlugin(project, "org.apache.tomcat.maven:tomcat7-maven-plugin");
+        if(!PlatformMode.isOpenShiftMode(project.getProperties())) {


What is the motivation of this check ?

rhuss · 2018-10-02T13:52:10Z

generator/webapp/src/main/java/io/fabric8/maven/generator/webapp/handler/WarS2IHandler.java

+    @Override
+    public boolean isApplicable() {
+        if(PlatformMode.isOpenShiftMode(project.getProperties())) {
+            return hasOneOf("**/META-INF/context.xml") || project.getPackaging().equals("war");


Ok, got it. You have a dedicated handler only for s2i build ?. But shouldn't we just check then for the packaging ? Not sure why a JAR archive with a "**/META-INF/context.xml" should be considered as a war file.

rhuss · 2018-10-03T08:28:31Z

@rohanKanojia Thanks a lot for your changes which adhere to the comments I did. However while reviewing and some minor refactorings, I thought again about this PR and I now believe, that a Generator shouldn't be actually responsible for the creation of the binary input.

Why? Well, because a generator is only about creating the configuration of an image which then can be created in multiple ways (docker, s2i binary, s2i docker, ...). So the generator should be agnostic of the concrete creation of the image.

What now? The generator selects a base image and the base image is actually that part which in the s2i binary input case determines the format of the binary input. So my suggestion would be to either examine the generated images for the 'from' images or evaluate a certain meta-data (label) which could be set by the generator. Then a kind of 'BinaryBuildSelector' would examine this base image and creates the binary input which fits this s2i builder image.

Does this make sense? If so, I would like to postpone this story for the next sprint implement it in a generator independent way.

rohanKanojia · 2018-10-03T08:33:25Z

@rhuss : ah, okay

rohanKanojia added the WIP label Aug 14, 2018

rohanKanojia requested a review from rhuss August 14, 2018 15:54

rohanKanojia force-pushed the issue825 branch from dd8e107 to a87538b Compare August 14, 2018 16:05

rohanKanojia force-pushed the issue825 branch from a87538b to 0746196 Compare August 16, 2018 13:27

rohanKanojia force-pushed the issue825 branch from 0746196 to 8beda35 Compare August 24, 2018 09:54

rohanKanojia removed the WIP label Aug 24, 2018

rhuss suggested changes Sep 10, 2018

View reviewed changes

rohanKanojia force-pushed the issue825 branch 2 times, most recently from 32afdd1 to 8c6af3a Compare September 17, 2018 13:05

Fix fabric8io#825 : More flexible S2I binary build data format

da1a4d0

rohanKanojia force-pushed the issue825 branch from 8c6af3a to da1a4d0 Compare September 24, 2018 09:13

rhuss added the target/4.0 PR for targeted to 4.0.x label Sep 30, 2018

rhuss reviewed Oct 2, 2018

View reviewed changes

rohanKanojia closed this Feb 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #825 : More flexible S2I binary build data format #1355

Fix #825 : More flexible S2I binary build data format #1355

rohanKanojia commented Aug 14, 2018

rhuss commented Aug 14, 2018

rhuss left a comment

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rhuss Sep 10, 2018

rohanKanojia Sep 16, 2018

codecov bot commented Sep 17, 2018 •

edited

Loading

rhuss Oct 2, 2018

rhuss Oct 2, 2018

rhuss Oct 2, 2018

rhuss Oct 2, 2018

rhuss Oct 2, 2018

rhuss Oct 2, 2018

rhuss commented Oct 3, 2018 •

edited

Loading

rohanKanojia commented Oct 3, 2018

Fix #825 : More flexible S2I binary build data format #1355

Fix #825 : More flexible S2I binary build data format #1355

Conversation

rohanKanojia commented Aug 14, 2018

rhuss commented Aug 14, 2018

rhuss 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 17, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

rhuss commented Oct 3, 2018 • edited Loading

rohanKanojia commented Oct 3, 2018

codecov bot commented Sep 17, 2018 •

edited

Loading

rhuss commented Oct 3, 2018 •

edited

Loading