Sonarcloud won't show issues discovered by golangci-linter

vbortkevic · December 27, 2021, 1:33pm

Template for a good new topic, formatted with Markdown:

ALM used: GitHub
CI system used: Github Actions
Languages of the repository: Go
Error observed:
We have Github action to run golangci-linter for code like this:
golangci-lint run --new-from-rev=${{ github.event.pull_request.base.sha }} --whole-files --out-format checkstyle > report.out
This report then is passed to Sonarcloud in properties file:

#Path with golangci-lint discovered issues
sonar.go.golangci-lint.reportPaths=cloudwaste.report.out

In golangci-linter configuration file we have separated some linters to produce severity=“error”, and some severity=“info” only:

<?xml version="1.0" encoding="UTF-8"?>
<checkstyle version="5.0">
  <file name="path/to/folder/aws.go">
    <error column="2" line="78" message="`awsSecurityGroupSubtype` is unused" severity="info" source="deadcode">
    </error>
>
  </file>
  <file name="path/to/interaction_resultprocessor.go">
    <error column="3" line="61" message="Error return value is not checked" severity="error" source="errcheck">
    </error>
    <error column="0" line="8" message="File is not `gci`-ed with -local github.com/organisation/project" severity="info" source="gci">
    </error>
  </file>
</checkstyle>

However nor errors with severity=“error” neither with severity=“info” are not shown Sonarcloud on PR analysis

Only one Smell as Minor issue is discovered on this error:
<error column="0" line="2" message="line is 248 characters" severity="info" source="lll">

Can you please explain how to properly setup Sonarcloud and golangci-linter so they can work together and produce proper report which can be helpful for developers?

Thank you.

vbortkevic · December 27, 2021, 1:56pm

Ok, after creating this post I found similar here Change external issues language, type and severity - #6 by Quentin which points to this ticket https://jira.sonarsource.com/browse/SONARSLANG-471
But it seems nothing have been improved since…
Is there some possibility some more precision mappings will be introduced?
On the other hand, this:

To give you more details, in the case of GolangCILint (and all Checkstyle format), issues with severity=error will have type bug, and code smell otherwise, and issues with severity=info will have severity minor and major otherwise.

Is not works as expected:
In my report I have 30 severity=“info” and only one shown on sonarcloud (mentioned in my post before) and one severity=“error” not present on report at all…

Sylvain_Combe · December 27, 2021, 2:29pm

Hi @vbortkevic
I’m not very familiar with golangci-lint but I understand the --whole-files parameter as causing the tool to report issues that are outside the scope of the PR (new and changed lines). From there (many) issues in the report will not get imported by SonarCloud as it focuses precisely on the PR scope.
Can you make sure you are not in this case and let me know?

And indeed, the golangci linter report parser applies a mapping logic to the found issues. As the reports lack information for a perfect fit in SonarCloud, the mapping cannot be perfect.
Many linters natively support the SonarSource Generic Issue format, maybe you can promote it with the golangci linter team.

vbortkevic · December 27, 2021, 2:58pm

Thank you Sylvain for response. The flag --whole-files is to scan whole file where the changes were made by developer (not only lines of code he writes recently) therefore this is not the case.

It is however unclear to me how should I declare linters so they will be displayed on Sonarcloud report as bugs?
Currently on the PR my linter discovers and passes to sonarcloud 3 errors (severity=“error”) and 33 info (severity=“info”). But on sonarcloud dashboard I can see only 3 info’s and none error:

Sylvain_Combe · December 27, 2021, 3:10pm

That was my point. Many issues in your report may not be imported by SonarCloud because they are not part of the PR (since you used the --whole-files parameter).
if you analyze a PR, SonarCloud will not import any issue outside of the PR scope, highlighted in yellow in SonarCloud UI:

vbortkevic · December 27, 2021, 3:31pm

But previously when we had not separated severity into ERROR and INFO and all golangci linters reported with severity=“error” it worked just fine: same flag --whole-files didn’t cause any issues.
Each issue was market as a bug.

But then we changed behavior to do not misleading developers and introduced two levels: Error and Info.
Now it behaves randomly: on Sonarcloud I can see only few info’s as Code smell and no bugs at all…

Sylvain_Combe · December 28, 2021, 5:23pm

Hi @vbortkevic
can you share a screenshot showing some New Code with no issue imported, and the snippet from the golangci-linter listing at least one issue in this code area?

vbortkevic · December 29, 2021, 12:21pm

Hi @Sylvain_Combe,
I’m not sure what you mean by this:

screenshot showing some New Code with no issue imported, and the snippet from the golangci-linter listing at least one issue in this code area

Are we’re looking for New Code in the file, which produce no issues. But after linting with golangci-linter there are some issues?
If so, then it sounds like a nonsense, because clean code will not produce issues on linting.
What I’m looking for is: since file is modified (even new code is clean) and --whole-files present, then show all discovered issues imported from golangci-linter grouped by severity…

Sylvain_Combe · December 29, 2021, 1:36pm

