Issue #157: SonarQube 7.2+ support #165

muhlba91 · 2018-11-15T06:48:11Z

Issue #157. Basic changes taken from #158, then I updated dependencies of Checkstyle and SonarQube API. I decided to only update SonarQube until 7.2 (7.3 also works) to keep compatibility with 7.2 SonarQube installations. However, I successfully tested analysis in SonarQube 7.3.

Apparently, there were quite a few API changes which meant I had to change the one or the other thing in the files and tests, which explains the huge number of changed files. In addition, I tried to clean up a few code parts using Java 8 language features.

Unfortunately, I wasn't able to get a few tests running (most likely because I wasn't really able to figure out what they are testing specifically and how to fix it) - they are now ignored:

CheckTests::verifyTestConfigurationFiles
CheckstyleExecutorTest::executeException

Furthermore, I integrated the upstream Checkstyle config but I need help in fixing the last errors which correspond to the import order. I wasn't able to figure out an IntelliJ setting that made the checks happy.

muhlba91 · 2018-11-15T06:57:11Z

Regarding the failing CI builds:

TeamCity: locally mvn test works without any issues
Wercker: please update the used SonarQube box to at least 7.2.
Travis: those are the Checkstyle issues with the wrong import order

romani · 2018-11-15T09:18:22Z

@muhlba91 , thanks a lot, please resolve conflict of rebase to let easily get your sources.
please squash all commits in one, it also helps.

muhlba91 · 2018-11-15T09:29:50Z

@romani rebased onto master as well as squashed commits into one.

rnveach · 2018-11-15T11:17:44Z

@muhlba91 TeamCity can't be run locally, so you will only see validation on server. It failed with the following:

Unable to load the mojo 'test' (or one of its required components) from the plugin 'org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M1'

It might just be a server issue. I tried re-running it.

Travis import order violations look valid. You should be able to reproduce by running mvn verify test locally. They will need to be fixed.

All CI should be green.

rnveach · 2018-11-15T11:19:57Z

I wasn't able to figure out an IntelliJ setting that made the checks happy.

Does this help any?
http://checkstyle.sourceforge.net/idea.html#Organize_Imports
If not, you may have to organize imports by hand unless someone else knows IntelliJ.

rnveach · 2018-11-15T11:27:27Z

@muhlba91 I peeked at the changes and I am seeing a ton of unnecessary changes that are not related to this PR.
I see formatting changes, putting final on method parameters, changing location of curlies from being on separate line to being on same line, changing non-lambda code to lambda code, reformatting existing code.

This many changes, some which should be wrong or unnecessary, will make this PR very hard to review.

rnveach

@romani We may need to relook at checkstyle setup in this project.
I don't understand why these changes aren't producing violations.

checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleAuditListener.java

checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleConfiguration.java

muhlba91 · 2018-11-15T14:26:26Z

@rnveach i updated as far as I was able to but I couldn't figure out why the tests run through without any issue the first time on Travis (and with local execution) but the second time (after instrumentation) with the error java.lang.NoClassDefFoundError: Could not initialize class org.sonar.java.JavaClasspath$$EnhancerByMockitoWithCGLIB$$42d68e21 and I think the missing coverage shown is caused by this issue. Can you take a look, please?

As for the Wercker build, please update the used box so that it's at least SonarQube 7.2, otherwise it fails.

romani · 2018-11-17T00:40:18Z

https://github.com/checkstyle/sonar-checkstyle/blob/master/wercker.yml#L1

@muhlba91 , please send separate PR for
https://github.com/checkstyle/sonar-checkstyle/blob/master/checkstyle-sonar-plugin/config/Dockerfile
I will merge it and push to dockerhub to let this PR pass.

muhlba91 · 2018-11-17T06:55:14Z

@romani please find the Docker update at #166

romani · 2018-12-08T14:41:28Z

@muhlba91 , please continue,
new image 7.4-comminity is at https://hub.docker.com/r/checkstyle/sonarqube-maven-git/tags/

muhlba91 · 2018-12-10T08:28:55Z

@romani thank you.

