Change category uniqueness test #872
Conversation
Turboscan only allows a single combination of tool name and automation details id for testing category uniqueness. Previously, the check in the action was not entirely correct since it only looked at the _category_ and not the combination of the category and the tool name. It's even more precise now since it is looking at the actual, computed value of the automation details id, rather than an inputted value of the category. This change also includes a refactoring where the action is now avoiding multiple parsing/stringifying of the sarif files. Instead, sarif is parsed once at the start of the process and stringified once, after sarif processing is completely finished.
|
Interesting...quite a few failing tests, but all of these are legitimately trying to upload multiple times with the same tool/automation id. Here are the failing jobs and they are all failing since they are merging runs from multiple files into a single sarif.
I understand why this is happening, but I'm not sure if this is the best behaviour. The default automation details id and tool are the same for all runs in a single job. So, we can't enforce uniqueness on them. However, since these really are analyses different languages, they really should have distinct categories. I guess it's too late now to change this without breaking existing users. I think I need to change things so that identical automation ids/tools in different runs of the same sarif are tolerated. |
|
I dont think I a lot of knowledge about the action - so I have assigned @Daverlo instead as he did the initial category work here. Happy to sync more generally on the behavior. But since https://github.com/github/code-scanning/issues/1555#issuecomment-843367976 (which was after the initial category work) I think you might have more information on the wanted direction than me :) |
There was a problem hiding this comment.
Up to you if you want to wait for further review, but I think this is good and implements the check correctly now.
The fact that the sanitization of the env var makes us throw the error is slightly more cases is fine I think. It's a corner case that you're using two categories that only differ in case or non-alphanumeric characters. I think we can just go with this for now and change it in the future if it ever becomes a problem.
src/upload-lib.ts
Outdated
| } | ||
| core.exportVariable(sentinelEnvVar, sentinelEnvVar); | ||
| } | ||
| } |
There was a problem hiding this comment.
This closing brace is indented one space too many? Not sure why the formatter isn't complaining about it as my editor spots it 🤔
There was a problem hiding this comment.
That explains the failing linting PR check.
A single SARIF file should be allowed to have duplicated categories.
aeisenberg
force-pushed
the
aeisenberg/category-with-tool
branch
from
43018ce to
ab1f709
Compare
|
I'll keep this open for another day or two to see if @Daverlo has any comments, and then I'll merge if there's nothing else. |
|
Merging this based on prior approval. |
Turboscan only allows a single combination of tool name and automation
details id for testing category uniqueness.
Previously, the check in the action was not entirely correct since it
only looked at the category and not the combination of the category
and the tool name.
It's even more precise now since it is looking at the actual, computed
value of the automation details id, rather than an inputted value of
the category.
This change also includes a refactoring where the action is now avoiding
multiple parsing/stringifying of the sarif files. Instead, sarif is
parsed once at the start of the process and stringified once, after
sarif processing is completely finished.
Merge / deployment checklist