Hi @vbortkevic
as I mentioned above, no issue will ever get imported outside of the New Code area (highlighted in yellow) for a PR analysis . Even if the external report has dozens of issues, if those issues are not attached to this new code area, they will not be imported by SonarQube.
When you analyze a PR, it does not make sense to import issues outside of the changed code area, does it?

If you need a report on all areas of your code, you must run a branch analysis.

vbortkevic · December 29, 2021, 1:40pm

Hi @Sylvain_Combe
yeah, looks like I understood what you are saying…
I have removed the --whole-files flag and now linter check for new code only.

Then the same 7 issues (1 error and 6 info) are shown on Sonarcloud dashboard.

However my question is still open: Can we somehow have option to use this --whole-files flag and import golangci-linter issues to be shown on PR even those isses are not related to new code?

Thank you!

Sylvain_Combe · December 29, 2021, 1:53pm

Hi @vbortkevic

However my question is still open: Can we somehow have option to use this --whole-files flag and import golangci-linter issues to be shown on PR even those isses are not related to new code?

No it is not possible. And we believe it would be wrong to allow this as it might break the whole Clean As You Code idea.
In your particular case, I believe the underlying idea is to slightly enlarge the New Code period to include all touched files, forcing developers to address some or all the legacy debt in those files.
Can you confirm?

If that’s the case, I would advise against it. You can expect developers to understand how it works (and how to sidestep it) very rapidly, and application design would suffer.

vbortkevic · December 29, 2021, 2:00pm

Hi @Sylvain_Combe ,
our intention was to use ‘boyscout rule’: ’ Always leave the code better than you found it forcing developers to make code better and cleaner over the time…

vbortkevic · January 3, 2022, 9:30am

Hi @Sylvain_Combe,
Happy New Year!
It looks like Sonarcloud imports issues in some strange order.
Below is example, where I have two info issues:

  ::info file=path/to/file/recommendation.go,line=253::File is not `gofumpt`-ed (gofumpt)
  ::info file=path/to/file/recommendation.go,line=264,col=3::return statements should not be cuddled if block has more than two lines (wsl)

and then first issue was imported as code smell, where second one (wsl) was imported as bug.
Can you please explain why it happens so?
Thank you!

Sylvain_Combe · January 3, 2022, 10:02am

Hi @vbortkevic
and happy new year as well.
That is quite strange (and not in line with the import implementation), would you be able to share a reproducer?

vbortkevic · January 3, 2022, 10:37am

Hi,
not sure what you mean reproducer?

Sylvain_Combe · January 3, 2022, 10:45am

Some code that I could use to reproduce the problem.

vbortkevic · January 3, 2022, 11:47am

Hi, you can check with something like this, where number is used instead of some variable:

func splitSkuName(name string) string {
	var skuName string
	splitSkuName := strings.Split(name, "_")
	if len(splitSkuName) >= 3 {
		skuName = fmt.Sprintf("%s %s", splitSkuName[1], splitSkuName[2])
	} else {
		skuName = fmt.Sprintf(splitSkuName[1])
	}
	return strings.TrimSpace(skuName)
}

This then is discovered as mnd:Magic number by golangci-lint as Bug

Where this one line:

savingsAmount := monthlyForecast.Add(targetModCurrentPrice.Mul(monthlyForecast)).Round(2)

Have been treated as Code smell.
I have attached two pictures showing the same PR where the same issues like mnd:Magic number, return statements should not be cuddled if block has more than two lines, only one cuddle assignment allowed before if statement are shown on sonarcloud report as Bug, others as Code Smell.
Is this expected behavior?

In this case only one Bug should be displayed (not 16):
Error: should rewrite http.NewRequestWithContext or add (*Request).WithContext (noctx)

Sylvain_Combe · January 3, 2022, 4:53pm

Hi @vbortkevic
I think I caught something.
After the project main branch has been analyzed with some issues categorized as bugs, they stay categorized as such whatever the changes applied to branch (or PR?) analysis, and even after the branch has been ‘cleaned’ with a new analysis.

I need to discuss this internally and will come back to you.

Sylvain_Combe · January 5, 2022, 10:28am

Hi @vbortkevic
the language team has looked into this. The mapping logic applied to golancici-lint reports sets an issue type only once per source (the linter name with those reports) and will then stick to it with subsequent analysis (and across branches in some cases). This report source is the identifier mapped to the SonarQube rule key.

This leads to all issues from the same “source”, i.e. linter to be assigned the same issue type, with cross branch and PR mechanisms making changes difficult to predict (and follow).
So if you really need to distinguish different issues from the same linters into code smells and bugs, at this stage a work around could be to append the intended severity to the linter name in the reports.

We may help with an improvement at some point, but we don’t have any ETA. I’ll update you when I have news.

And I would add a final note about this PR analysis scope initial topic with a link to Clean as You Code: How to win at Code Quality without even trying

Best
Sylvain

Quentin · January 11, 2022, 12:48pm

Hello @vbortkevic and @Sylvain_Combe

Thanks for the investigation and discussion.

The information we have in this report is indeed limited, but we should be able to do something to improve this inconsistency. Ticket created: SONARSLANG-548.

Best,
Quentin