Refactor #802: Remove resource object post processing #1521

rohanKanojia · 2019-02-13T12:57:43Z

Refactor #678 #802

core/src/main/java/io/fabric8/maven/core/util/kubernetes/KubernetesResourceUtil.java

...her/standard/src/main/java/io/fabric8/maven/enricher/standard/DefaultControllerEnricher.java

enricher/standard/src/main/java/io/fabric8/maven/enricher/standard/PodAnnotationEnricher.java

lordofthejars · 2019-02-18T10:35:48Z

...ard/src/main/java/io/fabric8/maven/enricher/standard/openshift/DeploymentConfigEnricher.java

+    }
+
+    private Boolean isAutomaticTriggerEnabled(MavenEnricherContext enricherContext, Boolean defaultValue) {
+        if(enricherContext.getProperty("fabric8.openshift.enableAutomaticTrigger") != null) {


If getProperty can be called without casting then we will not have a Maven dependency anymore.

enricher/api/src/main/java/io/fabric8/maven/enricher/api/Enricher.java

rhuss · 2019-02-19T11:04:58Z

core/src/main/java/io/fabric8/maven/core/config/RuntimeMode.java

+     * Detect automatically whether running on OpenShift or Kuberentes.
+     * This is done by contacting an API server
+     */
+    auto(true, "Auto");


Iiur then "PlatformMode" is used for creating the resources (i.e. for doing the two passes through the enricher chain), and "RuntimeMode" is for deployment (i.e. to select which client to use for applying resources).

What does then "auto" mean here ? I would also recommend to adapt the documentation to better reflect this distinction.

rhuss · 2019-02-19T11:11:53Z

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

@@ -293,10 +278,8 @@
    // Access for creating OpenShift binary builds
    private ClusterAccess clusterAccess;

-    private PlatformMode platformMode;
+    private RuntimeMode runtimeMode;


There must be no reference to the runtimeMode in ResourceMojo. Do you know why this is needed ?

It's used to find out which runtime we're using here and setting some properties(Maybe they were relevant in fabric8 context):

fabric8-maven-plugin/plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/ResourceMojo.java

Line 489 in 19c3689

platformMode = clusterAccess.resolvePlatformMode(mode, log);

Plus we add ImageChange triggers in deployment config based on runtime mode:

fabric8-maven-plugin/plugin/src/main/java/io/fabric8/maven/plugin/converter/DeploymentOpenShiftConverter.java

Lines 117 to 127 in 19c3689

if (mode.equals(PlatformMode.openshift)) {

for (Map.Entry<String, String> entry : containerToImageMap.entrySet()) {

String containerName = entry.getKey();

ImageName image = new ImageName(entry.getValue());

String tag = image.getTag() != null ? image.getTag() : "latest";

specBuilder.addNewTrigger()

.withType("ImageChange")

.withNewImageChangeParams()

.withAutomatic(enableAutomaticTrigger)

.withNewFrom()

.withKind("ImageStreamTag")

We should really try hard to get rid of this. I.e. we should check who is using this property and seek for a solution which doesn't involve this runtime mode.

The reason why I'm arguing so hard for removing it is that it can't be that the resources are created differently depending on whether you are connected to a Kuberntes or OpenShift cluster. This totally defeats repeatable builds.

IMO the DeploymentOpenShiftConverter should either use the new PlatformMode or we should question why it adds the image trigger here. If this is because e.g. needed for 'fabric8:build' triggering a redeployment, then this functionality should go to 'fabric8:build' to patch existing deployment configs, or even trigger a redeployment on their own.

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

rhuss · 2019-02-19T11:12:30Z

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

@@ -416,18 +399,17 @@ public void executeInternal() throws MojoExecutionException, MojoFailureExceptio
            resolvedImages = getResolvedImages(images, log);
            if (!skip && (!isPomProject() || hasFabric8Dir())) {
                // Extract and generate resources which can be a mix of Kubernetes and OpenShift resources
-                KubernetesList resources = generateResources(resolvedImages, remoteResources);
-                // Adapt list to use OpenShift specific resource objects
-                KubernetesList openShiftResources = convertToOpenShiftResources(resources);


👍 yeah, finally ;-)

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

lordofthejars

Minor comment

lordofthejars · 2019-02-20T14:34:41Z

core/src/main/java/io/fabric8/maven/core/handler/DeploymentConfigHandler.java

+import io.fabric8.maven.core.util.kubernetes.KubernetesHelper;
+import io.fabric8.maven.docker.config.ImageConfiguration;
+import io.fabric8.maven.docker.util.ImageName;
+import io.fabric8.openshift.api.model.*;


Avoid star import.

rhuss

Very nice work, really love it.

I did some sanity checks on ResourceMojo and the Enricher API, see my comments.

The only thing which we really should fix is the usage of the RuntimeMode during resource creation. IMO we should get rid of this by checking where it is used and either find a different way to implement that behaviour or, in the worst case, remove that functionality then (and maybe implement it differently).

@rohanKanojia it would be awesome if you could identify the remaining use cases of the RuntimeMode in ResourceMojo.

rhuss · 2019-02-21T09:07:28Z

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

@@ -293,10 +278,8 @@
    // Access for creating OpenShift binary builds
    private ClusterAccess clusterAccess;

-    private PlatformMode platformMode;
+    private RuntimeMode runtimeMode;


We should really try hard to get rid of this. I.e. we should check who is using this property and seek for a solution which doesn't involve this runtime mode.

The reason why I'm arguing so hard for removing it is that it can't be that the resources are created differently depending on whether you are connected to a Kuberntes or OpenShift cluster. This totally defeats repeatable builds.

rhuss · 2019-02-21T09:11:15Z

enricher/api/src/main/java/io/fabric8/maven/enricher/api/Enricher.java

@@ -63,14 +37,12 @@
     *
     * @param builder the build to examine and add to
     */
-    void addMissingResources(KubernetesListBuilder builder);
+    void enrich(PlatformMode platformMode, KubernetesListBuilder builder);


Looks very nice now ! It has now the two inner passes from the initial suggestion.

Wdyt about renaming these two methods to "create" (was "enrich") and to "enrich" (was "adapt") as suggested in #678 ?

That way we can clearly indicate the intension of this API, too.

rhuss · 2019-02-21T09:12:26Z

enricher/api/src/main/java/io/fabric8/maven/enricher/api/EnricherContext.java

-    OpenShiftDependencyResources getOpenshiftDependencyResources();
+    PlatformMode getPlatformMode();
+
+    RuntimeMode getRuntimeMode();


This must not be there. (see other comments)

rhuss · 2019-02-21T09:14:03Z

enricher/api/src/main/java/io/fabric8/maven/enricher/api/EnricherContext.java

+
+    Object getProperty(String key);
+
+    Properties getProperties();


What's about having only one of these methods to keep the API slim?

Alternatively, you could make getProperty() a "default" interface method, too.

And don't forget to add JavaDoc. While I wouldn't insist on having docs everywhere, for public API which are used e.g. for implementing Enrichers, good documentation is mandatory.

rohanKanojia · 2019-02-25T12:11:05Z

@rhuss : I've filed an issue related to refactoring GeneratorContext out of ResourceMojo which can be done later: #1528

+ Remove of getLabels(), getAnnotations() + Remove getSelector(), addMetadata() + Rename enrich() -> create() and adapt() -> enrich() for better readability. + Removed RuntimeMode from ResourceMojo

rohanKanojia added the pr/wip Work in Progress, do not merge label Feb 13, 2019

rohanKanojia force-pushed the pr/issue802 branch from d290284 to 7bd668c Compare February 13, 2019 12:58

rohanKanojia assigned rohanKanojia and devang-gaur Feb 13, 2019

rohanKanojia force-pushed the pr/issue802 branch 2 times, most recently from 5bee1ab to 3323f13 Compare February 15, 2019 08:02

rohanKanojia removed the pr/wip Work in Progress, do not merge label Feb 15, 2019

rohanKanojia force-pushed the pr/issue802 branch from 3323f13 to 259c7cb Compare February 15, 2019 08:09

lordofthejars suggested changes Feb 18, 2019

View reviewed changes

rhuss reviewed Feb 19, 2019

View reviewed changes

enricher/api/src/main/java/io/fabric8/maven/enricher/api/Enricher.java Outdated Show resolved Hide resolved

rhuss reviewed Feb 19, 2019

View reviewed changes

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/ResourceMojo.java Show resolved Hide resolved

rhuss reviewed Feb 19, 2019

View reviewed changes

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/ResourceMojo.java Outdated Show resolved Hide resolved

rhuss reviewed Feb 19, 2019

View reviewed changes

plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/ResourceMojo.java Outdated Show resolved Hide resolved

rohanKanojia force-pushed the pr/issue802 branch 2 times, most recently from 6ae4cbe to 9ffd67f Compare February 20, 2019 12:19

rohanKanojia and others added 4 commits February 20, 2019 17:50

Refactor fabric8io#802: Remove resource object post processing

a036621

Refactor resource handlers and converters

70899cb

Refactor resource handlers and converters (#2)

3a260c6

Remove OpenshiftConverters as they aren't needed anymore

5163539

rohanKanojia force-pushed the pr/issue802 branch 3 times, most recently from cdcb17f to e0df31d Compare February 20, 2019 12:39

lordofthejars reviewed Feb 20, 2019

View reviewed changes

rohanKanojia force-pushed the pr/issue802 branch from e0df31d to 334d1f8 Compare February 20, 2019 17:13

rhuss suggested changes Feb 21, 2019

View reviewed changes

rohanKanojia force-pushed the pr/issue802 branch 2 times, most recently from f03d063 to 6db2ffa Compare February 22, 2019 08:40

rohanKanojia force-pushed the pr/issue802 branch from 6db2ffa to 653c95e Compare February 22, 2019 09:10

rohanKanojia added the target/4.0 PR for targeted to 4.0.x label Feb 25, 2019

lordofthejars approved these changes Feb 25, 2019

View reviewed changes

Refactoring Enricher interface for more clarity

a456ac4

+ Remove of getLabels(), getAnnotations() + Remove getSelector(), addMetadata() + Rename enrich() -> create() and adapt() -> enrich() for better readability. + Removed RuntimeMode from ResourceMojo

rohanKanojia force-pushed the pr/issue802 branch from 653c95e to a456ac4 Compare February 26, 2019 06:06

rohanKanojia merged commit b3c6060 into fabric8io:master Feb 26, 2019

This was referenced Feb 26, 2019

Refactor Enricher Workflow to allow direct generation of OpenShift objects #678

Closed

Remove resource object post-processing #802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor #802: Remove resource object post processing #1521

Refactor #802: Remove resource object post processing #1521

rohanKanojia commented Feb 13, 2019

lordofthejars Feb 18, 2019

rhuss Feb 19, 2019

rhuss Feb 19, 2019

rohanKanojia Feb 19, 2019

rohanKanojia Feb 20, 2019

rhuss Feb 21, 2019

rhuss Feb 21, 2019

rhuss Feb 19, 2019

lordofthejars left a comment

lordofthejars Feb 20, 2019

rhuss left a comment

rhuss Feb 21, 2019

rhuss Feb 21, 2019

rhuss Feb 21, 2019

rhuss Feb 21, 2019

rohanKanojia commented Feb 25, 2019

	if (mode.equals(PlatformMode.openshift)) {
	for (Map.Entry<String, String> entry : containerToImageMap.entrySet()) {
	String containerName = entry.getKey();
	ImageName image = new ImageName(entry.getValue());
	String tag = image.getTag() != null ? image.getTag() : "latest";
	specBuilder.addNewTrigger()
	.withType("ImageChange")
	.withNewImageChangeParams()
	.withAutomatic(enableAutomaticTrigger)
	.withNewFrom()
	.withKind("ImageStreamTag")

Refactor #802: Remove resource object post processing #1521

Refactor #802: Remove resource object post processing #1521

Conversation

rohanKanojia commented Feb 13, 2019

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

lordofthejars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

rohanKanojia commented Feb 25, 2019