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.

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

Add sonarqube output #288

Merged
merged 7 commits into from
Mar 20, 2019
Merged

Add sonarqube output #288

merged 7 commits into from
Mar 20, 2019

Conversation

kmcrawford
Copy link
Contributor

For Issue #287

output/sonarqube_format.go Outdated Show resolved Hide resolved
output/sonarqube_format.go Outdated Show resolved Hide resolved
@kmcrawford kmcrawford closed this Mar 11, 2019
@kmcrawford kmcrawford reopened this Mar 11, 2019
@kmcrawford kmcrawford closed this Mar 11, 2019
@kmcrawford kmcrawford reopened this Mar 11, 2019
@kmcrawford
Copy link
Contributor Author

kmcrawford commented Mar 11, 2019

travis keeps failing for silly go get network issues.
The command "go get -u golang.org/x/lint/golint" failed and exited with 1 during .

@gcmurphy
Copy link
Member

The latest travis-ci run fails on the tip: https://travis-ci.org/securego/gosec/jobs/504930446. gosec runs against itself, and on the tip it seems like a package is missing (which is triggering failure):

could not import golang.org/x/sys/cpu (cannot find package "golang.org/x/sys/cpu" in any of..

Likely introduced by golang/go#24843

So I'm ok with merging this with the ci job failure.

cc/ @MVrachev

Copy link
Member

@gcmurphy gcmurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting a PR! 🍺

There are a couple of things we might look at tweaking but looks fairly good.

/cc @ccojocar @MVrachev what are your thoughts around adding elapsed time and truncating file paths in issue output?

@@ -165,19 +165,19 @@ func loadRules(include, exclude string) rules.RuleList {
return rules.Generate(filters...)
}

func saveOutput(filename, format string, issues []*gosec.Issue, metrics *gosec.Metrics, errors map[string][]gosec.Error) error {
func saveOutput(filename, format, rootPath string, issues []*gosec.Issue, metrics *gosec.Metrics, errors map[string][]gosec.Error) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure modifying the interface to achieve a truncated file path is the best solution here. Is there any specific reason for it? Sonarqube only recognize relative paths?

This is certainly something we can look at fixing though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sonarqube reports from the root of the project. I needed a way to remove the full path. Happy to refactor, if you can think of a better way of doing this.

Copy link
Contributor Author

@kmcrawford kmcrawford Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example of this working:
image

From:

                {
			"engineId": "gosec",
			"ruleId": "G505",
			"primaryLocation": {
				"message": "Blacklisted import crypto/sha1: weak cryptographic primitive",
				"filePath": "service/caching.go",
				"textRange": {
					"startLine": 5,
					"endLine": 5
				}
			},
			"type": "VULNERABILITY",
			"severity": "MAJOR",
			"effortMinutes": 5
		}

},
Type: "VULNERABILITY",
Severity: getSonarSeverity(issue.Severity.String()),
EffortMinutes: 5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why hardcode a value here? We could potentially actually capture time elapsed here as part of the reporting process. (start_time, finish_time) to the stats area.

Copy link
Contributor Author

@kmcrawford kmcrawford Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, I just didn’t want to change your project too much when I actually have no idea how long it will take to fix. Since this is used to calculate technical debt in sonarqube, as in how long will it take to fix. 5 minutes seems better than zero, but could be hours since I wouldn't know the impact of the change to the users code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this is an estimation for developers which indicates how long it would take to fix the issue. In this case, I don't see how we can measure it, therefore I am fine with a constant. Maybe in future, it can be defined per issue for a more granular estimation.

I would extract this in a constant with a clear meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

output/formatter.go Outdated Show resolved Hide resolved
output/formatter.go Outdated Show resolved Hide resolved
output/formatter.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 13, 2019

Codecov Report

Merging #288 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #288   +/-   ##
======================================
  Coverage    56.3%   56.3%           
======================================
  Files           9       9           
  Lines         492     492           
======================================
  Hits          277     277           
  Misses        188     188           
  Partials       27      27

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bd007e...ba566ec. Read the comment docs.

@kmcrawford
Copy link
Contributor Author

All open requests have been updated.

@kmcrawford
Copy link
Contributor Author

bump

Copy link
Member

@gcmurphy gcmurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good addition thanks!. I don't have access to sonarqube to make sure the integration works but have verified the output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants