Add setting to limit number of issues posted to a pull request across multiple runs

[janniksam]

Lets imagine, in theory, my pullrequest is fixing somehing in a old legacy class and 200 warnings pop up during code analyse of my PR build. Most of aren't results from by my changes. MaxIssuesToPost is set to 50 warnings, so 50 warnings will be output to the Pull Request..

Now I'll fix all the 50 warnings in my next commit to the PR. The build will be rerun and reanalyse everything. Will it output the next 50 warnings or does it detect that it already detect the 50 submitted warnings?

It already gave me 50, and I wouldn't want to fix another 50, when it comes to legacy code. We're aiming for small but steady steps to a zero warning policy. (currently we're at 14000 ;-))

[pascalberger]

Will it output the next 50 warnings or does it detect that it already detect the 50 submitted warnings?

Yes, in this case it will mark the first 50 as resolved and post the next 50.

This is how I currently approach this scenario:

1. We don't mix fixing of existing code analysis issues with other PRs. So if we create a PR changing something in an existing code base, we normally won't fix the issues in the same PR. If you have a policy that comments need to be resolved you can mark the comments as Won't Fix (if you mark them as Resolved they will be reopened the next run by the addin since they are not actually resolved)
2. We added logic to our build script, which filters out existing issues and only posts issues which are introduced by this PR to the build script. This is done by the following steps:
3. Persist issues for every build and store them as build artifacts
4. Read target branch from PR
5. Detect common Git commit for source and target branch
6. Detect build for the common Git commit
7. Read issues from build artifacts
8. Deserialize issues
9. Filter existing issues from the list of issues posted to the pull request

For most of the steps above there are aliases and helpers available, but there's currently no out-of-the box implementation for this. It might be possible to add this to Cake.Issues.Recipe (cake-contrib/Cake.Issues.Recipe#33), but I'm not sure how easy it is to implement this in a generic way. But if you're interested in this approach I definitely help you with code snippets.

Another possibility would be to introduce a new setting which limits number of issues posted to a single pull request across multiple build runs. This should be possible, since we have marked issues posted by the addin and are already reading them back.

Let me know if any of these approaches help in your use case.

[janniksam]

Another possibility would be to introduce a new setting which limits number of issues posted to a single pull request across multiple build runs. This should be possible, since we have marked issues posted by the addin and are already reading them back.

Thanks for always getting back to me so quickly. Thats really helpful.

Unfortunately I don't have the spare time to work at that first time-connsuming algorithm you wrote down for me (which I've understood completely) . Maybe in the future, but I am already way past my time estimate implementing the code analysis for my company.

The approach, that I had personally in mind, you actually thought about aswell:

"Another possibility would be to introduce a new setting which limits number of issues posted to a single pull request across multiple build runs. This should be possible, since we have marked issues posted by the addin and are already reading them back."

That seems to be a very quick (subjective statement :-)) but ok'ish way to do the "capping". I figured, you could detect the already posted comments. I know that it does have some negative aspects (e.g. new warnings could be skipped, when 100+ already existing warnings are priorized), but I could live with that for now.

I would love to see that, if it doesnt take too much time. If I can help you somehow, please let me know.

[janniksam]

I really don't want to rush/annoy you and its not that urgent.
But maybe you have a time estimate, whether it can take days or weeks or even months?

I might be able to check out the code myself and spend some of my free time and try to get a switch implemented for that feature.

[pascalberger]

I started working on it yesterday and hope to finish it by the end of this weekend (probably sooner). While all basic parts are already there, there's some logic involved which needs to be done correctly and I want to get this right with proper test coverage.

If you are interested to give it a try yourself I can show you what needs to be changed.

[pascalberger]

@janniksam If you want to test you can checkout the changes from #138 and build the addin locally. I can also later provide a beta version for testing once the PRs are merged.

It adds a new setting ReportIssuesToPullRequestSettings.MaxIssuesToPostAcrossRuns which is applied after ReportIssuesToPullRequestSettings.MaxIssuesToPostForEachIssueProvider and ReportIssuesToPullRequestSettings.MaxIssuesToPost.

Behavior is like this. Assuming you have the following issues:

• 70 issues of provider A
• 70 issues of provider B

And the following settings:

• MaxIssuesToPostForEachIssueProvider: 20
• MaxIssuesToPost: 30
• MaxIssuesToPostAcrossRuns: 55

The following will happen:

• First run: 30 issues are posted but not more than 20 of either provider A or B
• If you fix the issues and run again: 25 issues are posted but not more than 20 of either provider A or B
• If you fix the issues and run again: The remaining 85 won't be posted in this pull request

[janniksam]

Looks pretty good 👍 Good job. Will test it tomorrow or maybe on monday.

BTW: I wonder, what happens on the first run. Which provider will win? Is it random (one provider will output 20 and one 10 issues) or is it distributed equally between both providers (each provider will output 15 issues)?

[pascalberger]

BTW: I wonder, what happens on the first run. Which provider will win? Is it random (one provider will output 20 and one 10 issues) or is it distributed equally between both providers (each provider will output 15 issues)?

Issues are filtered by priority (higher priorities are kept) and issues with a file path are prioritized over issues without a file path. Beside that it is undetermined.

[janniksam]

Hey Pascal. Did you create a beta (or even final) release for this feature? I would be interested in that. 👍

[pascalberger]

Not yet, but can create a beta version later today

[pascalberger]

@janniksam I've released 0.8.1-beta0001 containing this feature