I have updated to pass the Travis builds now and merged in your branch for the update to 8.12 since I think I had to update as well (I can't remember the reason anymore, tbh).

However, the build error with wercker happens due to: wercker expects the container to run as root so that it can create the pipeline directory under /pipeline. Because the container is using a non-root user the pipeline can't run. I'd suggest removing this line: https://github.com/checkstyle/sonar-checkstyle/blob/master/checkstyle-sonar-plugin/config/Dockerfile#L27 to get the compatibility again?

rnveach · 2018-12-10T13:29:06Z

@muhlba91 As we are waiting on CI to pass, can you please fix up indentation changes in your PR. It is making it very hard to review your work to see what has really changed.

Please review your changes being listed under https://github.com/checkstyle/sonar-checkstyle/pull/165/files and make corrections where a change isn't necessary.

I am specifically talking about indentation changes in all the POMs.
Other changes like adding/removing lines, changing normal code to Java 8 compliant (lambda, Objects), isn't bad, but we prefer this type of work done in a separate commit/PR. It makes reviews alot easier.

muhlba91 · 2018-12-10T14:54:02Z

@rnveach done as much as possible.

romani · 2018-12-10T16:08:18Z

@muhlba91 , please remove extra changes, squash all in one commit, to let me review all from scratch.

muhlba91 · 2018-12-11T12:53:40Z

@romani changes minimized and squashed to one commit.

romani · 2018-12-11T13:55:48Z

Please remove upgrade to checkstyle 8.12 , it has to be done in separate update, PR already sent.

Please remove extra indentation.

Please move all updates for modern java style to separate commit/PR , I will merge them quickly. I am ok with improvements but not together (in the same commit) as functional change.

From wercker CI:

command is failed : mkdir -p "/pipeline"

Looks like the try to put something in our container, but previously there was root user, but now now main user is sonarqube. Investigation us required.

muhlba91 · 2018-12-13T11:32:48Z

Cleanup done.

Regarding the indentation: those should only be within the files I actually changed and are applied by the default IntelliJ formatting. Please provide an export of your formatting config because otherwise I'd have to mess around with my entire configuration to figure out how to configure it to match yours or I'd have to open the files with a normal editor to apply the same indentation for the methods.

Regarding Wercker: I mentioned this issue above. As far as I got to know from looking at Wercker (I have never heard of it until now tbh) they do not support moving this /pipeline directory somewhere else and neither do they support non-root containers. This is an open issue related to this: https://github.com/wercker/wercker/issues/229 - there's a workaround suggested but, first, as mentioned I have never used Wercker before and do not have enough time playing around with it, and, secondly, I am not sure how "clean" and "good" this workaround is, especially because you are using a custom made Docker image anyways. In fact, probably just leaving the container user as root might be a better "workaround".

rnveach · 2018-12-13T12:29:13Z

@romani I am not seeing anything at http://checkstyle.sourceforge.net/idea.html for disabling auto-formatting.

@muhlba91

Please provide an export of your formatting config because otherwise I'd have to mess around with my entire configuration to figure out how to configure it to match yours or I'd have to open the files with a normal editor to apply the same indentation for the methods.

Even if we had a configuration to give you, not everyone uses IntelliJ as their IDE. This would always cause difference issues as one person would always force the formatting to IntelliJ and the other we do what they want as long as Checkstyle doesn't complain. I personally use Eclipse, and I have no auto-formatting turned on because of this. I can manually format specific sets of code that I am writing, but I never run anything on the whole file.

I don't know IntelliJ to say how you should configure it and it doesn't seem to be documented on our site, but you will need to find a way to disable it for Checkstyle projects. I have seen other big projects encourage this too.

muhlba91 · 2018-12-13T13:00:23Z

@romani updated to remove indentation of methods not touched.

romani · 2018-12-16T17:07:04Z

I need to figure out wercker failure reason and fix it.

romani · 2018-12-20T15:28:40Z

probably just leaving the container user as root might be a better "workaround".

this cause sonarqube webservice to fail on start.

Looks like we need to CI for such testing ...

romani · 2018-12-26T05:18:11Z

@muhlba91 , please rebase on latest master code.
lets pass TC, while I am investigating how to fix wercker.

romani

items to improve:

checkstyle-sonar-plugin/pom.xml

romani · 2018-12-28T15:20:31Z

checkstyle-sonar-plugin/pom.xml

+    <dependency>
+      <groupId>com.puppycrawl.tools</groupId>
+      <artifactId>checkstyle</artifactId>
+      <version>${checkstyle.version}</version>


historically sonar plugin required plugin jar have a conflict with sonar over guava version (sonar used very old version of guava).
That is why we have "checkstyle-all" sub project.

Doe sit become not necessary any more ?

I have tried excluding the checkstyle-all subproject from all pom files and the entire plugin passes CI and manual tests on SQ 7.5 as well. Hence, I don't really see the purpose of the subproject.
Please check whether we can entirely remove it or additional changes are needed because of this.

checkstyle-sonar-plugin/pom.xml

romani · 2018-12-28T15:24:05Z

checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleConfiguration.java

    public static final String PROPERTY_GENERATE_XML = "sonar.checkstyle.generateXml";

    private static final Logger LOG = LoggerFactory.getLogger(CheckstyleConfiguration.class);

    private final CheckstyleProfileExporter confExporter;
    private final RulesProfile profile;
-    private final Settings conf;
+    private final org.sonar.api.config.Configuration conf;


Why full name is used ?
I do not see a conflict, please use import.

conflict is within already used com.puppycrawl.tools.checkstyle.api.Configuration.

romani · 2018-12-28T15:34:02Z

...style-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileExporter.java

        writer.append(filtersXml);

        writer.append(CLOSE_MODULE);
    }

    private boolean isSuppressWarningsEnabled() {
-        final String filtersXml = settings.getString(CheckstyleConstants.CHECKER_FILTERS_KEY);
+        final String filtersXml = settings.get(CheckstyleConstants.CHECKER_FILTERS_KEY)
+                .orElse(null);


in other parts of code you fight with null usage, but keep it here.
Please keep usage of Option as it comes from library, especially when context if this variable is very small.

I have kept it in order to require the minimal change of this method. As you have said, code cleanup regarding modern Java syntax/features should be done in a separate PR, I'd see such a change as a separate task.

...style-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileImporter.java

...style-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleRulesDefinition.java

pom.xml

rnveach · 2018-12-28T18:38:11Z

Please also rebase on latest master as there are conflicts.

romani · 2019-01-06T01:04:08Z

@muhlba91 , here is how to run your update on sonarqube 7.4 - #175 (comment), please have this update in your commit to switch circleci to 7.4.

but I insist to keep compilation dependency to be 6.7 , but check it in run time on CI on 6.7 and 7.4

Please proceed with your update, we appreciate your help. CI is ready for you.

ediri · 2019-01-06T11:02:59Z

looking forward for the new plugin! 👍

muhlba91 · 2019-01-09T09:41:18Z

@romani changes done and also tested manually on SQ 7.5. Please check my comments on the 3 three open conversations of your requested changes.

I have included the CircleCI changes but it doesn't seem like the CI is running for any push. Did you enable the integration for PRs?

Also, I have kept compatibility with the LTS release.

romani · 2019-01-11T04:42:12Z

Please close this PR, and create new, it should be hooked in circleci and make sure you rebase on latest master

muhlba91 mentioned this pull request Nov 15, 2018

Plugin doesn't work with SonarQube 7.3 #157

Closed

rnveach reviewed Nov 15, 2018

View reviewed changes

romani mentioned this pull request Dec 8, 2018

upgrade squid bridge #158

Closed

Tom-Van-Asch approved these changes Dec 16, 2018

View reviewed changes

romani changed the title ~~SonarQube 7.2+ support~~ Issue #157: SonarQube 7.2+ support Dec 16, 2018

romani requested changes Dec 28, 2018

View reviewed changes

#157 add support for SQ 7.2+

1ffe733

muhlba91 closed this Jan 11, 2019

muhlba91 mentioned this pull request Jan 11, 2019

#157 add support for SQ 6.7+ #186

Merged

Issue #157: SonarQube 7.2+ support #165

Issue #157: SonarQube 7.2+ support #165

Conversation

muhlba91 commented Nov 15, 2018 • edited Loading

muhlba91 commented Nov 15, 2018 • edited Loading

romani commented Nov 15, 2018

muhlba91 commented Nov 15, 2018

rnveach commented Nov 15, 2018 • edited Loading

rnveach commented Nov 15, 2018 • edited Loading

rnveach commented Nov 15, 2018

rnveach left a comment

Choose a reason for hiding this comment

muhlba91 commented Nov 15, 2018

romani commented Nov 17, 2018 • edited Loading

muhlba91 commented Nov 17, 2018

romani commented Dec 8, 2018

muhlba91 commented Dec 10, 2018 • edited Loading

rnveach commented Dec 10, 2018

muhlba91 commented Dec 10, 2018

romani commented Dec 10, 2018

muhlba91 commented Dec 11, 2018

romani commented Dec 11, 2018 • edited Loading

muhlba91 commented Dec 13, 2018

rnveach commented Dec 13, 2018 • edited Loading

muhlba91 commented Dec 13, 2018

romani commented Dec 16, 2018

romani commented Dec 20, 2018

romani commented Dec 26, 2018

romani left a comment

Choose a reason for hiding this comment

romani Dec 28, 2018

Choose a reason for hiding this comment

muhlba91 Jan 9, 2019

Choose a reason for hiding this comment

romani Dec 28, 2018

Choose a reason for hiding this comment

muhlba91 Jan 9, 2019

Choose a reason for hiding this comment

romani Dec 28, 2018

Choose a reason for hiding this comment

muhlba91 Jan 9, 2019

Choose a reason for hiding this comment

rnveach commented Dec 28, 2018

romani commented Jan 6, 2019

ediri commented Jan 6, 2019

muhlba91 commented Jan 9, 2019 • edited Loading

romani commented Jan 11, 2019 • edited Loading

muhlba91 commented Nov 15, 2018 •

edited

Loading

muhlba91 commented Nov 15, 2018 •

edited

Loading

rnveach commented Nov 15, 2018 •

edited

Loading

rnveach commented Nov 15, 2018 •

edited

Loading

romani commented Nov 17, 2018 •

edited

Loading

muhlba91 commented Dec 10, 2018 •

edited

Loading

romani commented Dec 11, 2018 •

edited

Loading

rnveach commented Dec 13, 2018 •

edited

Loading

muhlba91 commented Jan 9, 2019 •

edited

Loading

romani commented Jan 11, 2019 •

edited

Loading