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

Add configuration option to delete notes instead of resolving them #494

Closed
wants to merge 2 commits into from

Conversation

teake
Copy link
Contributor

@teake teake commented Nov 23, 2021

This will reduce PR decoration clutter where lots of commits are added. Note that it only works for GitLab.

To reduce merge conflicts, the commit in this PR is done on top of that of #487. Hence this PR contains also the diff of #487. To see the diff of the relevant commit go to 21e3ec4.

Teake Nutma added 2 commits November 17, 2021 17:06
This will reduce PR decoration clutter where lots of commits are added.
It only works for GitLab at the moment.
@felixoi
Copy link

felixoi commented Nov 23, 2021

I've done something similar in a fork of this project (forked due to the lack of time to make it pr-ready). My changes work differently in some points:

  • Always post the summary as first comment (before issue comments)
  • Never delete the old summary. Only update the comment and unresolve/resolve the summary thread if necessary
  • Never delete issue threads as there might be a discussion ongoing

@teake
Copy link
Contributor Author

teake commented Nov 23, 2021

@felixoi I've looked at "post as first comment" a while back, but that requires elevated privileges according to the docs so I abandoned it.

Your second point is a good idea, since that would reduce the number of emails GitLab sends out. Unfortunately I've spent already too much time on this, so I'm going to run locally with what's in this PR.

@felixoi
Copy link

felixoi commented Nov 23, 2021

@teake By "first comment" I though about just posting it before the issue comments, not manipulating the created_at date. Most of the time it will be the first comment in a merge request then (cannot be guaranteed tho).

I'm also time limited right now, this one is a good start tho. If it would get merged, I could make a new pull request with some more options/changes to the behavior.

@mc1arke
Copy link
Owner

mc1arke commented Dec 18, 2021

Similar to my comments on #487: I don't really want to have the plugin providing non-standard configuration options, and definitely don't want any options that are misleading to users.

The plugin moved away from deleting comments due to it leaving orphaned discussions and system notes in Gitlab so didn't actually reduce the noise in the UI and reduced the overall UX. I'd be ok with the summary comments being deleted if there are no child comments on them, but am not convinced deleting annotations is the right thing to do.

@felixoi
Copy link

felixoi commented Dec 19, 2021

@mc1arke Is the standard configuration that it just posts the summary again and again or is it just editing the first post?

@mc1arke
Copy link
Owner

mc1arke commented Dec 19, 2021

@felixoi It posts a new summary comment on each run, and resolves any existing summary comment discussions

@teake
Copy link
Contributor Author

teake commented Dec 20, 2021

Similar to my comments on #487: I don't really want to have the plugin providing non-standard configuration options, and definitely don't want any options that are misleading to users.

Could you elaborate a bit on why you find this misleading? To be frank, I don't see it.

My main beef with resolved summary comments is that you cannot collapse or hide them in the GitLab UI. Any child comments can be hidden, but the parent comment (which is quite big in terms of vertical height!) cannot. So on MRs with lots of commits and SonarQube summary comments, there will be a lot of space taken up with redundant information. So I do really think removing the summary comments reduces clutter.

But yeah, orphaned comments are not particularly nice. Alas, will not be able to find any more time to put into this MR, so feel free to close.

@austen-herbst
Copy link

As a user of this plugin with GitLab, we have the same issue as @teake with the resolved summary comments. They take up so much space (since they can't be collapsed) and really clutter up the MR activity timeline. We might have 10 of these summaries in the timeline if the build ran 10 times. I like the idea of not removing them if there is a thread attached though.

I think it makes sense to never delete the individual issue comments, as those are collapsed when resolved.

@felixoi
Copy link

felixoi commented Dec 21, 2021

As stated before, I've experienced exactly the same as @austen-herbst and @teake. An individual issue comment should never be removed (they're getting collapsed by Gitlab automatically). My solution is still to just update the first summary comment instead of creating a new one every time. Works pretty well.

@mc1arke mc1arke closed this Dec 29, 2021
@mably
Copy link

mably commented Jul 29, 2022

@teake looks like I just tried to reimplement your own PR ;) #648

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.

5 participants