Use basepom as parent instead of Sonatype OSS #377

zregvart · 2017-05-25T12:51:09Z

This is can be contentious. Basepom is a project by one of Maven's most prolific commiters it embodies best practices and, contentiously enough, enforces most of them.
I propose changing Syndesis parent's parent to Basepom. Using Basepom is either hit-or-miss as it can be quite annoying in enforcing the right way of using dependency management and enforcing style checks.
I've used it before and I'm a big fan, but then again I do love to self immolate myself with every check imaginable.
Creating a WIP PR to test the grounds with just a couple of changes done (doesn't build ATM).

jimmibot · 2017-05-25T12:51:11Z

@zregvart, thanks! @KurtStam, please review this.

pure-bot · 2017-05-25T12:54:47Z

⚠️ Status check default returned failure.

Build finished.

See https://ci.fabric8.io/job/syndesis-rest-pullreq/599/ for more details.

pure-bot · 2017-05-25T12:55:12Z

⚠️ Status check ci/circleci returned failure.

Your tests failed on CircleCI

See https://circleci.com/gh/syndesisio/syndesis-rest/954?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link for more details.

pure-bot · 2017-05-25T12:55:12Z

⚠️ Status check ci/circleci returned failure.

Your tests failed on CircleCI

See https://circleci.com/gh/syndesisio/syndesis-rest/954?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link for more details.

pure-bot · 2017-05-25T12:56:27Z

⚠️ Status check continuous-integration/jenkins/pr-merge returned error.

This commit cannot be built

See https://jenkins-syndesis-ci.b6ff.rh-idev.openshiftapps.com/job/syndesis-rest/job/PR-377/1/display/redirect for more details.

rhuss · 2017-05-26T06:55:03Z

Seems like that findbugs doesn't like the generated immutable classes (therefore the build breaks).

Actually, I like the idea to apply common practices and enforce some rules, but I wonder whether we should invest energy right now into this if it is not yet clear in which direction syndesis is 'go'ing ? // @jimmidyson

zregvart · 2017-05-26T08:50:11Z

Yes, my point exactly, to see if this is an area we would like to invest in. Being a fan of the project(s) [basepom, syndesis] I'm biased, and I would like to make this as default, but it comes as a cost.
For example I don't think that the generated *Immutable classes should have with... methods for helper methods, for example isEndpointProperty is not a property but it is being treated as such by immutables.
Various other issues were caught by basepom configuration:

pushing hibernate dependancy from model where javax.validation would be more appropriate (f7714dd#diff-18ca018dfcf70cd5c109eec42650a39bR46)
duplicate classes on classpath (aopalliance both present in spring-aop and depended upon (f7714dd#diff-600376dffeb79835ede4a0b285078036R336)
tab vs space (yippee we get to do that one), JSL declaration order (70af94c)

One thing to note is that basepom is configurable, and has skip properties for every check, so we can bypass findbugs with basepom.check.skip-findbugs.

pure-bot · 2017-05-27T08:12:12Z

⚠️ Status check continuous-integration/jenkins/pr-merge returned error.

This commit cannot be built

See https://jenkins-syndesis-ci.b6ff.rh-idev.openshiftapps.com/job/syndesis-rest/job/PR-377/2/display/redirect for more details.

pure-bot · 2017-05-27T08:12:43Z

⚠️ Status check ci/circleci returned failure.

Your tests failed on CircleCI

See https://circleci.com/gh/syndesisio/syndesis-rest/961?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link for more details.

pure-bot · 2017-05-27T08:12:43Z

⚠️ Status check ci/circleci returned failure.

Your tests failed on CircleCI

See https://circleci.com/gh/syndesisio/syndesis-rest/961?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link for more details.

pure-bot · 2017-05-27T08:14:23Z

⚠️ Status check default returned failure.

Build finished.

See https://ci.fabric8.io/job/syndesis-rest-pullreq/608/ for more details.

This changes the parent POM for the project to basepom[1] and with that brings about a handful of Maven plugin configurations with the ambition to increase the code quality and minimize issues that are caused by misaligned, duplicate or unwanted dependencies. Things of note that are now turned on: - static code checking (findbugs, pmd, checkstyle) - dependency analysis (declared but unused, used but undeclared, version misalignment, duplicate classes and resources) - running tests in parallel and random order - character encodings are set uniformly to UTF-8 (for instance file.encoding when running tests) - JAR manifests contain a wealth of information (versions, git information, like branch, hash...) [1] https://github.com/basepom/basepom

zregvart · 2017-05-27T18:00:09Z

connector-catalog/pom.xml

@@ -32,16 +32,28 @@
  <dependencies>
    <dependency>
      <groupId>org.apache.camel</groupId>
+      <artifactId>camel-catalog</artifactId>


Used dependencies need to be declared

zregvart · 2017-05-27T18:00:31Z

connector-catalog/src/main/java/io/syndesis/connector/catalog/ConnectorCatalog.java

@@ -29,7 +29,7 @@

 public class ConnectorCatalog {

-    private final static Logger log = LoggerFactory.getLogger(ConnectorCatalog.class);
+    private static final Logger log = LoggerFactory.getLogger(ConnectorCatalog.class);


JLS declaration order must be followed

zregvart · 2017-05-27T18:00:41Z

controllers/pom.xml

@@ -32,6 +32,16 @@
  <dependencies>
    <dependency>
      <groupId>io.syndesis</groupId>
+      <artifactId>core</artifactId>


Used dependencies need to be declared

zregvart · 2017-05-27T18:00:58Z

controllers/src/main/java/io/syndesis/controllers/integration/DemoHandlerProvider.java

@@ -15,9 +15,13 @@
 */
 package io.syndesis.controllers.integration;

-import java.util.*;
+import java.util.Arrays;


No stars in imports

pure-bot · 2017-05-27T18:01:34Z

⚠️ Status check continuous-integration/jenkins/pr-merge returned error.

This commit cannot be built

See https://jenkins-syndesis-ci.b6ff.rh-idev.openshiftapps.com/job/syndesis-rest/job/PR-377/3/display/redirect for more details.

zregvart · 2017-05-27T18:01:49Z

controllers/src/main/java/io/syndesis/controllers/integration/DemoHandlerProvider.java

@@ -34,7 +38,7 @@
                            );
    }

-    class DemoHandler implements StatusChangeHandler {
+    static class DemoHandler implements StatusChangeHandler {


Inner classes that do not reference outer need to be static

zregvart · 2017-05-27T18:01:49Z

controllers/src/main/java/io/syndesis/controllers/integration/IntegrationController.java

@@ -16,8 +16,15 @@
 package io.syndesis.controllers.integration;

 import java.io.IOException;
-import java.util.*;
-import java.util.concurrent.*;
+import java.util.Date;


No stars in imports

zregvart · 2017-05-27T18:01:58Z

controllers/src/main/java/io/syndesis/controllers/integration/IntegrationController.java

@@ -48,7 +56,7 @@
    private ExecutorService executor;
    private ScheduledExecutorService scheduler;

-    private final static long SCHEDULE_INTERVAL_IN_SECONDS = 60;
+    private static final long SCHEDULE_INTERVAL_IN_SECONDS = 60;


JLS declaration order

Not really a fan of enforcing JLS ordering as imo it doesn't add much value and is just nit picking.

zregvart · 2017-05-27T18:02:09Z

controllers/src/main/java/io/syndesis/controllers/integration/NoopHandlerProvider.java

@@ -15,9 +15,14 @@
 */
 package io.syndesis.controllers.integration;

-import java.util.*;
+import java.util.Arrays;


No stars in imports

Actually I almost never look at imports so don't mind much how they look like. But I'm fine with any import rule as along as IDEA supports it.

zregvart · 2017-05-27T18:02:18Z

controllers/src/main/java/io/syndesis/controllers/integration/online/ActivateHandler.java

@@ -16,12 +16,20 @@
 package io.syndesis.controllers.integration.online;

 import java.io.IOException;
-import java.util.*;
+import java.util.ArrayList;


No stars in imports

zregvart · 2017-05-27T18:02:28Z

controllers/src/main/java/io/syndesis/controllers/integration/online/ActivateHandler.java

@@ -49,7 +57,7 @@
    private final GitHubService gitHubService;
    private final ProjectGenerator projectConverter;

-    private final static Logger log = LoggerFactory.getLogger(ActivateHandler.class);
+    private static final Logger log = LoggerFactory.getLogger(ActivateHandler.class);


JLS declaration order

zregvart · 2017-05-27T18:02:48Z

controllers/src/main/java/io/syndesis/controllers/integration/online/DeactivateHandler.java

@@ -15,14 +15,17 @@
 */
 package io.syndesis.controllers.integration.online;

+import java.util.Arrays;


No stars in imports

zregvart · 2017-05-27T18:28:14Z

runtime/src/main/java/io/syndesis/runtime/EventBusToServerSentEvents.java

@@ -100,16 +100,19 @@ protected boolean reservationCheck(HttpServerExchange exchange) {
            responseHeaders.put(new HttpString(CorsHeaders.ACCESS_CONTROL_ALLOW_ORIGIN), origin);

            String value = requestHeaders.getFirst(CorsHeaders.ACCESS_CONTROL_REQUEST_HEADERS);
-            if (value != null)
+            if (value != null) {


zregvart · 2017-05-27T18:28:49Z

verifier/src/main/java/io/syndesis/verifier/LocalProcessVerifier.java

@@ -37,6 +47,8 @@
 public class LocalProcessVerifier {

    private final ProjectGenerator projectGenerator;
+
+    @SuppressFBWarnings("UWF_NULL_FIELD")
    private String localMavenRepoLocation = null; // "/tmp/syndesis-local-mvn-repo";


❓ only when debugging? always stays null?

zregvart · 2017-05-27T18:29:10Z

verifier/src/main/java/io/syndesis/verifier/LocalProcessVerifier.java

@@ -164,7 +176,7 @@ private String getConnectorClasspath(Connector connector) throws IOException, In
    }

    private String parseClasspath(InputStream inputStream) throws IOException {
-        BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream));
+        BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8));


charsets, so you don't shoot yourself in the foot

zregvart · 2017-05-27T18:30:53Z

This will be it for the first iteration of this. Please see the comments I provided to help you review. Thanks 🤗

pure-bot · 2017-05-28T17:39:36Z

⚠️ Status check continuous-integration/jenkins/pr-merge returned error.

This commit cannot be built

See https://jenkins-syndesis-ci.b6ff.rh-idev.openshiftapps.com/job/syndesis-rest/job/PR-377/5/display/redirect for more details.

pure-bot · 2017-05-28T17:49:36Z

⚠️ Status check continuous-integration/jenkins/pr-merge returned error.

This commit cannot be built

See https://jenkins-syndesis-ci.b6ff.rh-idev.openshiftapps.com/job/syndesis-rest/job/PR-377/6/display/redirect for more details.

This increases the time budget for running individual tests from 30s to 300s in an effort to allow the tests to run on the _internal_ (hosted on openshiftapps.com) Jenkins.

pure-bot · 2017-05-29T07:09:19Z

⚠️ Status check continuous-integration/jenkins/pr-merge returned error.

This commit cannot be built

See https://jenkins-syndesis-ci.b6ff.rh-idev.openshiftapps.com/job/syndesis-rest/job/PR-377/7/display/redirect for more details.

rhuss

Thanks, @zregvart for working on cleaning up the code base. I agree with most of the rules (except when they get too nit-picky lick JLS order enforcement).

BTW, for f-m-p and d-m-p we use a Sonarqube integration in order to achieve something similar.

From my POV this PR looks good and I'm +1 for merging it.

rhuss · 2017-05-29T07:37:07Z

controllers/src/main/java/io/syndesis/controllers/integration/IntegrationController.java

@@ -48,7 +56,7 @@
    private ExecutorService executor;
    private ScheduledExecutorService scheduler;

-    private final static long SCHEDULE_INTERVAL_IN_SECONDS = 60;
+    private static final long SCHEDULE_INTERVAL_IN_SECONDS = 60;


Not really a fan of enforcing JLS ordering as imo it doesn't add much value and is just nit picking.

rhuss · 2017-05-29T07:37:55Z

controllers/src/main/java/io/syndesis/controllers/integration/NoopHandlerProvider.java

@@ -15,9 +15,14 @@
 */
 package io.syndesis.controllers.integration;

-import java.util.*;
+import java.util.Arrays;


Actually I almost never look at imports so don't mind much how they look like. But I'm fine with any import rule as along as IDEA supports it.

zregvart · 2017-05-29T10:08:26Z

I don't think that the rules force any import ordering, just forbids usage of star imports. The only concern I have is the There was a timeout or other error in the fork error that is emitted from failsafe plugin on the internal Jenkins, I increased the default 30 sec timeout to 300 sec, but it did not help 😞. I think it might be caused by not having a local Maven repository volume or using a Maven repository manager -- so the downloads take a bit longer than on the other CI environments.

rhuss · 2017-05-29T10:53:09Z

I don't think that the rules force any import ordering, just forbids usage of star imports.

JLS ordering doesn't refer to the ordering of the imports but the order in which you provide 'public final static ...' modifiers. I always fail in remembering the "proper" order (as its not really important to me).

zregvart · 2017-05-29T11:19:20Z

On other projects (like Camel) there is a checkstyle rule that the imports need to be sorted in a certain way, I was commenting that this checkstyle configuration doesn't have that...

JLS ordering doesn't refer to the ordering of the imports but the order in which you provide 'public final static ...' modifiers. I always fail in remembering the "proper" order (as its not really important to me).

Yeah, I have those as muscle memory now, and my IDE (sorry Eclipse) fixes those auto magically if I put them out of JLS order, this might be a good alternative for IDEA intellij-plugin-save-actions or built in functionality. In Eclipse I have a radical cleanup on save setup, that will turn ugly unformatted code into code that looks like Juergen Hoeller wrote it :)

rhuss · 2017-05-29T11:57:54Z

Yeah, I have those as muscle memory now, and my IDE (sorry Eclipse) fixes those auto magically if I put them out of JLS order

I'm pretty sure that IntelliJ has something like this, too. I only question the value of this rule, which is tedious if it has to be fixed (causing an additional turn around). When I look at a declaration line I always read it from left to right and the order of the modifiers is of no value to me at all. It's just a rule for the rule's sake. But yeah, if we agree on it, I will be a good citizen then of course, too ;-)

KurtStam · 2017-05-31T14:24:06Z

Is this in addition to the productization base pom we use? I thought most of stuff is in there too?

zregvart · 2017-05-31T15:05:21Z

Is this in addition to the productization base pom we use? I thought most of stuff is in there too?

Not sure if the productization changes parent pom. Some of the stuff is like maven-jar-plugin configuration or source encoding, but most either isn't or isn't enforced: for instance dependency checks and code style checks, the test execution timeout; the commit message goes has a bit more about this (7a899ce).

I think that it is a good thing to have some checks in place (and base-pom provides properties to skip the ones we don't want) before the codebase gets too big, later it will be much more difficult. It will bring about some frustration at first, but it can be tweaked with (subjective) minimal effort, so my (subjective) opinion is that it's worth it. And I'm willing to carry this and and help with any issues that come up.

jimmidyson · 2017-06-05T11:11:23Z

retest this please

jimmidyson · 2017-06-05T11:13:49Z

I quite like this :) Considering the deprecation warning on https://github.com/sonatype/oss-parents, we should migrate to a different parent, but should we fork basepom into syndesisio org so we can use that as a parent rather than basepom directly? Should be pretty easy to keep up to date as we need to, but gives us the option to diverge if we need to as well. Thoughts @zregvart @rhuss? I'd like to get this resolved and merged today or tomorrow if possible and applied to all our Maven projects.

rhuss · 2017-06-06T09:46:48Z

@jimmidyson @zregvart I think we should only fork when we indeed require some customization. Otherwise its just more work.

But +1 for merging this PR as soon as possible.

zregvart · 2017-06-06T10:14:07Z

I don't think we would benefit much from our own fork., so I would not fork basepom, it's easy to tweak and override (IMHO), and we should probably just track it and upgrade to newer versions when convenient.

pure-bot · 2017-06-06T10:21:10Z

Pull request approved by @jimmidyson - applying approved label

jimmidyson · 2017-06-06T10:22:58Z

Merged - let's see how it goes and tweak as we need to going forward.

Thanks @zregvart!

jimmibot assigned KurtStam May 25, 2017

ghost assigned zregvart May 25, 2017

jimmibot requested a review from KurtStam May 25, 2017 12:51

ghost added the in progress label May 25, 2017

zregvart force-pushed the all_your_basepom_are_belong_to_us branch from 70af94c to e223bc6 Compare May 27, 2017 08:06

zregvart force-pushed the all_your_basepom_are_belong_to_us branch from e223bc6 to 7a899ce Compare May 27, 2017 17:53

zregvart commented May 27, 2017

View reviewed changes

zregvart changed the title ~~Use basepom as parent instead of Sonatype OSS - WIP~~ Use basepom as parent instead of Sonatype OSS May 27, 2017

zregvart force-pushed the all_your_basepom_are_belong_to_us branch from dcfa8ba to 73fc73b Compare May 28, 2017 17:41

refactor(maven): increase the test time budget

c91ed34

This increases the time budget for running individual tests from 30s to 300s in an effort to allow the tests to run on the _internal_ (hosted on openshiftapps.com) Jenkins.

zregvart force-pushed the all_your_basepom_are_belong_to_us branch from 73fc73b to c91ed34 Compare May 29, 2017 06:59

zregvart mentioned this pull request May 29, 2017

Code style #20

Closed

rhuss reviewed May 29, 2017

View reviewed changes

pure-bot bot added the approved label Jun 6, 2017

jimmidyson approved these changes Jun 6, 2017

View reviewed changes

pure-bot bot merged commit 3ef8ee9 into syndesisio:master Jun 6, 2017

zregvart deleted the all_your_basepom_are_belong_to_us branch June 6, 2017 11:19

rhuss mentioned this pull request Jun 22, 2017

Slow builds #400

Closed

Use basepom as parent instead of Sonatype OSS #377

Use basepom as parent instead of Sonatype OSS #377

Conversation

zregvart commented May 25, 2017

jimmibot commented May 25, 2017

pure-bot bot commented May 25, 2017

pure-bot bot commented May 25, 2017

pure-bot bot commented May 25, 2017

pure-bot bot commented May 25, 2017

rhuss commented May 26, 2017

zregvart commented May 26, 2017

pure-bot bot commented May 27, 2017

pure-bot bot commented May 27, 2017

pure-bot bot commented May 27, 2017

pure-bot bot commented May 27, 2017

Choose a reason for hiding this comment

zregvart May 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pure-bot bot commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zregvart May 27, 2017 • edited

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

zregvart May 27, 2017 • edited

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

zregvart commented May 27, 2017

pure-bot bot commented May 28, 2017

pure-bot bot commented May 28, 2017

pure-bot bot commented May 29, 2017

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

zregvart commented May 29, 2017

rhuss commented May 29, 2017

zregvart commented May 29, 2017

rhuss commented May 29, 2017

KurtStam commented May 31, 2017

zregvart commented May 31, 2017

jimmidyson commented Jun 5, 2017

jimmidyson commented Jun 5, 2017

rhuss commented Jun 6, 2017

zregvart commented Jun 6, 2017

pure-bot bot commented Jun 6, 2017

jimmidyson commented Jun 6, 2017

zregvart May 27, 2017 •

edited

zregvart May 27, 2017 •

edited

zregvart May 27, 2017 •

edited