Undetermined reason for undetermined contextual analysis status #155
Conversation
formats/sarifutils/sarifutils.go
Outdated
| } | ||
|
|
||
| func GetRuleUndeterminedReason(rule *sarif.ReportingDescriptor) string { | ||
| return GetRuleProperty("undetermined_reason", rule) |
There was a problem hiding this comment.
The function split is positive overall - but I don't see any other usage for GetRuleProperty, so in that case, I don't see a reason for the dedicated fucntion.
Couldn't we use it for the applicability status itself?
There was a problem hiding this comment.
We can use it for applicability status as it is also received in the Rule property. Do you want me to change applicability fetch to this function ?
There was a problem hiding this comment.
yes - otherwise the usage of the this generic function just one misses the point IMO
There was a problem hiding this comment.
Yes please do use the new GetRuleProperty when applicable. but the GetRuleUndeterminedReason does not belong here. please move it to jasutils package.
This files is only for utils to access, create or set values in sarif
| @@ -13,14 +13,16 @@ func CreateRunWithDummyResults(results ...*sarif.Result) *sarif.Run { | |||
| return run | |||
| } | |||
|
|
|||
| func CreateRunWithDummyResultAndRuleProperties(property, value string, result *sarif.Result) *sarif.Run { | |||
| func CreateRunWithDummyResultAndRuleMultipleProperties(result *sarif.Result, properties, values []string) *sarif.Run { | |||
There was a problem hiding this comment.
The function name indicates that we have a function for multipleProperties, and a one for single.
If it's just that one function - I think you can keep the name.
| @@ -13,14 +13,16 @@ func CreateRunWithDummyResults(results ...*sarif.Result) *sarif.Run { | |||
| return run | |||
| } | |||
|
|
|||
| func CreateRunWithDummyResultAndRuleProperties(property, value string, result *sarif.Result) *sarif.Run { | |||
| func CreateRunWithDummyResultAndRuleMultipleProperties(result *sarif.Result, properties, values []string) *sarif.Run { | |||
There was a problem hiding this comment.
Also - why did you switch the order of the args?
There was a problem hiding this comment.
I thought it was nicer to have the arrays at the end of the function, what do you think?
| @@ -96,6 +96,7 @@ type CveRow struct { | |||
| type Applicability struct { | |||
| Status string `json:"status"` | |||
| ScannerDescription string `json:"scannerDescription,omitempty"` | |||
| UndeterminedReason string `json:"undeterminedReason,omitempty"` | |||
There was a problem hiding this comment.
where do we use it? UndeterminedReason is being inserted into - but not called...
There was a problem hiding this comment.
It is only in simple-json, and it is under CveRow (CveRow has Applicability struct as one of its components, it gets printed automatically)
| run.Tool.Driver.Rules[0].Properties[property] = value | ||
| run.Tool.Driver.Rules[0].Properties = make(sarif.Properties, len(properties)) | ||
| for index, _ := range properties { | ||
| run.Tool.Driver.Rules[0].Properties[properties[index]] = values[index] |
There was a problem hiding this comment.
suggest makeing sure that len(values)==len(properties)
formats/sarifutils/sarifutils.go
Outdated
| } | ||
|
|
||
| func GetRuleUndeterminedReason(rule *sarif.ReportingDescriptor) string { | ||
| return GetRuleProperty("undetermined_reason", rule) |
There was a problem hiding this comment.
Yes please do use the new GetRuleProperty when applicable. but the GetRuleUndeterminedReason does not belong here. please move it to jasutils package.
This files is only for utils to access, create or set values in sarif
barv-jfrog
added
the
safe to test
github-actions
bot
removed
the
safe to test
…o undetermined-text
barv-jfrog
added
the
safe to test
github-actions
bot
removed
the
safe to test
devbranch.go vet ./....go fmt ./....Description:
Scanners return a reason for why the result was undetermined. This result is on the scanner level (Rule), and not on evidence level in contrary to applicable status which returns "reason" on the evidence level. We need to support this undetermined reason and show it next to "applicable" and "scannerDescription".