Add automationdetails id to runs - #446
Daverlo merged 5 commits into
Conversation
Sorry, something went wrong.
starcke
left a comment
There was a problem hiding this comment.
Generally looks good to me. Asked for one more test. It'll probably be good to get a sanity check from someone that have worked on the action.
Sorry, something went wrong.
| analysisKey, | ||
| "{}" | ||
| ); | ||
| t.deepEqual(modifiedSarif, expectedSarif); |
There was a problem hiding this comment.
Should we add test to assert that if there already is an ID then it is not overwritten?
Sorry, something went wrong.
| if (run.automationDetails === undefined) { | ||
| run.automationDetails = { | ||
| id: automationID, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Do you also want to handle the case where run.automationDetails is defined, but id is missing? Eg-
run: {
...
automationDetails: { }
}
Sorry, something went wrong.
There was a problem hiding this comment.
My initial idea was not to handle that case. The are some other fields in the object that serve the same purpose (guid and correlationGuid), so one option would be to recreate the id based on that, but following the example is not the case.
"id": "Nightly CredScan run for sarif-sdk/master/x86/debug/2018-10-05", "guid": "11111111-...", "correlationGuid": "22222222-...."
The correlationGuid identifies the Nightly CredScan run for sarif-sdk/master/x86/debug/ part of the id but there is no direct way of getting one from the other.
Sorry, something went wrong.
starcke
left a comment
There was a problem hiding this comment.
👍
Sorry, something went wrong.
We are starting the process of deprecating the use of analysis_key and environment in code scanning, and instead use the automationDetails property. This PR starts populating this field, using the analysis_key and environment.
Merge / deployment checklist