Conversation
autonomousapps
left a comment
There was a problem hiding this comment.
Thanks for this PR. I have not done a thorough review because I don't have time at the moment, but I'm very open to seeing this progress and eventually get merged.
We ought to be able to resolve the issue with projectHealth not having access to the reporting handler. I think for this to be considered feature-complete, it would have to work for both buildHealth and projectHealth.
The build file scanning may have some issues, but I haven't thoroughly inspected it. As you know, the plugin's analysis uses Gradle's APIs to find dependencies—it doesn't actually scan build scripts unless it's trying to rewrite them. So that'll have to be done on a best-effort basis (which is what I think you're doing).
|
|
||
| private val SARIF_TOOL = Tool( | ||
| driver = ToolComponent( | ||
| guid = "f9137358-f4fb-44f2-8300-39ca0b85fb77", |
There was a problem hiding this comment.
From what I understand, it's just a static unique identifiable ID for every tool. I just generated a random UUID and plopped it there.
| fromConfiguration = theRemove.fromConfiguration!!, | ||
| toConfiguration = theAdd.toConfiguration!! | ||
| toConfiguration = theAdd.toConfiguration!!, | ||
| declarationLineNumber = theRemove.buildFileDeclarationLineNumber ?: theAdd.buildFileDeclarationLineNumber |
There was a problem hiding this comment.
theAdd won't have a current declaration. The only things that would have a current declaration relate to remove advice and change advice.
There was a problem hiding this comment.
Ah, you are right. Do I remove this? I have added it to all types just for consistency's sake.
|
I had a thought about this and I think I do not want to modify the *this should be opt-in, based on user preference. public data class SarifAdvice( // name TBD
advice: Advice,
lineNumber: Int?,
)And then we could emit a sorted set of There are already examples in the project of configuring something in the root and having that value picked up safely in subprojects. If you need pointers, please let me know. |
|
So you would want to generate yet another json next to the regular advice json just for an extra property and then consume that json in addition to the regular json? Sounds a pretty complicated thing to do. Or did I miss the intention here? |
Yes, that's correct. And also that feature should be opt-in, based on user preference (as expressed in the In my opinion, this is actually easier than what you've spiked here. json is cheap. |
PR adds an ability for the plugin to also export the report in the standard
.sarifformat, in addition to the txt.Would appreciate feedback on some things:
build.gradle(.kts)file and just string matching of the advice's coordinates. Is that okay? Could we come up with a better way to do this?buildHealthtask, but not theprojectHealth. The issue is that individual projects do not have access to the reporting config and thus could not enable this. If necessary, I could addpublic fun reporting(action: Action<ReportingHandler>)to theDependencyAnalysisSubExtensionand add sarif output to theprojectHealth(This PR is based on the
3.5.1due to #1653, as it makes testing easier for me. When the code is final, I can resolve conflicts and merge latest version in)this fixes #1094