Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#790: Add an ability to edit Summary Note instead of creating a new one #791

Closed

Conversation

alexintech
Copy link

@alexintech alexintech commented Aug 3, 2023

Closes #790

  • Added configuration option to allow edit existing Summary Note instead of creating a new thread;
  • Added tests for the current behavior;
  • GitlabMergeRequestDecoratorTest -> switch to JUnit 5;
  • Some efforts to simplify creation of mocks for GitlabMergeRequestDecoratorTest and minimize duplication.

@mc1arke
Copy link
Owner

mc1arke commented Sep 16, 2023

We used to edit summary notes but moved away from it for a few reasons:

  1. Gitlab does not notify when a note is edited, so reviewers don't know if something has changed
  2. No history is recorded in an edit, so any conversation that spawns from an item in the summary note may be misleading given the final summary note content
  3. Allowing configuration for this is non-standard so isn't covered under the Sonarqube documentation, and is therefore difficult to point people at

I'm presuming the driver behind this is that Gitlab doesn't collapse summary notes when the thread is resolved, so would suggest an alternative approach of creating a thread to add summary comments to with the initial message being Sonarqube Scan Result or something similar so that the resolved thread collapses to a small message

@alexintech
Copy link
Author

Sorry for the late response.

  1. Gitlab does not notify when a note is edited, so reviewers don't know if something has changed

Yes, it's true. But Gitlab notifies if all issues have been resolved.

Expect another situation. Developer opens MR, SonarQube summary note is OK. Then developer pushes new commits, and every time SonarQube is OK. It sends a lot of annoying notifications.

  1. No history is recorded in an edit, so any conversation that spawns from an item in the summary note may be misleading given the final summary note content

I don't know how exactly SonarQube Developer Edition decorates MR's. All I have found is this video. As I understand it updates existing summary note, and does not creates a new one.

Other tools also usually edit summary notes in Gitlab's MR. Gitlab's junit reports, code climate reports in MR also shows only the current state.

Also in our experience conversations usually spawns on individual line comments and never on summary note.

I presume that somebody should have a history for any change, so I added the configuration option to save the same behavior.

  1. Allowing configuration for this is non-standard so isn't covered under the Sonarqube documentation, and is therefore difficult to point people at

As I understand from the mentioned video, current behavior is already non-standard.

Many forks from this repository implements similar options, but that forks are not very popular or are not supported in long term. I suppose that making configuration options for some different behaviors would satisfy community needs.

Some additional documentation can be added to README or can be published some static docs on github pages.

I'm presuming the driver behind this is that Gitlab doesn't collapse summary notes when the thread is resolved, so would suggest an alternative approach of creating a thread to add summary comments to with the initial message being Sonarqube Scan Result or something similar so that the resolved thread collapses to a small message

Not only collapsing, also annoying notifications. I suppose that in your approach viewing the latest (actual) summary would not be convenient.

@mc1arke
Copy link
Owner

mc1arke commented Dec 29, 2023

The linked video doesn't show a re-scan of a Merge Request, only the manual change of the status of a finding and a (realtime?) submission of a change in status to Gitlab. The video appears to show that the existing comment is deleted and a new one submitted given the lack of edited marker and the timestamp of the summary comment, which would be a preferred option given the new message would result in a new notification being generated for anyone watching the MR.

I also don't want us adding more configuration flags into the plugin unless there's a strict requirement to need a flag (i.e. the plugin won't work without setting a certain value in a configuration flag). The plugin takes an opinionated approach to operation to save on the overhead of testing combinations of configuration or handing different set-ups when investigating issues.

@OneCyrus
Copy link
Contributor

OneCyrus commented Jan 8, 2024

I really want to this feature as well. currenlty a lot of the PRs look like the screenshot below in GitHub for us. It would be great if we could reduce the noise by updating an existing comment.
also another issue is that it doesn't work properly in monorepos as it doesn't respect project keys and closes everything even from other keys.

image

@mc1arke mc1arke closed this Feb 6, 2024
@hballangan-mdsol
Copy link

hballangan-mdsol commented Apr 12, 2024

I've been pushing sonarqube in our CI and this has been one of the reviews of our users as well. It creates too much noise since it notifies everyone for every commit. In github actions, if we want to get notified of failures then we only need to add the Quality Gate check action.

Ideally we only want to get emails if it's a user review, not for PR decorations.
Sonarqube issues are code smells and syntax-related that developers should fix themselves. Or to be left as a technical debt. Organic conversations in the PR would revolve around the code logic and implementation itself, not syntax-related issues generated by the static code analysis. So we won't really mind if we lose the previous report details.

But I do agree that we should caution against introducing new flags. So either we implement this, or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Gitlab] Edit Summary Note instead of creating a new one every time
4 participants