Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gitlab codeclimate improvements #123

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Gitlab codeclimate improvements #123

wants to merge 10 commits into from

Conversation

baaym
Copy link
Contributor

@baaym baaym commented Mar 30, 2018

Hello!

I recently started using this plugin on our GitLab EE instance, and in general it works like a charm. When I configured the CodeClimate report, things were less smooth unfortunately.

On our main branch we have SonarQube running in publish mode. In publish mode, this plugin only handles issues that are reported as new by SQ. This means that in our main branch the CodeClimate file never contains all issues. In preview mode, which is being used by merge requests, all issues are correctly reported.

GitLab compares both CodeClimate files when showing the merge request window. So when the full CodeClimate file in the merge request is compared to the near-empty one in the main branch, you can imagine the shock ;)

The goal of this merge request is to add support for writing all issues to the CodeClimate file, regardless of operating mode. I tried to do this by introducing a new property, called sonar.gitlab.json_all_issues. Basically all behavior is the same, except when you set that property to true and you run in publish mode. In that case you will get all issues in the JSON file, while still reporting only new issues as comments.

Unfortunately this required some poking around in unfamiliar code. There are a few things I'm not really proud of - for example the Reporter.build() method signature change to name one. Perhaps it's not all that bad, that's for the maintainers to judge ;)

Please let me know what you think!

Regards,
Martijn

@gabrie-allaigre
Copy link
Owner

Hi,
Thanks,
I look at what you did.

@baaym
Copy link
Contributor Author

baaym commented Apr 9, 2018 •
edited
Loading

Hi @gabrie-allaigre,

I tested this on our SonarQube instance, and unfortunately there's an error I will have to look at:

[INFO] Executing post-job GitLab Commit Issue Publisher
[WARNING] Property 'sonar.gitlab.commit_sha' is not declared as multi-values/property set but was read using 'getStringArray' method. The SonarQube plugin declaring this property should be updated.
[ERROR] Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.4.0.905:sonar (default-cli) on project xxx: SonarQube failed to complete the review of this commit: Issues are only available to PostJobs in 'issues' mode. -> [Help 1]

I think the warning is the result of some other changes in the master branch.

Regards,
Martijn

@gabrie-allaigre
Copy link
Owner

Hi,
In
PropertyDefinition.builder(GITLAB_COMMIT_SHA).name("GitLab Commit SHA").description("The commit revision for which project is built.").category(CATEGORY) .subCategory(SUBCATEGORY).index(6).hidden().build()

Add .multiValues(true)

@baaym
Copy link
Contributor Author

baaym commented Apr 14, 2018

Hi @gabrie-allaigre,

I reverted a lot of changes since I found out that the check for new issues was not done from the CommitPublishPostJob, but inside ReporterBuilder.getStreamIssue(). I think I got confused due to the sonarFacade.getNewIssues() call in the CommitPublishPostJob. This method doesn't get new issues at all, it gets all issues. Filtering happens later inside the ReportBuilder. Perhaps it's good to change the name of SonarFacade.getNewIssues()?

The merge request should be complete now. I will test these changes in the office on Monday, and will let you know what comes out. After that, and if the merge request is approved, I think it's a good idea to squash all these commits ;)

@baaym
Copy link
Contributor Author

baaym commented Apr 16, 2018

Hi @gabrie-allaigre,

The plug-in runs without errors or warnings now. However, the sonarFacade.getNewIssues() method returns 0 issues. I see it was recently changed in Master when updating to SQ 7.0. Perhaps a bug was introduced?

@gabrie-allaigre
Copy link
Owner

Hi,
sonarFacade.getNewIssues() return issues for ref_name branch only.

@baaym
Copy link
Contributor Author

baaym commented Apr 18, 2018

Perhaps that's the issue. I think in the Community Edition of SonarQube the branch identifier is deprecated since 6.6. See: https://docs.sonarqube.org/display/SONAR/Analysis+Parameters#AnalysisParameters-ProjectConfiguration.1

That means that projects that supply a sonar.branch property are probably not registered as that branch. My projectname is automatically appended with the branch name (develop), however the actual branch identifier in the GUI shows master.

Perhaps the plugin should not rely on the actual GitLab branch name to do the searching in SonarQube. Does the SonarQube API provide info on which SonarQube branch is currently being analysed?

@baaym
Copy link
Contributor Author

baaym commented Apr 26, 2018

Hi @gabrie-allaigre,

I'm abandoning this pull request. I made a change internally so the proper branch was selected. Unfortunately the results coming from the search in publish mode deviate too much from the results coming from the preview mode. I think that if one wants to use SonarQube to generate CodeClimate files for GitLab EE, only preview mode should be used on all branches.

The changes in this pull request can still be useful for others though, so feel free to merge anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@baaym @gabrie-allaigre