Unconditionally disable combining SARIF files for GHES 3.18 - #2961
Conversation
Sorry, something went wrong.
| semver.lt(githubVersion.version, "3.18.0") | ||
| ) { | ||
| return false; | ||
| if (githubVersion.type === GitHubVariant.GHES) { |
There was a problem hiding this comment.
This logic looks good, but it took a while for me to parse it because there there's a double negative: "should disable" and false means it's enabled.
It might make things easier to understand if you negate the function name and all of the return values. So, allowCombinedSarifFiles or something.
It may not actually be any simpler, but maybe it's worth a try.
This is a non-blocking comment.
Sorry, something went wrong.
There was a problem hiding this comment.
I agree that it's somewhat hard to parse, but since being "enabled" is the default case for now, I think having this as a shouldDisable method makes sense for now. Once the feature flag has been removed, I think it would make to make a change to make being "disabled" the default case.
Sorry, something went wrong.
There was a problem hiding this comment.
Fine by me.
Sorry, something went wrong.
This unconditionally disables combining of SARIF files with non-unique categories for GHES 3.18 since this feature is unsupported on GHES 3.18. For dotcom (and GHEC-DR), we'll depend on the feature flag for now.
Merge / deployment checklist