Add support for migrating secret scanning resolution comments #1241
Conversation
Unit Test Results811 tests 811 ✅ 22s ⏱️ Results for commit cc8aa0c. ♻️ This comment has been updated with latest results. |
| { | ||
| state, | ||
| resolution, | ||
| resolution_comment = resolutionComment ?? string.Empty |
There was a problem hiding this comment.
Why is it being set to empty string? If nothing is passed it makes sense for the default to be null.
| resolution_comment = resolutionComment ?? string.Empty | |
| resolution_comment = resolutionComment |
There was a problem hiding this comment.
Hello, thanks for the comments.
Added it that way to align with what is done for the dismissedComment for code scanning alerts (cf. https://github.com/github/gh-gei/blob/main/src/Octoshift/Services/GithubApi.cs#L933 )
var payload = state == "open"
? (new { state })
: (object)(new
{
state,
dismissed_reason = dismissedReason,
dismissed_comment = dismissedComment ?? string.Empty
});
await _client.PatchAsync(url, payload);Your suggestion makes sense though, so would you prefer me to do that in a separate pull request to align both changes, or change both it in this pull request?
There was a problem hiding this comment.
👋 ok got it. So I guess as you said the other one needs to be changed too. I think it's better to do that one in a separate PR.
There was a problem hiding this comment.
Understood. I have opened #1248 and added your suggestions there. Thanks again!
| @@ -2782,6 +2789,30 @@ public async Task UpdateSecretScanningAlert_Calls_The_Right_Endpoint_With_Payloa | |||
| _githubClientMock.Verify(m => m.PatchAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null)); | |||
| } | |||
|
|
|||
|
|
|||
| [Fact] | |||
| public async Task UpdateSecretScanningAlert_Replaces_Null_Resolution_Comment_With_Empty_String() | |||
There was a problem hiding this comment.
Based on my earlier comment we probably won't need this test.
There was a problem hiding this comment.
Agreed.
And if we align with code scanning, we probably don't need that test either: UpdateCodeScanningAlert_Replaces_Null_Dismissed_Comment_With_Empty_String
As per previous comment, happy to do a separate pull request to align both code scanning and secret scanning, or do the changes in this one.
There was a problem hiding this comment.
I have opened #1248 and added your suggestions there. Thanks again!
Happy to work on it next.
| } | ||
|
|
||
| [Fact] | ||
| public async Task One_Secret_Updated_With_No_Resolution_Comment() |
There was a problem hiding this comment.
Not sure if this adds value, seems like a bit of an overkill IMO.
There was a problem hiding this comment.
What would be your preferred option:
- remove completely that test. This would align with the current tests for code scanning where a dismissedComment is always set (so we are not testing the use case where no comments were set)
- rework both code scanning and secret scanning tests to "squeeze" one alert with no comment (to exercise the code, but not as a separate test case).
- keep that test separate and add a similar overkill test in code scanning 😄
There was a problem hiding this comment.
So I am leaning towards option 1, since the way I think about this is a scenario where no comments are set still means we're setting it but to a null value and we have test coverage that shows we indeed use the passed value and pass it along.
There was a problem hiding this comment.
Thanks again for all the comments. I have now removed that test case.
|
Added support for migrating secret scanning resolution comments
ThirdPartyNotices.txt(if applicable)Fixes #1240