Extreme make over #17

filipesperandio · 2017-10-04T21:05:38Z

I recommend reviewing by navigating the branch instead of looking at diffs as this is a complete new structure.

New tooling
Bring released versions of sonarlint-cli, sonarlint-core and sonarlint-client-api
Apply extensions on top of them
Keep only files that matter
Sanity Check Test

Related: qltysh/qlty#746

…support newer attributes and are compatible with current sonarlint-cli implementation

…tever we need

…-java into fe/new-structure

dblandin · 2017-10-06T15:19:35Z

README.md

-
-Copyright 2016-2017 SonarSource.
-
-Licensed under the [GNU Lesser General Public License, Version 3.0](http://www.gnu.org/licenses/lgpl.txt)


Shouldn't we keep a README.md file?

dblandin · 2017-10-06T15:20:20Z

NOTICE.txt

-mailto:info AT sonarsource DOT com
-
-This product includes software developed at
-SonarSource (http://www.sonarsource.com/).


I'm not sure of the licensing details but we may be required to keep this notice?

dblandin · 2017-10-06T15:21:40Z

Makefile

+
+test: image
+	docker run --rm -ti -w /usr/src/app -u root $(IMAGE_NAME) ./gradlew clean test
+


💄 extra line

dblandin · 2017-10-06T15:32:26Z

src/main/java/cc/JsonReport.java

+                    }
+                } else {
+                    lines.addProperty("begin", 1);
+                    lines.addProperty("end", 1);


Noting that this is not new, and was in the code before this PR

This seems a little risky. If we don't have a reliable location information for an issue, we probably shouldn't output it at all.

Do we know of issues that should apply generally to the file and not to a specific block of source code?

Wondering if we should just emit a warning here instead of emitting an issue with bad data.

@dblandin Yeah, I am not very happy with this class either.
I am creating an gh-issue with these points and will address those in another PR, WTDY?

Sounds good 👍

Would you mind commenting here with that issue link when it's available?

Issue: https://github.com/codeclimate/sonar-java/issues/19

dblandin · 2017-10-06T15:33:20Z

src/main/java/cc/JsonReport.java

+                JsonObject content = new JsonObject();
+                json.add("content", content);
+                content.addProperty("body", ruleDetails.getHtmlDescription());
+                // // ruleDetails.getExtendedDescription();


What does #getExtendedDescription contain? Do we still need this comment?

dblandin · 2017-10-06T15:57:43Z

fixtures/java_lib/main/java/Library.java

+			}
+		}
+	}
+}


We can probably drop this generated file.

This is fixture for testing.

Ahh, cool cool 👍

dblandin · 2017-10-06T15:58:12Z

fixtures/java_lib/test/java/LibraryTest.java

+        Library classUnderTest = new Library();
+        assertTrue("someLibraryMethod should return 'true'", classUnderTest.someLibraryMethod());
+    }
+}


We can probably drop this generated file.

dblandin · 2017-10-06T15:59:57Z

gradlew

@@ -0,0 +1,172 @@
+#!/usr/bin/env sh


Commented on the Dockerfile as well but I'm wondering if we can use the package manager for the gradle install instead of committing this script.

dblandin · 2017-10-06T17:23:52Z

src/main/java/org/sonarlint/cli/CustomMain.java

+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+package org.sonarlint.cli;


Is this the right package for this class? Should it com.codeclimate.engine or something like that? Doesn't seem like it's meant to be part of sonarlint itself.

Only put this here because of some package protected methods we need access.
Already working to improve this in another branch, if it is OK to leave it as is for now.

Ahh, interesting. Yeah, in that case it's probably fine to leave as is. Would be great to minimize dependencies on protected methods in general.

dblandin · 2017-10-06T17:26:03Z

src/main/java/org/sonarlint/cli/CustomMain.java

