Azure DevOps PR decoration undocumented behavior?

sonarcloud
vsts

(S.V.Groeneveld) #1

We’re using SonarCloud integration with Azure DevOps, and have enabled PR decoration.
This all works fine, we can see comments being generated in Pull Requests.

But we’re noticing a few oddities:

  • On a large pull request there were 20 comments generated by SonarCloud, but when we navigated to the SonarCloud PR analysis page there were actually 75 issues found. And every PR build it would add a few new comments to the PR… Is there some undocumented max limit on the number of comments it can post on a PR?

  • When we change the issue status within SonarCloud, it automatically updates the status accordingly in the Pull Request. But when we set the status in the PR to “WontFix” upon next build it will mark the issue in SonarCloud as “Fixed” (instead of “WontFix”).

  • SonarCloud comments seem to be deleted from PR’s after resolving them. We never delete our manual comments, since this is making it difficult to read historical PR’s and understand the reason things got changed. Why is SonarCloud doing this?


(Fabrice Bellingard) #2

I’m not aware of such limit. Maybe @Julien_HENRY can give more insights.

When you say “when we set the status in the PR to WontFix”, you mean when you do it in Azure DevOps UI?

I guess the idea is to not “pollute” too much the PR with useless comments (when issues are fixed, there’s no more problem). You would expect the comment to be set to “Closed”?


(Stephen Larkin) #3

We have also encountered the 20 comment limit cap on VSTS, we had just assumed it was some sort of limitation.


(S.V.Groeneveld) #4

Sorry, that was indeed unclear. All status changes that are done in SonarCloud get reflected immediately in the Azure DevOps PR, that works fine.
But when we change the status in the Azure DevOps PR to “WontFix”, the status on SonarCloud will be “Fixed” after next build.

Exactly, either marked as closed, resolved or wontfix… which are the three “inactive” states in Azure DevOps. But it’s the author of the PR that should decide on that status…
We have people actually replying to SonarCloud’s comments how they fixed it, or how they defend why they won’t fix it. If SonarCloud then deletes its own comment, you end up with a bunch of replies to a “* Comment deleted
I don’t think any PR system hard-deletes comments when issues are resolved, but most implement some way to automatically hide resolved or closed comments.


(Fabrice Bellingard) #5

Thanks for the details. I tried to reproduce this behaviour, and I could not. There must be something missing in the scenario. Are you sure that some part of the code (even not on the very same line) did not get changed and therefore resolved the issue?

Indeed, when relying on a comment-based feature to show automatic review issues, maybe we should behave that way. I’m creating VSTS-184 to track this.

Thanks!


(Peter Connor) #6

Has the non-delete feature been implemented now? Because we are now seeing all of our SonarCloud comments are not being deleted and always remain as ‘active’ once they have been created.


(Fabrice Bellingard) #7

As you can see on VSTS-184, nothing has been done on this topic for now.

Can you clarify what changed on your end? I’ve looked at some projects on my side, and everything seems to work as expected.


(Peter Connor) #8

Hi @Fabrice_Bellingard,

The comment feature works as expected, but once we solve the issue the comment is no longer deleted by the SonarCloud user, whereas it always used to be.

The screenshot shows the commented code has been deleted, but the SonarCloud comment still remains.

Thanks,
Pete


(Fabrice Bellingard) #9

Hi @pconnor!

Following your message, I’ve done some tests to check if something had changed in the behaviour, and on my side, everything works as expected: the comments get deleted/removed when I fix issues inside the branch of a given PR. Here’s what I did for my tests:

  • I create a branch
  • I update one file to add an obvious issue (like adding a public field to a class)
  • I create a PR for that branch
  • Once the analysis is done, I check that the issue (comment) is available on the PR
  • Then I fix the issue on the branch and push the change
  • Once the analysis is done, I check that the issue (comment) is no longer visible

I did a couple of variations (like answering the issue comment before fixing it), and every time it works as expected (= the issue comment disappears).

I suspect there is something specific in your use case that we’re missing, but I don’t see what.


(S.V.Groeneveld) #10

The main question is: ‘what is expected?’…

I noticed yesterday that a SonarCloud comment on a fixed issue was not deleted upon next analysis during PR build. And I’m totally happy with that!

We always have developers reply to a comment and mark it as ‘Resolved’ in Azure DevOps right after they push their fix. Would that manual status change have any influence on SC behavior?


(Fabrice Bellingard) #11

The currently expected behaviour is: the issue comments are no longer visible from the PR when an issue gets fixed in a PR.

I’d like to get @pconnor’s perspective on this. It seems to me that you might not expect the same behaviour. Should issue comment be removed or not when an issue gets fixed?

Thanks for this info! This might help reproduce this “unexpected” behaviour.


(Peter Connor) #12

We have just recently obtained ISO 27001 accreditation, so anything that can be used as an audit trail / show we are sticking to procedures is a good thing. I guess in an ideal world for us would be when an issue raised by SonarCloud has been fixed, the original comment would be marked as fixed and there would be an additional comment from SonarCloud under the original one saying “This is has now been resolved”.

The only reason I questioned this change for us originally is our devs were starting to think that they hadn’t fixed their issues because the comments were still remaining, it was just different to the behaviour they were used to. But if we can show in some form or another than the issue has been fixed (status / additional comment) then that would be ideal.