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

Integrate SARIF report with Github code scanning #3359

Merged
merged 3 commits into from
Jan 11, 2021

Conversation

chao2zhang
Copy link
Member

@chao2zhang chao2zhang commented Jan 8, 2021

This should wrap up basic support for #3045. This PR integrates SARIF output with Github code scanning by adding a new GitHub workflow using detekt without type resolution.

  • Fix a bug in KtCompiler.kt that we need to yield the absolute path of the base path.
  • Use %SRCROOT% as uriBaseId. In SARIF support for coding scanning, %SRCROOT% was used as an example, which seems to be the only way that works with Github Action. If we pass the base path based on the local directory, like "uriBaseId": "/Users/chaozhang/detekt", it will not be consumed by Github.
  • In the screenshot, we are showing duplicate messages. This is because not each CodeSmell defines its own message. It will fallback to issue description if no message is specified, which is exactly the case in the manual test.

Follow up

  • Once this PR is merged, I will add a specific page in doc on integrating with Github actions.
  • Type resolution is still experimental that I see errors in the log. Running detekt with type resolution will also generate one SARIF per source set and require Github work flow to upload multiple SARIF files, instead of one file. Given these complications, I am only using the SARIF output for detekt run without type resolution in this PR.
  • Once there is a new detekt version released, I can change the option to enable the SARIF output by default + to enable relative path by default given the project root. (If we resolve Setup the correct pipeline for integration test of detekt gradle plugin #3324, we no longer need to wait for new detekt versions to be released).

Test

See the PR on my own fork of detekt, you can find GitHub codescanning like the following:
image

@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #3359 (c87b80a) into master (a17e332) will increase coverage by 0.00%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3359   +/-   ##
=========================================
  Coverage     80.27%   80.27%           
  Complexity     2717     2717           
=========================================
  Files           445      445           
  Lines          8238     8245    +7     
  Branches       1563     1565    +2     
=========================================
+ Hits           6613     6619    +6     
  Misses          785      785           
- Partials        840      841    +1     
Impacted Files Coverage Δ Complexity Δ
...tlab/arturbosch/detekt/core/reporting/Reporting.kt 94.11% <ø> (ø) 0.00 <0.00> (ø)
.../main/kotlin/io/github/detekt/parser/KtCompiler.kt 77.27% <0.00%> (-4.55%) 8.00 <0.00> (ø)
...etekt/core/tooling/ProcessingSpecSettingsBridge.kt 92.85% <50.00%> (-7.15%) 0.00 <0.00> (ø)
...io/github/detekt/report/sarif/SarifOutputReport.kt 94.73% <90.00%> (+4.11%) 4.00 <0.00> (ø)
...n/kotlin/io/github/detekt/report/sarif/SarifDsl.kt 100.00% <100.00%> (ø) 0.00 <0.00> (ø)

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 a17e332...0cec1b8. Read the comment docs.

@@ -17,3 +17,5 @@ txt:./build/detekt-report.txt
sarif:./build/detekt-report.sarif
-p
./build/detekt-formatting.jar
-bp
.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good default value for the cli, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am assuming either

  1. When passing -bp without arguments, use the user.dir as the base path
  2. When not passing -bp, change the default behavior to output relative paths

I like 2, but not sure if it will be too breaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Adding an option in the detekt config file to specify the behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that if you don't set -bp the working dir is used by default. This way all the reports will use relative paths. I think that all of them should, those files should be sharables so it have little sense to have a full path. (Again this is out of scope of the PR but just ideas about how we could improve this new feature)

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it is a good idea to open an issue to document all things to follow up? This PR is one of the working stream.

Copy link
Member Author

@chao2zhang chao2zhang Jan 8, 2021

Choose a reason for hiding this comment

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

#3360 was created to track the follow-up issues. There are many good suggestions like this so I think we need at least a single place to document them.

.github/workflows/detekt-with-type-resolution.yaml Outdated Show resolved Hide resolved
.github/workflows/detekt-with-type-resolution.yaml Outdated Show resolved Hide resolved
@chao2zhang chao2zhang marked this pull request as draft January 8, 2021 14:59
@@ -17,3 +17,5 @@ txt:./build/detekt-report.txt
sarif:./build/detekt-report.sarif
-p
./build/detekt-formatting.jar
-bp
.
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that if you don't set -bp the working dir is used by default. This way all the reports will use relative paths. I think that all of them should, those files should be sharables so it have little sense to have a full path. (Again this is out of scope of the PR but just ideas about how we could improve this new feature)

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

👏

@schalkms
Copy link
Member

schalkms commented Jan 8, 2021

I don't quite understand the following comment. Could you please elaborate?
What happens with enabled type resolution?
What happens with disabled type resolution?

Type resolution is still experimental that I see errors in the log. Running detekt without type resolution will also end up uploading multiple SARIF files, instead of one file. I will investigate and change the type resolution when it is ready in the future.

@chao2zhang
Copy link
Member Author

chao2zhang commented Jan 9, 2021

I don't quite understand the following comment. Could you please elaborate?
What happens with enabled type resolution?
What happens with disabled type resolution?

Type resolution is still experimental that I see errors in the log. Running detekt without type resolution will also end up uploading multiple SARIF files, instead of one file. I will investigate and change the type resolution when it is ready in the future.

Running detekt with enabled type resolution on detekt yields compiler warnings:

                                                         ^
//Users/cazhang/detekt/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/baseline/BaselineFormatSpec.kt:56:45: error: cannot access 'Baseline': it is internal in 'io.gitlab.arturbosch.detekt.core.baseline'
            val savedBaseline by memoized { Baseline(setOf("4", "2", "2"), setOf("1", "2", "3")) }
                                            ^
//Users/cazhang/detekt/detekt-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/baseline/BaselineFormatSpec.kt:61:30: error: cannot access 'BaselineFormat': it is internal in 'io.gitlab.arturbosch.detekt.core.baseline'
                val format = BaselineFormat()
                             ^

Running detekt with enabled type resolution will generate one file per source set(I need to correct my original PR summary), hence we need to upload multiple SARIF output to Github, which has not been battled-tested and is not on the official documentation yet.

Therefore, the github workflow I added in this PR uploads the SARIF output from detekt with disabled type resolution.

@schalkms
Copy link
Member

schalkms commented Jan 9, 2021

Thank you for the remark! I understand the current limitations.

@schalkms schalkms merged commit c87a0ac into detekt:master Jan 11, 2021
arturbosch pushed a commit that referenced this pull request Jan 16, 2021
* Integrate SARIF report with Github Actions

* Simplify workflow setup

* Set OriginalUriBaseIds in output report
@arturbosch arturbosch added this to the 1.16.0 milestone Jan 18, 2021
@arturbosch arturbosch added the housekeeping Marker for housekeeping tasks and refactorings label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup the correct pipeline for integration test of detekt gradle plugin
4 participants