+import static org.sonarlint.cli.SonarProperties.PROJECT_HOME;
+
+
+public class CustomMain extends org.sonarlint.cli.Main {


I'm wondering if we can pick a better name than CustomMain. Maybe Engine if this is meant to be the engine wrapper around sonarlint?

What about SonarlintWrapper?

Sure, that sounds good to me. Maybe even SonarlintEngineWrapper to be more specific.

filipesperandio · 2017-10-09T20:18:29Z

@dblandin Thanks for the feedbacks:

Dockerfile changes include gradle installation and project dependencies caching
Removed gradlew files
No more /code-read-write
NOTICE/LICENSE/README are back
CustomMain: saving the renaming to another PR as the other branch already has a bunch of changes
Fixed WORKDIR /code

Appreciate another look at it if you have time!

dblandin · 2017-10-09T20:24:15Z

@filipesperandio Are there commits that haven't been pushed up yet?

I still see references to /code-read-write and I'm not seeing the gradle installation.

filipesperandio · 2017-10-09T21:15:17Z

@dblandin My bad. Everything pushed now.

dblandin · 2017-10-09T21:24:30Z

Dockerfile

-  /usr/src/app/dest/bin/sonarlint \
-  --src 'src/main/**/*.java'
+WORKDIR /code
+CMD cp -R /code /tmp/ && /usr/src/app/build/codeclimate-sonar /tmp/code


It looks like sonarlint supports a SONARLINT_USER_HOME environment variable. Could that be used to redirect the tool's output to another directory, leaving the source code under analysis untouched in /code?

SonarSource/sonarlint-core@220729b#diff-fc98b7a351de15143748ac690d4fc601

Trying to find a way to avoid this cp -R workaround.

@dblandin Didn't work. That reference seems outdated.
I think we can override it somewhere, but I would like to approach this in a separate PR as well. If it is OK for you?

Issue: https://github.com/codeclimate/sonar-java/issues/20

Works for me 👍

filipesperandio added 18 commits October 3, 2017 15:34

Initial commit.

8dfe9e2

Remove unecessary files

61aa0ab

Ignore .gradle and IntelliJ files

7708991

Use specific versions of sonarlint-core and sonarlit-client-api that …

6f85636

…support newer attributes and are compatible with current sonarlint-cli implementation

Override Sonarlint's classes to add CC's JsonReport

21d6b53

Give classpath priority to sonalint-core and sonarlint-client-api

02ac0f5

Keep license header

6bdd3d5

Move log messages to stderr

174fecf

Give app a proper name

115ebd1

Make app classes first priority on classpath - so we can override wha…

c38ac39

…tever we need

Docker setup

c634643

Save a few MBs on docker image

359b560

Optimize docker layers by caching project dependencies

7e908b8

Add sanitity check test

0de0f6f

Add CircleCI config

edb0c7e

Remove files for restructure

27cec4a

Merge branch 'master' of github.com:filipesperandio/codeclimate-sonar…

b361a70

…-java into fe/new-structure

Smarter ignores

f79ebde

filipesperandio requested review from nporteschaikin and brynary October 4, 2017 21:06

filipesperandio added 2 commits October 4, 2017 18:08

Sonar needs writable directory on alaysis, keep it documented

d865975

Fix problem with gradle base Docker image using its parent instead

803d487

filipesperandio requested a review from codeclimate-hermes October 6, 2017 12:31

codeclimate-hermes requested review from ABaldwinHunter and removed request for codeclimate-hermes October 6, 2017 12:31

dblandin reviewed Oct 6, 2017

View reviewed changes

filipesperandio added 4 commits October 6, 2017 18:19

Remove extraline

2e71f79

Bring back NOTICE/LICENSE/README

f1ce0da

Dockerfile improvements

7a323c5

Improve dependencies caching

3c80af9

Remove gradle wrapper

77c76a0

filipesperandio force-pushed the fe/new-structure branch from d662d96 to 77c76a0 Compare October 9, 2017 21:14

dblandin reviewed Oct 9, 2017

View reviewed changes

dblandin approved these changes Oct 9, 2017

View reviewed changes

filipesperandio merged commit 6beeb56 into channel/beta Oct 10, 2017

filipesperandio deleted the fe/new-structure branch October 13, 2017 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extreme make over #17

Extreme make over #17

filipesperandio commented Oct 4, 2017 •

edited

Loading

dblandin Oct 6, 2017

dblandin Oct 6, 2017

dblandin Oct 6, 2017

dblandin Oct 6, 2017

filipesperandio Oct 6, 2017

dblandin Oct 9, 2017 •

edited

Loading

filipesperandio Oct 9, 2017

dblandin Oct 6, 2017

dblandin Oct 6, 2017

filipesperandio Oct 6, 2017

dblandin Oct 9, 2017

dblandin Oct 6, 2017

dblandin Oct 6, 2017

dblandin Oct 6, 2017

filipesperandio Oct 6, 2017

dblandin Oct 9, 2017

dblandin Oct 6, 2017

filipesperandio Oct 6, 2017

dblandin Oct 9, 2017

filipesperandio commented Oct 9, 2017

dblandin commented Oct 9, 2017 •

edited

Loading

filipesperandio commented Oct 9, 2017

dblandin Oct 9, 2017

filipesperandio Oct 9, 2017

filipesperandio Oct 9, 2017

dblandin Oct 9, 2017


		Copyright 2016-2017 SonarSource.

		Licensed under the [GNU Lesser General Public License, Version 3.0](http://www.gnu.org/licenses/lgpl.txt)


		test: image
		docker run --rm -ti -w /usr/src/app -u root $(IMAGE_NAME) ./gradlew clean test

		import static org.sonarlint.cli.SonarProperties.PROJECT_HOME;


		public class CustomMain extends org.sonarlint.cli.Main {

Extreme make over #17

Extreme make over #17

Conversation

filipesperandio commented Oct 4, 2017 • edited Loading

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

dblandin Oct 9, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipesperandio commented Oct 9, 2017

dblandin commented Oct 9, 2017 • edited Loading

filipesperandio commented Oct 9, 2017

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

filipesperandio commented Oct 4, 2017 •

edited

Loading

dblandin Oct 9, 2017 •

edited

Loading

dblandin commented Oct 9, 2017 •

edited

Loading